Messages in this thread Patch in this message | | | Date | Mon, 4 May 2020 11:28:46 -0400 (EDT) | From | Mathieu Desnoyers <> | Subject | Re: [PATCH] percpu: Sync vmalloc mappings in pcpu_alloc() and free_percpu() |
| |
----- On May 4, 2020, at 11:12 AM, Joerg Roedel jroedel@suse.de wrote:
> On Thu, Apr 30, 2020 at 10:39:19PM -0400, Steven Rostedt wrote: >> What's so damn special about alloc_percpu()? It's definitely not a fast >> path. And it's not used often. > > Okay, I fixed it in the percpu code. It is definitly not a nice > solution, but having to call vmalloc_sync_mappings/unmappings() is not a > nice solution at any place in the code. Here is the patch which fixes > this issue for me. I am also not sure what to put in the Fixes tag, as > it is related to tracing code accessing per-cpu data from the page-fault > handler, not sure when this got introduced. Maybe someone else can > provide a meaningful Fixes- or stable tag. > > I also have an idea in mind how to make this all more robust and get rid > of the vmalloc_sync_mappings/unmappings() interface, will show more when > I know it works the way I think it does. > > Regards, > > Joerg > > From c616a9a09499f9c9d682775767d3de7db81fb2ed Mon Sep 17 00:00:00 2001 > From: Joerg Roedel <jroedel@suse.de> > Date: Mon, 4 May 2020 17:11:41 +0200 > Subject: [PATCH] percpu: Sync vmalloc mappings in pcpu_alloc() and > free_percpu() > > Sync the vmalloc mappings for all page-tables in the system when > allocating and freeing per-cpu memory. This is necessary for > architectures which use page-faults on vmalloc areas. > > The page-fault handlers accesses per-cpu data when tracing is enabled, > and fauling again in the page-fault handler on a vmalloc'ed per-cpu area > will result in a recursive fault. > > Signed-off-by: Joerg Roedel <jroedel@suse.de> > --- > mm/percpu.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/mm/percpu.c b/mm/percpu.c > index d7e3bc649f4e..6ab035bc6977 100644 > --- a/mm/percpu.c > +++ b/mm/percpu.c > @@ -1710,6 +1710,20 @@ static void __percpu *pcpu_alloc(size_t size, size_t > align, bool reserved, > trace_percpu_alloc_percpu(reserved, is_atomic, size, align, > chunk->base_addr, off, ptr); > > + /* > + * The per-cpu buffers might be allocated in the vmalloc area of the > + * address space. When the architecture allows faulting on the vmalloc > + * area and the memory allocated here is accessed in the page-fault > + * handler, the vmalloc area fault may be recursive and could never be > + * resolved. > + * This happens for example in the tracing code which allocates per-cpu > + * and accesses them when tracing page-faults. > + * To prevent this, make sure the per-cpu buffers allocated here are > + * mapped in all PGDs so that the page-fault handler will never fault > + * again on them. > + */ > + vmalloc_sync_mappings();
Placing this here is inefficient. It syncs mappings for each percpu allocation. I would recommend moving it right after __vmalloc() is called to allocate the underlying memory chunk instead:
static void *pcpu_mem_zalloc(size_t size, gfp_t gfp) { if (WARN_ON_ONCE(!slab_is_available())) return NULL;
if (size <= PAGE_SIZE) return kzalloc(size, gfp); else { void *p = __vmalloc(size, gfp | __GFP_ZERO, PAGE_KERNEL); /* Add comments here ... */ vmalloc_sync_mappings(); return p; } } > + > return ptr; > > fail_unlock: > @@ -1958,6 +1972,12 @@ void free_percpu(void __percpu *ptr) > > trace_percpu_free_percpu(chunk->base_addr, off, ptr); > > + /* > + * See comment at the vmalloc_sync_mappings() call in pcpu_alloc() for > + * why this is necessary. > + */ > + vmalloc_sync_unmappings();
I wonder why we'd ever need to explicitly invoke vmalloc_sync_unmappings(). Leaving a stale PTE mapping in place to be lazily unmapped does not seem to hurt even the tracing use-cases. Why add this call to vmalloc_sync_unmappings() at all ?
*If* this ends up being needed, it should be moved to:
static void pcpu_mem_free(void *ptr) { /* Add comments here... */ if (is_vmalloc_addr(ptr)) vmalloc_sync_unmappings(); kvfree(ptr); } So it is only called before the underlying vmalloc'd chunk is freed, rather than at each and every percpu free.
Thanks,
Mathieu
> + > spin_unlock_irqrestore(&pcpu_lock, flags); > > if (need_balance) > -- > 2.12.3
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
| |