On Tue, 23 Nov 2021, Michal Hocko wrote: > On Sat 20-11-21 21:51:20, Neil Brown wrote: > > On Sat, 20 Nov 2021, Matthew Wilcox wrote: > > > On Fri, Nov 19, 2021 at 10:14:38AM +1100, NeilBrown wrote: > > > > On Thu, 18 Nov 2021, Matthew Wilcox wrote: > > > > > Surely this should be gfpflags_allow_blocking() instead of poking about > > > > > in the innards of gfp flags? > > > > > > > > Possibly. Didn't know about gfpflags_allow_blocking(). From a quick > > > > grep in the kernel, a whole lot of other people don't know about it > > > > either, though clearly some do. > > > > > > > > Maybe we should reaname "__GFP_DIRECT_RECLAIM" to > > > > "__GFP_ALLOW_BLOCKING", because that is what most users seems to care > > > > about. > > > > > > I tend towards the school of thought that the __GFP flags should make > > > sense to the implementation and users should use either GFP_ or functions. > > > When we see users adding or subtracting __GFP flags, that's a problem. > > > > Except __GFP_NOWARN of course, or __GFP_ZERO, or __GFP_NOFAIL. > > What about __GFP_HIGHMEM? __GFP_DMA? __GFP_HIGH? > > > > They all seem to be quite meaningful to the caller - explicitly > > specifying properties of the memory or properties of the service. > > (But maybe you would prefer __GFP_HIGH be spelled "__GFP_LOW_WATERMARK" > > so it would make more sense to the implementation). > > > > __GFP_DIRECTRECLAIM seems to me to be more the exception than the rule - > > specifying internal implementation details. > > I do not think it is viable to fix up gfp flags to be consistent :/ You may be right :-) Of course if we don't try - you'll definitely be right. > Both __GFP_DIRECT_RECLAIM and __GFP_KSWAPD_RECLAIM are way too lowlevel > but historically we've had requests to inhibit kswapd for a particular > requests because that has led to problems - fun reading caf491916b1c1. Unfortunately that commit doesn't provide any reasoning, just an assertion. The best reasoning I could find was in caf491916b1c1 which was the initial revert. There the primary reasoning was "there is a bug that we don't have time for a proper fix before the next release, so let's just use this quick fix". ... and maybe "the quick fix" was "the right fix", but I cannot tell from the commit logs :-( > __GFP_ALLOW_BLOCKING would make a lot of sense but I am not sure it > would be a good match to __GFP_KSWAPD_RECLAIM. So? __GFP_ALLOW_BLOCKING makes it clear what is, or is not, acceptable to the caller. How much reclaim, or other activity, alloc_page() engages in is largely irrelevant to the caller as lock as it doesn't block if asked not to (and doesn't enter an FS if asked not to, etc). > > > Actually ... I take it back about __GFP_NOWARN. That probably shouldn't > > exist at all. Warnings should be based on how stressed the mm system is, > > not on whether the caller wants thinks failure is manageable. > > Unless we change the way when allocation warnings are triggered then we > really need this. There are many opportunistic allocations with a > fallback behavior which do not want to swamp kernel logs with failures > that are of no use. Think of a THP allocation that really want to be > just very quick and falls back to normal base pages otherwise. Deducing > context which is just fine to not report failures is quite tricky and it > can get wrong easily. Callers should know whether warning can be of any > use in many cases. "Unless" being the key work. It makes sense to warn when a __GFP_HIGH or __GFP_MEMALLOC allocation fails, because they are clearly important. It makes sense to warning if direct reclaim and retrying were enabled, as then alloc_page() has tried really hard, but failed anyway. Thought maybe if COSTLY_ORDER is exceeded, then the warning is unlikely to be interesting. But does it ever make sense to warn if either of __GFP_RETRY_MAYFAIL __GFP_NORETRY are present? If we always suppressed warning when those flags were present, then many (most?) uses for __GFP_NOWARN can be discarded. I can see that some of the __GFP flags are designed to each perform a single well-defined function and internally to mm/ that makes sense. But exposing those flags to all users appears to be a recipe for trouble. Hiding them all behind "__" doesn't stop people from using and misusing them. Others are externally meaningful. Making them visually similar to the ones we want to hide isn't helping anyone. When Willy wrote: > When we see users adding or subtracting __GFP flags, that's a problem. the "problem" is not so much in the fact that they *do* but in the fact that they *can*. I would be greatly in favour of GFP flags which made sense to callers, and a mapping to ALLOC_ flags in mm/ which makes sense to allocators. I doubt anything outside mm/ cares about whether KSWAPD is woken or not. IT probably should be for small-order allocations, and not so much for large-order allocations. but mm/khugepaged.c might make other decisions. Thanks, NeilBrown
