Re: [PATCH] MN10300: Fix the PERCPU() alignment to allow for workqueues

From: Tejun Heo <tj@kernel.org>
To: David Howells <dhowells@redhat.com>
Cc: torvalds@osdl.org, akpm@linux-foundation.org,
	linux-am33-list@redhat.com, linux-kernel@vger.kernel.org,
	Akira Takeuchi <takeuchi.akr@jp.panasonic.com>,
	Mark Salter <msalter@redhat.com>
Subject: Re: [PATCH] MN10300: Fix the PERCPU() alignment to allow for workqueues
Date: Tue, 26 Oct 2010 14:14:39 +0200	[thread overview]
Message-ID: <4CC6C62F.1000804@kernel.org> (raw)
In-Reply-To: <12101.1288088542@redhat.com>

Hello,

On 10/26/2010 12:22 PM, David Howells wrote:
>> Can you please double check the bug doesn't trigger with the section
>> alignment updated?
> 
> It can be made to trigger consistently without the change, and simply updating
> that alignment makes it go away.  It seems unlikely that it's affecting
> subsequent stuff in the final link since the PERCPU() is immediately followed
> by an alignment to PAGE_SIZE:
> 
> 	PERCPU(PAGE_SIZE)
> 	. = ALIGN(PAGE_SIZE);
> 
> I've attached the kernel log below.  CPUID is 0 indicating this happened on
> CPU 0 (the boot CPU).

Ah, I see now.  The actual areas are properly aligned but the percpu
address is determined as offset from the percpu output section base so
the percpu pointers in the percpu address space end up misaligned with
the actual kernel addresses and the code in workqueue checks the
address in percpu AS, so, yeap, it's caused by the misalignment of the
percpu section.  Except for triggering BUG_ON(), it shouldn't cause a
real issue tho as work_data points to the translated addresses in the
kernel AS for specific CPU.  Needs to be fixed anyways.

>> That said, I think it might be better to just remove the alignment parameter
>> from the macro and force align to PAGE_SIZE.
> 
> That's not necessarily good.  Two arches to note:
> 
> 	arch/x86/kernel/vmlinux.lds.S:  PERCPU(THREAD_SIZE)

I don't think the current percpu allocator honors alignment larger
than PAGE_SIZE no matter how large the alignment for the percpu output
section is.  I'll look into it deeper but I think we might just have
been lucky and the alignment somehow didn't bite us yet.  The only
user of THREAD_SIZE mask at this point seems to be cpu_init().  Maybe
we can remove this requirement.  I'll look into it.

> which may be bigger than PAGE_SIZE and:
> 
> 	arch/frv/kernel/vmlinux.lds.S:  PERCPU(4096)
> 
> FRV's page size is 16KB, so on that we really don't want it to be PAGE_SIZE.

Why not?  It's in the init section which will be freed anyway and with
the kernel image compression it's not even gonna add any noticeable
amount to the kernel image size.  There isn't any benefit in using
anything smaller than PAGE_SIZE for alignment.  Also, percpu allocator
guarantees alignment requirement upto PAGE_SIZE is honored.  If the
output section uses smaller alignment, the percpu AS will end up being
misaligned.

Thanks.

-- 
tejun