Date:	Thu, 20 Nov 2014 14:06:29 -0800
From:	Andy Lutomirski <luto@...capital.net>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	Tejun Heo <tj@...nel.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Dave Jones <davej@...hat.com>, Don Zickus <dzickus@...hat.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>,
	"the arch/x86 maintainers" <x86@...nel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Arnaldo Carvalho de Melo <acme@...stprotocols.net>
Subject: Re: frequent lockups in 3.18rc4

On Thu, Nov 20, 2014 at 1:58 PM, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Thu, 20 Nov 2014, Tejun Heo wrote:
>> On Thu, Nov 20, 2014 at 12:50:36AM +0100, Frederic Weisbecker wrote:
>> > > Are we talking about different per cpu allocators here or am I missing
>> > > something completely non obvious?
>> >
>> > That's the same allocator yeah. So if the whole memory is dereferenced,
>> > faults shouldn't happen indeed.
>> >
>> > Maybe that was a bug a few years ago but not anymore.
>>
>> It has been always like that tho.  Percpu memory given out is always
>> populated and cleared.
>>
>> > Is it possible that, somehow, some part isn't zeroed by pcpu_alloc()?
>> > After all it's allocated with vzalloc() so that part could be skipped. The memset(0)
>>
>> The vzalloc call is for the internal allocation bitmap not the actual
>> percpu memory area.  The actual address areas for percpu memory are
>> obtained using pcpu_get_vm_areas() call and later get populated using
>> map_kernel_range_noflush() (flush is performed after mapping is
>> complete).
>>
>> Trying to remember what happens with vmalloc_fault().  Ah okay, so
>> when a new PUD gets created for vmalloc area, we don't go through all
>> PGDs and update them.  The PGD entries get faulted in lazily.  Percpu
>> memory allocator clearing or not clearing the allocated area doesn't
>> have anything to do with it.  The memory area is always fully
>> populated in the kernel page table.  It's just that the population
>> happened while a different PGD was active and this PGD hasn't been
>> populated with the new PUD yet.
>
> It's completely undocumented behaviour, whether it has been that way
> for ever or not. And I agree with Fredric, that it is insane. Actuallu
> it's beyond insane, really.
>
>> So, yeap, vmalloc_fault() can always happen when accessing vmalloc
>> areas and the only way to avoid that would be removing lazy PGD
>> population - going through all PGDs and populating new PUDs
>> immediately.
>
> There is no requirement to go through ALL PGDs and populate that stuff
> immediately.
>
> Lets look at the two types of allocations
>
>    1) Kernel percpu allocations
>
>    2) Per process/task percpu allocations
>
> Of course we do not have a way to distinguish those, but we really
> should have one.
>
> #1 Kernel percpu allocations usually happen in the context of driver
>    bringup, subsystem initialization, interrupt setup etc.
>
>    So this is functionality which is not a hotpath and usually
>    requires some form of synchronization versus the rest of the system
>    anyway.
>
>    The per cpu population stuff is serialized with a mutex anyway, so
>    what's wrong to have a globaly visible percpu sequence counter,
>    which is incremented whenever a new allocation is populated or torn
>    down?
>
>    We can make that sequence counter a per cpu variable as well to
>    avoid the issues of a global variable (preferrably that's a
>    compile/boot time allocated percpu variable to avoid the obvious
>    circulus vitiosus)
>
>    Now after that increment the allocation side needs to wait for a
>    scheduling cycle on all cpus (we have mechanisms for that)
>
>    So in the scheduler if the same task gets reselected you check that
>    sequence count and update the PGD if different. If a task switch
>    happens then you also need to check the sequence count and act
>    accordingly.
>
>    If we make the sequence counter a percpu variable as outlined above
>    the overhead of checking this is just noise versus the other
>    nonsense we do in schedule().

This seems like a reasonable idea, but I'd suggest a minor change:
rather than using a sequence number, track the number of kernel pgds.
That number should rarely change, and it's only one byte long.  That
means that we can easily stick it in mm_context_t without making it
any bigger.

The count for init_mm could be copied into cpu_tlbstate, which is
always hot on context switch.

>
>
> #2 That's process related statistics and instrumentation stuff.
>
>    Now that just needs a immediate population on the process->mm->pgd
>    aside of the init_mm.pgd, but that's really not a big deal.
>
> Of course that does not solve the issues we have with the current
> infrastructure retroactively, but it allows us to avoid fuckups like
> the one Frederic was talking about that perf invented its own kmalloc
> based 'percpu' replacement just to workaround the shortcoming in a
> particular place.
>
> What really frightens me is the potential and well hidden fuckup
> potential which lurks around the corner and the hard to debug once in
> a while fallout which might be caused by this.

The annoying part of this is that pgd allocation is *so* rare that
bugs here can probably go unnoticed for a long time.

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists