On 3/23/22 14:36, Andrey Konovalov wrote: > On Wed, Mar 23, 2022 at 2:02 PM Sebastian Andrzej Siewior > <bigeasy@xxxxxxxxxxxxx> wrote: >> >> On 2022-03-23 12:48:29 [+0100], Vlastimil Babka wrote: >>>> +#ifdef CONFIG_KASAN_HW_TAGS >>>> #define ___GFP_SKIP_KASAN_POISON 0x1000000u >>>> +#else >>>> +#define ___GFP_SKIP_KASAN_POISON 0 >>>> +#endif >>>> #ifdef CONFIG_LOCKDEP >>>> #define ___GFP_NOLOCKDEP 0x2000000u >>>> #else >>>> @@ -251,7 +255,9 @@ struct vm_area_struct; >>>> #define __GFP_NOLOCKDEP ((__force gfp_t)___GFP_NOLOCKDEP) >>>> >>>> /* Room for N __GFP_FOO bits */ >>>> -#define __GFP_BITS_SHIFT (25 + IS_ENABLED(CONFIG_LOCKDEP)) >>>> +#define __GFP_BITS_SHIFT (24 + \ >>>> + IS_ENABLED(CONFIG_KASAN_HW_TAGS) + \ >>>> + IS_ENABLED(CONFIG_LOCKDEP)) >>> >>> This breaks __GFP_NOLOCKDEP, see: >>> https://lore.kernel.org/all/YjoJ4CzB3yfWSV1F@xxxxxxxxxxxxx/ >> >> This could work because ___GFP_NOLOCKDEP is still 0x2000000u. In >> ("kasan, page_alloc: allow skipping memory init for HW_TAGS") >> https://lore.kernel.org/all/0d53efeff345de7d708e0baa0d8829167772521e.1643047180.git.andreyknvl@xxxxxxxxxx/ >> >> This is replaced with 0x8000000u which breaks lockdep. >> >> Sebastian > > Hi Sebastian, > > Indeed, sorry for breaking lockdep. Thank you for the report! > > I wonder what's the proper fix for this. Perhaps, don't hide KASAN GFP > bits under CONFIG_KASAN_HW_TAGS? And then do: > > #define __GFP_BITS_SHIFT (27 + IS_ENABLED(CONFIG_LOCKDEP)) > > Vlastimil, Andrew do you have any preference? I guess it's the simplest thing to do for now. For the future we can still improve and handle all combinations of kasan/lockdep to occupy as few bits as possible and set the shift/mask appropriately. Or consider first if it's necessary anyway. I don't know if we really expect at any point to start triggering the BUILD_BUG_ON() in radix_tree_init() and then only some combination of configs will reduce the flags to a number that works. Or is there anything else that depends on __GFP_BITS_SHIFT? I mean if we don't expect to go this way, we can just define __GFP_BITS_SHIFT as a constant that assumes all the config-dependent flags to be defined (not zero). > If my suggestion sounds good, Andrew, could you directly apply the > changes? They are needed for these 3 patches: > > kasan, page_alloc: allow skipping memory init for HW_TAGS > kasan, page_alloc: allow skipping unpoisoning for HW_TAGS > kasan, mm: only define ___GFP_SKIP_KASAN_POISON with HW_TAGS > > As these depend on each other, I can't send separate patches that can > be folded for all 3. > > Thanks!
