Re: [PATCH] mm,page_owner: don't remove GFP flags in add_stack_record_to_list

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/30/24 1:49 AM, Dave Chinner wrote:
> On Mon, Apr 29, 2024 at 09:59:43AM +0200, Vlastimil Babka wrote:
>> On 4/29/24 7:47 AM, Christoph Hellwig wrote:
>> > This loses flags like GFP_NOFS and GFP_NOIO that are important to avoid
>> > deadlocks as well as GFP_NOLOCKDEP that otherwise generates lockdep false
>> > positives.
>> 
>> GFP_NOFS and GFP_NOIO translate to GFP_KERNEL without __GFP_FS/__GFP_IO so I
>> don't see how this patch would have helped with those.
>> __GFP_NOLOCKDEP is likely the actual issue and stackdepot solved it like this:
>> 
>> https://lore.kernel.org/linux-xfs/20240418141133.22950-1-ryabinin.a.a@xxxxxxxxx/
>>
>> So we could just do the same here.
> 
> Yes, it is __GFP_NOLOCKDEP that is the issue here, but
> cargo-cult-copying of that stackdepot fix is just whack-a-mole bug
> fixing without addressing the technical debt that got us here in the
> first place. Has anyone else bothered to look to see if kmemleak has
> the same problem?

Looks like you did :)

> If anyone bothered to do an audit, they would see that
> gfp_kmemleak_mask() handles the reclaim context masks correctly.
> Further, it adds NOWARN, NOMEMALLOC and
> NORETRY, which means the debug code is silent when it fails, it
> doesn't deplete emergency reserves and doesn't bog down retrying
> forever when there are sustained low memory situations.

So we do have NOWARN here. __GFP_RETRY_MAYFAIL might have been slightly
better than __GFP_NOWARN wrt "not retrying forever" but also not giving up
too soon. If we want to be really careful about reserves, it's a question
whether to keep the | GFP_ATOMIC which translates to leaving __GFP_HIGH.
OTOH if we don't keep it, these allocations might fail too easily from an
atomic context and we could miss the debugging data.

> This also points out that the page-owner/stackdepot code that strips
> GFP_ZONEMASK is completely redundant. Doing:
> 
> 	gfp_flags &= GFP_KERNEL|GFP_ATOMIC|__GFP_NOLOCKDEP;
> 
> strips everything but __GFP_RECLAIM, __GFP_FS, __GFP_IO,
> __GFP_HIGH and __GFP_NOLOCKDEP. This already strips the zonemask
> info, so there's no need to do it explicitly.

True.

> IOWs, the right way to fix this set of problems is to lift
> gfp_kmemleak_mask() to include/linux/gfp.h and then use it across
> all these nested allocations that occur behind the public
> memory allocation API.

Agree. But arguably these quick fixes adding __GFP_NOLOCKDEP were
appropriate for the late rc phase we're in.

> I've got a patchset under test at the moment that does this....

Great! Thanks.

> -Dave.





[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux