Re: [PATCH] gfp: GFP_RECLAIM_MASK should include __GFP_NO_KSWAPD

Michal Hocko
2015-10-14T07:41:43Z

On Wed 14-10-15 13:58:05, Pan Xinhui wrote:
> Hi, all
> I am working on some debug features' development.
> I use kmalloc in some places of *scheduler*.

This sounds inherently dangerous.

> And the gfp_flag is GFP_ATOMIC, code looks like
> p = kmalloc(sizeof(*p), GFP_ATOMIC);
>
> however I notice GFP_ATOMIC is still not enough. because when system
> is at low memory state, slub might try to wakeup kswapd. then some
> weird issues hit.

gfp flags have been reworked in the current mmotm tree so you want
__GFP_ATOMIC here. This will be non sleeping allocation which won't even
wake up kswapd. I guess you do not want/need to touch memory reserves
for something like a debugging feature (so you do not have to abuse
__GFP_HIGH)

[...]

> After some simple check, I change my codes. this time code looks like:
> p = kmalloc(sizeof(*p), GFP_ATOMIC | __GFP_NO_KSWAPD);
> I think this flag will forbid slub to call any scheduler codes. But issue still hit. :(
>
> my test result shows that __GFP_NO_KSWAPD is cleared when slub pass gfp_flag to page allocator!!!
>
> at last I found it is clear by codes below.
> 1441 static struct page *new_slab(struct kmem_cache *s, gfp_t flags, int node)
> 1442 {
> 1443 if (unlikely(flags & GFP_SLAB_BUG_MASK)) {
> 1444 pr_emerg("gfp: %u\n", flags & GFP_SLAB_BUG_MASK);
> 1445 BUG();
> 1446 }
> 1447
> 1448 return allocate_slab(s,
> 1449 flags & (GFP_RECLAIM_MASK | GFP_CONSTRAINT_MASK), node);//all other flags will be cleared. my god!!!
> 1450 }
>
> I think GFP_RECLAIM_MASK should include as many available flags as possible. :)

Not really. It should only contain those which are really reclaim
related. The fact that SLUB drops other flags is an internal detail
of the allocator. If the resulting memory doesn't match the original
requirements (e.g. zone placing etc...) then it is certainly a bug
but not a bug in GFP_RECLAIM_MASK.

Anyway you are right that GFP_RECLAIM_MASK should contain
__GFP_NO_KSWAPD resp. its new representation which is the case in the
current mmotm tree as pointed out in previous response.
--
Michal Hocko
SUSE Labs