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 <[email protected]>
---
mm/slub.c | 7 +++++++
1 file changed, 7 insertions(+)
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
--
2.25.1
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) <[email protected]>