| Message ID | ca8e3803-4757-358e-dcf2-4824213a9d2c@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | lib/stackdepot: stackdepot: don't use __GFP_KSWAPD_RECLAIM from __stack_depot_save() if atomic context | expand |
On 2023/05/20 20:33, Tetsuo Handa wrote: > @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries, > * contexts and I/O. > */ > alloc_flags &= ~GFP_ZONEMASK; > - alloc_flags &= (GFP_ATOMIC | GFP_KERNEL); > + if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) > + alloc_flags &= __GFP_HIGH; > + else > + alloc_flags &= GFP_KERNEL; > alloc_flags |= __GFP_NOWARN; Well, comparing with a report which reached __stack_depot_save() via fill_pool() ( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that above lines might be bogus. Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context. Then, these lines could be simplified like below. if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) alloc_flags = __GFP_HIGH | __GFP_NOWARN; else alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN; How is the importance of memory allocation in __stack_depot_save() ? If allocation failure is welcome, maybe we should not trigger OOM killer by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ... > page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER); > if (page)
On 2023/05/20 22:14, Tetsuo Handa wrote: > On 2023/05/20 20:33, Tetsuo Handa wrote: >> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries, >> * contexts and I/O. >> */ >> alloc_flags &= ~GFP_ZONEMASK; >> - alloc_flags &= (GFP_ATOMIC | GFP_KERNEL); >> + if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) >> + alloc_flags &= __GFP_HIGH; >> + else >> + alloc_flags &= GFP_KERNEL; >> alloc_flags |= __GFP_NOWARN; > > Well, comparing with a report which reached __stack_depot_save() via fill_pool() > ( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that > above lines might be bogus. > > Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because > fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context. > Then, these lines could be simplified like below. > > if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) > alloc_flags = __GFP_HIGH | __GFP_NOWARN; > else > alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN; > > How is the importance of memory allocation in __stack_depot_save() ? > If allocation failure is welcome, maybe we should not trigger OOM killer > by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ... > >> page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER); >> if (page) > Well, since stackdepot itself simply use GFP flags supplied by kasan, this should be considered as a kasan's problem? __kasan_record_aux_stack() { alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc); // May deadlock due to including __GFP_KSWAPD_RECLAIM bit. } Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. Where do we want to drop this bit (in the caller side, or in the callee side)?
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2023/05/20 22:14, Tetsuo Handa wrote: >> On 2023/05/20 20:33, Tetsuo Handa wrote: >>> @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries, >>> * contexts and I/O. >>> */ >>> alloc_flags &= ~GFP_ZONEMASK; >>> - alloc_flags &= (GFP_ATOMIC | GFP_KERNEL); >>> + if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) >>> + alloc_flags &= __GFP_HIGH; >>> + else >>> + alloc_flags &= GFP_KERNEL; >>> alloc_flags |= __GFP_NOWARN; >> >> Well, comparing with a report which reached __stack_depot_save() via fill_pool() >> ( https://syzkaller.appspot.com/bug?extid=358bb3e221c762a1adbb ), I feel that >> above lines might be bogus. >> >> Maybe we want to enable __GFP_HIGH even if alloc_flags == GFP_NOWAIT because >> fill_pool() uses __GFPHIGH | __GFP_NOWARN regardless of the caller's context. >> Then, these lines could be simplified like below. >> >> if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) >> alloc_flags = __GFP_HIGH | __GFP_NOWARN; >> else >> alloc_flags = (alloc_flags & GFP_KERNEL) | __GFP_NOWARN; >> >> How is the importance of memory allocation in __stack_depot_save() ? >> If allocation failure is welcome, maybe we should not trigger OOM killer >> by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ... >> >>> page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER); >>> if (page) >> > > Well, since stackdepot itself simply use GFP flags supplied by kasan, > this should be considered as a kasan's problem? > > __kasan_record_aux_stack() { > alloc_meta->aux_stack[0] = kasan_save_stack(GFP_NOWAIT, can_alloc); // May deadlock due to including __GFP_KSWAPD_RECLAIM bit. > } > > Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. > Where do we want to drop this bit (in the caller side, or in the callee side)? Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX (instead of GFP_ATOMIC) for debug code? The debug code may be called at almost arbitrary places, and wakeup_kswap() isn't safe to be called in some situations. BTW: I still think that it's better to show the circular lock order in the patch description. I know the information is in syzkaller report. It will make reader's life easier if the patch description is self-contained. Best Regards, Huang, Ying
On 2023/05/22 11:13, Huang, Ying wrote: >> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. >> Where do we want to drop this bit (in the caller side, or in the callee side)? > > Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX > (instead of GFP_ATOMIC) for debug code? The debug code may be called at > almost arbitrary places, and wakeup_kswap() isn't safe to be called in > some situations. What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT? Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks. Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and reduces latency of atomic allocations (which is important when called from "atomic" context). I consider that memory allocations which do not do direct reclaim should be geared towards less locking dependency. In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages. It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd upon GFP_ATOMIC or GFP_NOWAIT allocations. If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or GFP_NOWAIT is not recommended from the beginning...
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2023/05/22 11:13, Huang, Ying wrote: >>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. >>> Where do we want to drop this bit (in the caller side, or in the callee side)? >> >> Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX >> (instead of GFP_ATOMIC) for debug code? The debug code may be called at >> almost arbitrary places, and wakeup_kswap() isn't safe to be called in >> some situations. > > What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT? > Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe > enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks. > Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and > reduces latency of atomic allocations (which is important when called from "atomic" context). > I consider that memory allocations which do not do direct reclaim should be geared towards > less locking dependency. Except debug code, where do you find locking issues for waking up kswapd? > In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages. > It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM > right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd > upon GFP_ATOMIC or GFP_NOWAIT allocations. > > If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add > __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or > GFP_NOWAIT is not recommended from the beginning... From performance perspective, it's better to wake up kswapd as early as possible. Because it can reduce the possibility of the direct reclaiming, which may case very long latency. Best Regards, Huang, Ying
On 2023/05/22 12:07, Huang, Ying wrote: > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > >> On 2023/05/22 11:13, Huang, Ying wrote: >>>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. >>>> Where do we want to drop this bit (in the caller side, or in the callee side)? >>> >>> Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX >>> (instead of GFP_ATOMIC) for debug code? The debug code may be called at >>> almost arbitrary places, and wakeup_kswap() isn't safe to be called in >>> some situations. >> >> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT? >> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe >> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks. >> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and >> reduces latency of atomic allocations (which is important when called from "atomic" context). >> I consider that memory allocations which do not do direct reclaim should be geared towards >> less locking dependency. > > Except debug code, where do you find locking issues for waking up kswapd? I'm not aware of lockdep reports except debug code. But due to too many locking dependency, lockdep gives up tracking all dependency (e.g. https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2 https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7 https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9 ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock and zonelist_update_seq are candidates which need not to be held from interrupt context. > >> In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages. >> It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM >> right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd >> upon GFP_ATOMIC or GFP_NOWAIT allocations. >> >> If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add >> __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or >> GFP_NOWAIT is not recommended from the beginning... > >>From performance perspective, it's better to wake up kswapd as early as > possible. Because it can reduce the possibility of the direct > reclaiming, which may case very long latency. My expectation is that a __GFP_DIRECT_RECLAIM allocation request which happened after a !__GFP_KSWAPD_RECLAIM allocation request wakes kswapd before future __GFP_DIRECT_RECLAIM allocation requests have to perform the direct reclaiming. > > Best Regards, > Huang, Ying >
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2023/05/22 12:07, Huang, Ying wrote: >> Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: >> >>> On 2023/05/22 11:13, Huang, Ying wrote: >>>>> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. >>>>> Where do we want to drop this bit (in the caller side, or in the callee side)? >>>> >>>> Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX >>>> (instead of GFP_ATOMIC) for debug code? The debug code may be called at >>>> almost arbitrary places, and wakeup_kswap() isn't safe to be called in >>>> some situations. >>> >>> What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT? >>> Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe >>> enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks. >>> Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and >>> reduces latency of atomic allocations (which is important when called from "atomic" context). >>> I consider that memory allocations which do not do direct reclaim should be geared towards >>> less locking dependency. >> >> Except debug code, where do you find locking issues for waking up kswapd? > > I'm not aware of lockdep reports except debug code. > > But due to too many locking dependency, lockdep gives up tracking all dependency (e.g. > > https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2 > https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f > https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7 > https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9 > > ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock > and zonelist_update_seq are candidates which need not to be held from interrupt context. Why is it not safe to wake up kswapd/kcompactd from interrupt context? Best Regards, Huang, Ying
On 2023/05/23 9:07, Huang, Ying wrote: >>> Except debug code, where do you find locking issues for waking up kswapd? >> >> I'm not aware of lockdep reports except debug code. >> >> But due to too many locking dependency, lockdep gives up tracking all dependency (e.g. >> >> https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2 >> https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f >> https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7 >> https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9 >> >> ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock >> and zonelist_update_seq are candidates which need not to be held from interrupt context. > > Why is it not safe to wake up kswapd/kcompactd from interrupt context? I'm not saying it is not safe to wake up kswapd/kcompactd from interrupt context. Please notice that I'm using "need not" than "must not". Since total amount of RAM a Linux kernel can use had been increased over years, watermark gap between "kswapd should start background reclaim" and "current thread must start foreground reclaim" also increased. Then, randomly allocating small amount of pages from interrupt context (or atomic context) without waking up will not needlessly increase possibility of reaching "current thread must start foreground reclaim" watermark. Then, reducing locking dependency by not waking up becomes a gain. KASAN developers, I'm waiting for your response on How is the importance of memory allocation in __stack_depot_save() ? If allocation failure is welcome, maybe we should not trigger OOM killer by clearing __GFP_NORETRY when alloc_flags contained __GFP_FS ... part.
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> writes: > On 2023/05/23 9:07, Huang, Ying wrote: >>>> Except debug code, where do you find locking issues for waking up kswapd? >>> >>> I'm not aware of lockdep reports except debug code. >>> >>> But due to too many locking dependency, lockdep gives up tracking all dependency (e.g. >>> >>> https://syzkaller.appspot.com/bug?extid=8a249628ae32ea7de3a2 >>> https://syzkaller.appspot.com/bug?extid=a70a6358abd2c3f9550f >>> https://syzkaller.appspot.com/bug?extid=9bbbacfbf1e04d5221f7 >>> https://syzkaller.appspot.com/bug?extid=b04c9ffbbd2f303d00d9 >>> >>> ). I want to reduce locking patterns where possible. pgdat->{kswapd,kcompactd}_wait.lock >>> and zonelist_update_seq are candidates which need not to be held from interrupt context. >> >> Why is it not safe to wake up kswapd/kcompactd from interrupt context? > > I'm not saying it is not safe to wake up kswapd/kcompactd from interrupt context. > Please notice that I'm using "need not" than "must not". Got it. > Since total amount of RAM a Linux kernel can use had been increased over years, > watermark gap between "kswapd should start background reclaim" and "current thread > must start foreground reclaim" also increased. Then, randomly allocating small > amount of pages from interrupt context (or atomic context) without waking up > will not needlessly increase possibility of reaching "current thread must start > foreground reclaim" watermark. Then, reducing locking dependency by not waking up > becomes a gain. Personally, I prefer to wake up kswapd ASAP. And fix the deadlock if possible. Best Regards, Huang, Ying
On Mon 22-05-23 11:47:25, Tetsuo Handa wrote: > On 2023/05/22 11:13, Huang, Ying wrote: > >> Any atomic allocation used by KASAN needs to drop __GFP_KSWAPD_RECLAIM bit. > >> Where do we want to drop this bit (in the caller side, or in the callee side)? > > > > Yes. I think we should fix the KASAN. Maybe define a new GFP_XXX > > (instead of GFP_ATOMIC) for debug code? The debug code may be called at > > almost arbitrary places, and wakeup_kswap() isn't safe to be called in > > some situations. > > What do you think about removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT? Not a good idea IMO. It is really hard to achieve real locklessness in the page allocator. If we ever need something like that it should be pretty obviously requested by a dedicated gfp flag rather than overriding a long term established semantic. While GFP_ATOMIC is a bit of a misnomer it has many users who really only require non-sleeping behavior. > Recent reports indicate that atomic allocations (GFP_ATOMIC and GFP_NOWAIT) are not safe > enough to think "atomic". They just don't do direct reclaim, but they do take spinlocks. > Removing __GFP_KSWAPD_RECLAIM from GFP_ATOMIC and GFP_NOWAIT simplifies locking dependency and > reduces latency of atomic allocations (which is important when called from "atomic" context). I would really like to see any numbers to believe this is the case actually. Waking up kswapd should be pretty non-visible. > I consider that memory allocations which do not do direct reclaim should be geared towards > less locking dependency. > > In general, GFP_ATOMIC or GFP_NOWAIT users will not allocate many pages. This hugely depend on the workload. I do not think we can make any generic statements like that. > It is likely that somebody else tries to allocate memory using __GFP_DIRECT_RECLAIM > right after GFP_ATOMIC or GFP_NOWAIT allocations. We unlikely need to wake kswapd > upon GFP_ATOMIC or GFP_NOWAIT allocations. The thing is that you do not know this is a the case. You might have a IRQ heavy prossing making a lot of memory allocations (e.g. networking) while the rest of the processing doesn't require any additional memory. > If some GFP_ATOMIC or GFP_NOWAIT users need to allocate many pages, they can add > __GFP_KSWAPD_RECLAIM explicitly; though allocating many pages using GFP_ATOMIC or > GFP_NOWAIT is not recommended from the beginning... As much as I do not really like the long term GFP_ATOMIC semantic I do not think we should be changing it to what you are proposing for reasons mentioned above. GFP_NOWAIT change is even more questionable. Many users simply use GFP_NOWAIT as a way of an optimistic allocation with a more expensinsive fallback. We do not want to allow those consumers to consume watermark gap memory to force others to hit the direct reclaim wall. Really there is very likely only a handfull of users who cannot even wake kswapd or perform other non-sleeping locking and those should currently drop __GFP_KSWAPD_RECLAIM. Maybe we should consider an alias for them to not bother with the low level flag. Maybe we will need GFP_LOCKLESS or something similar.
diff --git a/lib/stackdepot.c b/lib/stackdepot.c index 2f5aa851834e..5c331a80b87a 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -405,7 +405,10 @@ depot_stack_handle_t __stack_depot_save(unsigned long *entries, * contexts and I/O. */ alloc_flags &= ~GFP_ZONEMASK; - alloc_flags &= (GFP_ATOMIC | GFP_KERNEL); + if (!(alloc_flags & __GFP_DIRECT_RECLAIM)) + alloc_flags &= __GFP_HIGH; + else + alloc_flags &= GFP_KERNEL; alloc_flags |= __GFP_NOWARN; page = alloc_pages(alloc_flags, DEPOT_POOL_ORDER); if (page)
syzbot is reporting lockdep warning in __stack_depot_save(), for wakeup_kswapd() from wake_all_kswapds() from __alloc_pages_slowpath() calls wakeup_kcompactd() when __GFP_KSWAPD_RECLAIM is set and __GFP_DIRECT_RECLAIM is not set (i.e. GFP_ATOMIC). Since __stack_depot_save() might be called with arbitrary locks held, __stack_depot_save() should not wake kswapd which in turn wakes kcompactd. Reported-by: syzbot <syzbot+ece2915262061d6e0ac1@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=ece2915262061d6e0ac1 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: cd11016e5f52 ("mm, kasan: stackdepot implementation. Enable stackdepot for SLAB") --- lib/stackdepot.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)