| Message ID | 20240411091832.608280-1-haifeng.xu@shopee.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slub: Clear __GFP_COMP flag when allocating 0 order page | expand |
On Thu, 11 Apr 2024, Haifeng Xu wrote: > @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, > struct slab *slab; > unsigned int order = oo_order(oo); > > + /* > + * If fallback to the minimum order allocation and the order is 0, > + * clear the __GFP_COMP flag. > + */ > + if (order == 0) > + flags = flags & ~__GFP_COMP; This would be better placed in allocate_slab() when the need for a fallback to a lower order is detected after the first call to alloc_slab_page().
On 4/11/24 6:51 PM, Christoph Lameter (Ampere) wrote: > On Thu, 11 Apr 2024, Haifeng Xu wrote: > >> @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, >> struct slab *slab; >> unsigned int order = oo_order(oo); >> >> + /* >> + * If fallback to the minimum order allocation and the order is 0, >> + * clear the __GFP_COMP flag. >> + */ >> + if (order == 0) >> + flags = flags & ~__GFP_COMP; > > > This would be better placed in allocate_slab() when the need for a > fallback to a lower order is detected after the first call to alloc_slab_page(). Yeah. Although I don't really see the harm of __GFP_COMP with order-0 in the first place, if the only issue is that the error output might be confusing. I'd also hope we should eventually get rid of those odd non-__GFP_COMP high-order allocations and then can remove the flag.
On 2024/4/12 00:51, Christoph Lameter (Ampere) wrote: > On Thu, 11 Apr 2024, Haifeng Xu wrote: > >> @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, >> struct slab *slab; >> unsigned int order = oo_order(oo); >> >> + /* >> + * If fallback to the minimum order allocation and the order is 0, >> + * clear the __GFP_COMP flag. >> + */ >> + if (order == 0) >> + flags = flags & ~__GFP_COMP; > > > This would be better placed in allocate_slab() when the need for a > fallback to a lower order is detected after the first call to alloc_slab_page(). Yes. Thanks for your suggestion.
On Fri, Apr 12, 2024 at 10:01:29AM +0200, Vlastimil Babka wrote: > On 4/11/24 6:51 PM, Christoph Lameter (Ampere) wrote: > > On Thu, 11 Apr 2024, Haifeng Xu wrote: > > > >> @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, > >> struct slab *slab; > >> unsigned int order = oo_order(oo); > >> > >> + /* > >> + * If fallback to the minimum order allocation and the order is 0, > >> + * clear the __GFP_COMP flag. > >> + */ > >> + if (order == 0) > >> + flags = flags & ~__GFP_COMP; > > > > > > This would be better placed in allocate_slab() when the need for a > > fallback to a lower order is detected after the first call to alloc_slab_page(). > > Yeah. Although I don't really see the harm of __GFP_COMP with order-0 in the > first place, if the only issue is that the error output might be confusing. > I'd also hope we should eventually get rid of those odd non-__GFP_COMP > high-order allocations and then can remove the flag. The patch seems pointless to me. I wouldn't clear the flag. If somebody finds it confusing, that's really just their expectations being wrong. folio_alloc() sets __GFP_COMP on all allocations, whether or not they're order 0.
On 2024/4/12 20:17, Matthew Wilcox wrote: > On Fri, Apr 12, 2024 at 10:01:29AM +0200, Vlastimil Babka wrote: >> On 4/11/24 6:51 PM, Christoph Lameter (Ampere) wrote: >>> On Thu, 11 Apr 2024, Haifeng Xu wrote: >>> >>>> @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, >>>> struct slab *slab; >>>> unsigned int order = oo_order(oo); >>>> >>>> + /* >>>> + * If fallback to the minimum order allocation and the order is 0, >>>> + * clear the __GFP_COMP flag. >>>> + */ >>>> + if (order == 0) >>>> + flags = flags & ~__GFP_COMP; >>> >>> >>> This would be better placed in allocate_slab() when the need for a >>> fallback to a lower order is detected after the first call to alloc_slab_page(). >> >> Yeah. Although I don't really see the harm of __GFP_COMP with order-0 in the >> first place, if the only issue is that the error output might be confusing. >> I'd also hope we should eventually get rid of those odd non-__GFP_COMP >> high-order allocations and then can remove the flag. > > The patch seems pointless to me. I wouldn't clear the flag. If > somebody finds it confusing, that's really just their expectations being > wrong. folio_alloc() sets __GFP_COMP on all allocations, whether or not > they're order 0. If we don't care about the warnings at all, then higher-order and lower-order allocations can set __GFP_COMP when creating a new slab, just like folio_alloc(). If so, there is no need to check the order in calculate_sizes() and we can set __GFP_COMP in kmem_cache by default. diff --git a/mm/slub.c b/mm/slub.c index e7bf1a1a31a8..49a3ebefab86 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -4461,9 +4461,7 @@ static int calculate_sizes(struct kmem_cache *s) if ((int)order < 0) return 0; - s->allocflags = 0; - if (order) - s->allocflags |= __GFP_COMP; + s->allocflags = __GFP_COMP; if (s->flags & SLAB_CACHE_DMA) s->allocflags |= GFP_DMA;
On Fri, Apr 12, 2024 at 10:14:39PM +0800, Haifeng Xu wrote: > diff --git a/mm/slub.c b/mm/slub.c > index e7bf1a1a31a8..49a3ebefab86 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -4461,9 +4461,7 @@ static int calculate_sizes(struct kmem_cache *s) > if ((int)order < 0) > return 0; > > - s->allocflags = 0; > - if (order) > - s->allocflags |= __GFP_COMP; > + s->allocflags = __GFP_COMP; > > if (s->flags & SLAB_CACHE_DMA) > s->allocflags |= GFP_DMA; Resend this with a proper changelog and you can add: Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
diff --git a/mm/slub.c b/mm/slub.c index a307d319e82c..d3e03dcb9ff2 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -1875,6 +1875,13 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node, struct slab *slab; unsigned int order = oo_order(oo); + /* + * If fallback to the minimum order allocation and the order is 0, + * clear the __GFP_COMP flag. + */ + if (order == 0) + flags = flags & ~__GFP_COMP; + if (node == NUMA_NO_NODE) folio = (struct folio *)alloc_pages(flags, order); else
We encounter warning messages when failing to create a new slab like this: page allocation failure: order:0, mode:0x1004000(GFP_NOWAIT|__GFP_COMP), nodemask=(null) It's a bit confusing for users because __GFP_COMP flag is used to create compound page which implies the order should not be 0. This is because minimum order will be tried if higher-order allocation fails and the minimum order is 0. It's pointless to allocate 0 order page with __GFP_COMP flag. Therefore, clear the __GFP_COMP flag when falling back to 0 order allocation which makes the order and gfp flags matched. Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> --- mm/slub.c | 7 +++++++ 1 file changed, 7 insertions(+)