| Message ID | 1537944728-18036-1-git-send-email-kernelfans@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/slub: disallow obj's allocation on page with mismatched pfmemalloc purpose | expand |
On Wed, 26 Sep 2018, Pingfan Liu wrote: > - > if (unlikely(!freelist)) { > slab_out_of_memory(s, gfpflags, node); > return NULL; > } > > + VM_BUG_ON(!pfmemalloc_match(page, gfpflags)); > page = c->page; > - if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags))) > + if (likely(!kmem_cache_debug(s)) > goto load_freelist; > > /* Only entered in the debug case */ > - if (kmem_cache_debug(s) && > - !alloc_debug_processing(s, page, freelist, addr)) > + if (!alloc_debug_processing(s, page, freelist, addr)) > goto new_slab; /* Slab failed checks. Next slab needed */ > - > - deactivate_slab(s, page, get_freepointer(s, freelist), c); In the debug case the slab needs to be deactivated. Otherwise the slowpath will not be used and debug checks on the following objects will not be done. > - return freelist; > + else > + goto load_freelist; > } > > /* >
On Thu, Sep 27, 2018 at 12:14 AM Christopher Lameter <cl@linux.com> wrote: > > On Wed, 26 Sep 2018, Pingfan Liu wrote: > > > - > > if (unlikely(!freelist)) { > > slab_out_of_memory(s, gfpflags, node); > > return NULL; > > } > > > > + VM_BUG_ON(!pfmemalloc_match(page, gfpflags)); > > page = c->page; > > - if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags))) > > + if (likely(!kmem_cache_debug(s)) > > goto load_freelist; > > > > /* Only entered in the debug case */ > > - if (kmem_cache_debug(s) && > > - !alloc_debug_processing(s, page, freelist, addr)) > > + if (!alloc_debug_processing(s, page, freelist, addr)) > > goto new_slab; /* Slab failed checks. Next slab needed */ > > - > > - deactivate_slab(s, page, get_freepointer(s, freelist), c); > > In the debug case the slab needs to be deactivated. Otherwise the > slowpath will not be used and debug checks on the following objects will > not be done. > Got it. Thanks, Pingfan
On Thu, Sep 27, 2018 at 9:15 PM Pingfan Liu <kernelfans@gmail.com> wrote: > > On Thu, Sep 27, 2018 at 12:14 AM Christopher Lameter <cl@linux.com> wrote: > > > > On Wed, 26 Sep 2018, Pingfan Liu wrote: > > > > > - > > > if (unlikely(!freelist)) { > > > slab_out_of_memory(s, gfpflags, node); > > > return NULL; > > > } > > > > > > + VM_BUG_ON(!pfmemalloc_match(page, gfpflags)); > > > page = c->page; > > > - if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags))) > > > + if (likely(!kmem_cache_debug(s)) > > > goto load_freelist; > > > > > > /* Only entered in the debug case */ > > > - if (kmem_cache_debug(s) && > > > - !alloc_debug_processing(s, page, freelist, addr)) > > > + if (!alloc_debug_processing(s, page, freelist, addr)) > > > goto new_slab; /* Slab failed checks. Next slab needed */ > > > - > > > - deactivate_slab(s, page, get_freepointer(s, freelist), c); > > > > In the debug case the slab needs to be deactivated. Otherwise the > > slowpath will not be used and debug checks on the following objects will > > not be done. > > After taking a more closely look at the debug code, I consider whether the alloc_debug_processing() can be also called after get_freelist(s, page), then deactivate_slab() is not required . My justification is the debug code will take the same code path as the non-debug, hence the page will experience the same transition on different list like the non-debug code, and help to detect the bug, also it will improve scalability on SMP. Besides this, I found the debug code is not scalable well, is it worth to work on it? Thanks, Pingfan
On Sun, 30 Sep 2018, Pingfan Liu wrote: > > > In the debug case the slab needs to be deactivated. Otherwise the > > > slowpath will not be used and debug checks on the following objects will > > > not be done. > > > > After taking a more closely look at the debug code, I consider whether > the alloc_debug_processing() can be also called after get_freelist(s, > page), then deactivate_slab() is not required . My justification is > the debug code will take the same code path as the non-debug, hence > the page will experience the same transition on different list like > the non-debug code, and help to detect the bug, also it will improve > scalability on SMP. > Besides this, I found the debug code is not scalable well, is it worth > to work on it? The debug code is kept out of the hot path intentionally because it does not scale well and reduces performance. Its compiled in in case we have to track down a nasty memory corruption bug on a prod kernel that cannot be easily rebuilt.
diff --git a/mm/slub.c b/mm/slub.c index a68c2ae..e152634 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2551,23 +2551,21 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node, } freelist = new_slab_objects(s, gfpflags, node, &c); - if (unlikely(!freelist)) { slab_out_of_memory(s, gfpflags, node); return NULL; } + VM_BUG_ON(!pfmemalloc_match(page, gfpflags)); page = c->page; - if (likely(!kmem_cache_debug(s) && pfmemalloc_match(page, gfpflags))) + if (likely(!kmem_cache_debug(s)) goto load_freelist; /* Only entered in the debug case */ - if (kmem_cache_debug(s) && - !alloc_debug_processing(s, page, freelist, addr)) + if (!alloc_debug_processing(s, page, freelist, addr)) goto new_slab; /* Slab failed checks. Next slab needed */ - - deactivate_slab(s, page, get_freepointer(s, freelist), c); - return freelist; + else + goto load_freelist; } /*
new_slab_objects() always return c->page matching the required gfpflags, but the current code is misleading and ___slab_alloc->deactivate_slab seems to allow not-pfmemalloc purpose obj to be allocated from pfmemalloc-purpose page. This patch re-organize the code to eliminate the confusion. Signed-off-by: Pingfan Liu <kernelfans@gmail.com> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Andrew Morton <akpm@linux-foundation.org> --- mm/slub.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)