On Mon 22-07-24 20:09:37, Barry Song wrote: > On Mon, Jul 22, 2024 at 7:26 PM Michal Hocko <mhocko@xxxxxxxx> wrote: > > > > On Sun 21-07-24 10:14:03, Barry Song wrote: > > > On Fri, Jul 19, 2024 at 7:53 PM Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > > > > > > > > On Fri, Jul 19, 2024 at 07:43:38PM +1200, Barry Song wrote: > > > > > I doubt this is going to work as users can use a variant to save gfp_flags. > > > > > On the other hand, isn't it necessarily a bug of vdpa, why can't it be mm? > > > > > > > > > > if mm disallows GFP_NOFAIL, there must be a doc to say that; if it allows, > > > > > we should never return NULL. > > > > > > > > Yeah. Maybe the right answer is to have separate _nofail variants that > > > > don't take any flags and make GFP_NOFAIL an entirely mm-private internal > > > > flags that is rejected by all external interfaces. That should also > > > > really help with auditing the users. > > > > This would require duplicating many of our allocations APIs. > > > > > Just like Michal has consistently asserted that using GFP_NOFAIL with > > > non-wait is against the rules, I think we should enforce this policy by: > > > > > > diff --git a/include/linux/gfp_types.h b/include/linux/gfp_types.h > > > index 313be4ad79fd..a5c09f9590f2 100644 > > > --- a/include/linux/gfp_types.h > > > +++ b/include/linux/gfp_types.h > > > @@ -258,7 +258,7 @@ enum { > > > #define __GFP_KSWAPD_RECLAIM ((__force gfp_t)___GFP_KSWAPD_RECLAIM) > > > /* kswapd can wake */ > > > #define __GFP_RECLAIM ((__force > > > gfp_t)(___GFP_DIRECT_RECLAIM|___GFP_KSWAPD_RECLAIM)) > > > #define __GFP_RETRY_MAYFAIL ((__force gfp_t)___GFP_RETRY_MAYFAIL) > > > -#define __GFP_NOFAIL ((__force gfp_t)___GFP_NOFAIL) > > > +#define __GFP_NOFAIL ((__force gfp_t)(___GFP_NOFAIL | ___GFP_DIRECT_RECLAIM)) > > > #define __GFP_NORETRY ((__force gfp_t)___GFP_NORETRY) > > > > > > Anyone misusing GFP_ATOMIC | __GFP_NOFAIL in an atomic context > > > risks experiencing a crash due to sleep in atomic. This is a common > > > consequence, as all instances of sleep in atomic should result in the > > > same issue. > > > > I really dislike any of __GFP_$FOO to have side effects like this. > > Please let's not overdo this. > > Okay, but my point is that if GFP_NOFAIL is inevitably blockable, why > not enforce this and let users understand that it is definitively > blockable? ust like when we call alloc_pages(GFP_KERNEL), we know > it might sleep. __GFP_$FOO are usually low level. GFP_$FOO are high level and they combine several subflags to have a specific meaning. So this would need to be GFP_NOFAIL. Btw. the same applies to __GFP_NORETRY and __GFP_RETRY_MAYFAIL. Whether changing existing users of those flags is worth is a different question. -- Michal Hocko SUSE Labs