* slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away.
@ 2022-03-22 17:39 Sebastian Andrzej Siewior
2022-03-23 10:09 ` Vlastimil Babka
0 siblings, 1 reply; 4+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-03-22 17:39 UTC (permalink / raw)
To: linux-mm; +Cc: Vlastimil Babka, Michal Hocko, Thomas Gleixner
Run into
| ======================================================
| WARNING: possible circular locking dependency detected
| 5.17.0-next-20220322 #19 Not tainted
| ------------------------------------------------------
| kswapd1/513 is trying to acquire lock:
| ffff888555b7e628 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_icwalk_ag+0x36c/0x810
|
| but task is already holding lock:
| ffffffff82a2fb20 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat+0x600/0x740
|
| which lock already depends on the new lock.
|
|
| the existing dependency chain (in reverse order) is:
|
| -> #1 (fs_reclaim){+.+.}-{0:0}:
| fs_reclaim_acquire+0xaa/0xe0
| __kmalloc_node+0x65/0x3e0
| xfs_attr_copy_value+0x70/0xa0
…
and I think this is similar to commit
704687deaae76 ("mm: make slab and vmalloc allocators __GFP_NOLOCKDEP aware")
and maybe something like this:
diff --git a/mm/slab.h b/mm/slab.h
--- a/mm/slab.h
+++ b/mm/slab.h
@@ -717,7 +717,7 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
struct obj_cgroup **objcgp,
size_t size, gfp_t flags)
{
- flags &= gfp_allowed_mask;
+ flags &= gfp_allowed_mask | __GFP_NOLOCKDEP;
might_alloc(flags);
?
Sebastian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away.
2022-03-22 17:39 slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away Sebastian Andrzej Siewior
@ 2022-03-23 10:09 ` Vlastimil Babka
2022-03-23 11:30 ` Sebastian Andrzej Siewior
0 siblings, 1 reply; 4+ messages in thread
From: Vlastimil Babka @ 2022-03-23 10:09 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, linux-mm; +Cc: Michal Hocko, Thomas Gleixner
On 3/22/22 18:39, Sebastian Andrzej Siewior wrote:
> Run into
> | ======================================================
> | WARNING: possible circular locking dependency detected
> | 5.17.0-next-20220322 #19 Not tainted
> | ------------------------------------------------------
> | kswapd1/513 is trying to acquire lock:
> | ffff888555b7e628 (&xfs_dir_ilock_class){++++}-{3:3}, at: xfs_icwalk_ag+0x36c/0x810
> |
> | but task is already holding lock:
> | ffffffff82a2fb20 (fs_reclaim){+.+.}-{0:0}, at: balance_pgdat+0x600/0x740
> |
> | which lock already depends on the new lock.
> |
> |
> | the existing dependency chain (in reverse order) is:
> |
> | -> #1 (fs_reclaim){+.+.}-{0:0}:
> | fs_reclaim_acquire+0xaa/0xe0
> | __kmalloc_node+0x65/0x3e0
> | xfs_attr_copy_value+0x70/0xa0
> …
>
> and I think this is similar to commit
> 704687deaae76 ("mm: make slab and vmalloc allocators __GFP_NOLOCKDEP aware")
>
> and maybe something like this:
>
> diff --git a/mm/slab.h b/mm/slab.h
> --- a/mm/slab.h
> +++ b/mm/slab.h
> @@ -717,7 +717,7 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
> struct obj_cgroup **objcgp,
> size_t size, gfp_t flags)
> {
> - flags &= gfp_allowed_mask;
> + flags &= gfp_allowed_mask | __GFP_NOLOCKDEP;
Hmm but gfp_allowed_mask already should contain __GFP_NOLOCKDEP after
kernel_init_freeable() is reached. I doubt we can reach fs or reclaim
code before that?
>
> might_alloc(flags);
>
> ?
>
> Sebastian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away.
2022-03-23 10:09 ` Vlastimil Babka
@ 2022-03-23 11:30 ` Sebastian Andrzej Siewior
2022-03-23 11:45 ` Vlastimil Babka
0 siblings, 1 reply; 4+ messages in thread
From: Sebastian Andrzej Siewior @ 2022-03-23 11:30 UTC (permalink / raw)
To: Vlastimil Babka; +Cc: linux-mm, Michal Hocko, Thomas Gleixner
On 2022-03-23 11:09:35 [+0100], Vlastimil Babka wrote:
> > --- a/mm/slab.h
> > +++ b/mm/slab.h
> > @@ -717,7 +717,7 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
> > struct obj_cgroup **objcgp,
> > size_t size, gfp_t flags)
> > {
> > - flags &= gfp_allowed_mask;
> > + flags &= gfp_allowed_mask | __GFP_NOLOCKDEP;
>
> Hmm but gfp_allowed_mask already should contain __GFP_NOLOCKDEP after
> kernel_init_freeable() is reached. I doubt we can reach fs or reclaim
> code before that?
That is way past init. I have
gfp_allowed_mask = 0x1ffffff
__GFP_NOLOCKDEP = 0x8000000
Looking at the origin of gfp_allowed_mask
| /* Room for N __GFP_FOO bits */
| #define __GFP_BITS_SHIFT (24 + \
| 3 * IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \
| IS_ENABLED(CONFIG_LOCKDEP))
| #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
which in my case 24 + 1 but it would need to be something like
27 * IS_ENABLED(CONFIG_LOCKDEP)
or a better way to come up with __GFP_BITS_MASK.
Sebastian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away.
2022-03-23 11:30 ` Sebastian Andrzej Siewior
@ 2022-03-23 11:45 ` Vlastimil Babka
0 siblings, 0 replies; 4+ messages in thread
From: Vlastimil Babka @ 2022-03-23 11:45 UTC (permalink / raw)
To: Sebastian Andrzej Siewior, Andrey Konovalov, Andrew Morton, Marco Elver
Cc: linux-mm, Michal Hocko, Thomas Gleixner
On 3/23/22 12:30, Sebastian Andrzej Siewior wrote:
> On 2022-03-23 11:09:35 [+0100], Vlastimil Babka wrote:
>>> --- a/mm/slab.h
>>> +++ b/mm/slab.h
>>> @@ -717,7 +717,7 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s,
>>> struct obj_cgroup **objcgp,
>>> size_t size, gfp_t flags)
>>> {
>>> - flags &= gfp_allowed_mask;
>>> + flags &= gfp_allowed_mask | __GFP_NOLOCKDEP;
>>
>> Hmm but gfp_allowed_mask already should contain __GFP_NOLOCKDEP after
>> kernel_init_freeable() is reached. I doubt we can reach fs or reclaim
>> code before that?
>
> That is way past init. I have
> gfp_allowed_mask = 0x1ffffff
> __GFP_NOLOCKDEP = 0x8000000
>
> Looking at the origin of gfp_allowed_mask
>
> | /* Room for N __GFP_FOO bits */
> | #define __GFP_BITS_SHIFT (24 + \
> | 3 * IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \
> | IS_ENABLED(CONFIG_LOCKDEP))
> | #define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
Mainline has this:
#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP))
which looks OK.
The '24' comes from "kasan, mm: only define ___GFP_SKIP_KASAN_POISON
with HW_TAGS" in mmotm.
It makes ___GFP_SKIP_KASAN_POISON optional, that's why the decrease to
24. However this is not about the number of flags, but the highest bit,
which can be that of __GFP_NOLOCKDEP and it's then not covered by
__GFP_BITS_SHIFT if lockdep is enabled and kasan not...
> which in my case 24 + 1 but it would need to be something like
> 27 * IS_ENABLED(CONFIG_LOCKDEP)
>
> or a better way to come up with __GFP_BITS_MASK.
>
> Sebastian
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-03-23 11:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-22 17:39 slab_pre_alloc_hook() strips __GFP_NOLOCKDEP away Sebastian Andrzej Siewior
2022-03-23 10:09 ` Vlastimil Babka
2022-03-23 11:30 ` Sebastian Andrzej Siewior
2022-03-23 11:45 ` Vlastimil Babka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.