[PATCH] mm: remove offset check on page->compound_head and folio->lru

[PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

FOLIO_MATCH() is used to make sure struct page and folio has identical
layout for the first several words.

The comparison of offset between page->compound_head and folio->lru is
more like an internal check in struct page.

This patch just removes it.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/mm_types.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 0a2de709fe40..087d6768bc78 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -289,7 +289,6 @@ static_assert(sizeof(struct page) == sizeof(struct folio));
 FOLIO_MATCH(flags, flags);
 FOLIO_MATCH(lru, lru);
 FOLIO_MATCH(mapping, mapping);
-FOLIO_MATCH(compound_head, lru);
 FOLIO_MATCH(index, index);
 FOLIO_MATCH(private, private);
 FOLIO_MATCH(_mapcount, _mapcount);
-- 
2.33.1

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Matthew Wilcox]

On Thu, Jan 06, 2022 at 11:52:54PM +0000, Wei Yang wrote:
> FOLIO_MATCH() is used to make sure struct page and folio has identical
> layout for the first several words.
> 
> The comparison of offset between page->compound_head and folio->lru is
> more like an internal check in struct page.
> 
> This patch just removes it.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

No.

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Fri, Jan 07, 2022 at 03:59:01AM +0000, Matthew Wilcox wrote:
>On Thu, Jan 06, 2022 at 11:52:54PM +0000, Wei Yang wrote:
>> FOLIO_MATCH() is used to make sure struct page and folio has identical
>> layout for the first several words.
>> 
>> The comparison of offset between page->compound_head and folio->lru is
>> more like an internal check in struct page.
>> 
>> This patch just removes it.
>> 
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>No.

Hi, Matthew

Would you mind sharing some insight on this check?

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Matthew Wilcox]

On Fri, Jan 07, 2022 at 01:40:59PM +0000, Wei Yang wrote:
> On Fri, Jan 07, 2022 at 03:59:01AM +0000, Matthew Wilcox wrote:
> >On Thu, Jan 06, 2022 at 11:52:54PM +0000, Wei Yang wrote:
> >> FOLIO_MATCH() is used to make sure struct page and folio has identical
> >> layout for the first several words.
> >> 
> >> The comparison of offset between page->compound_head and folio->lru is
> >> more like an internal check in struct page.
> >> 
> >> This patch just removes it.
> >> 
> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> >
> >No.
> 
> Hi, Matthew
> 
> Would you mind sharing some insight on this check?

It's right there in the comments.  If you can't be bothered to read,
why should I write?

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Andrew Morton]

On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Fri, Jan 07, 2022 at 01:40:59PM +0000, Wei Yang wrote:
> > On Fri, Jan 07, 2022 at 03:59:01AM +0000, Matthew Wilcox wrote:
> > >On Thu, Jan 06, 2022 at 11:52:54PM +0000, Wei Yang wrote:
> > >> FOLIO_MATCH() is used to make sure struct page and folio has identical
> > >> layout for the first several words.
> > >> 
> > >> The comparison of offset between page->compound_head and folio->lru is
> > >> more like an internal check in struct page.
> > >> 
> > >> This patch just removes it.
> > >> 
> > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > >
> > >No.
> > 
> > Hi, Matthew
> > 
> > Would you mind sharing some insight on this check?
> 
> It's right there in the comments.

Well I can't figure out which comment you're referring to?

> If you can't be bothered to read, why should I write?

I don't think the punishment comes close to fitting the crime here :(

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Fri, Jan 07, 2022 at 10:11:20PM +0000, Matthew Wilcox wrote:
>> Hi, Matthew
>> 
>> Would you mind sharing some insight on this check?
>
>It's right there in the comments.  If you can't be bothered to read,
>why should I write?

I am not sure which comments it refers to.

Maybe we can add a "Refer to comment" above this code?

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Matthew Wilcox]

On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
> 
> > On Fri, Jan 07, 2022 at 01:40:59PM +0000, Wei Yang wrote:
> > > On Fri, Jan 07, 2022 at 03:59:01AM +0000, Matthew Wilcox wrote:
> > > >On Thu, Jan 06, 2022 at 11:52:54PM +0000, Wei Yang wrote:
> > > >> FOLIO_MATCH() is used to make sure struct page and folio has identical
> > > >> layout for the first several words.
> > > >> 
> > > >> The comparison of offset between page->compound_head and folio->lru is
> > > >> more like an internal check in struct page.
> > > >> 
> > > >> This patch just removes it.
> > > >> 
> > > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> > > >
> > > >No.
> > > 
> > > Hi, Matthew
> > > 
> > > Would you mind sharing some insight on this check?
> > 
> > It's right there in the comments.
> 
> Well I can't figure out which comment you're referring to?

         * WARNING: bit 0 of the first word is used for PageTail(). That
         * means the other users of this union MUST NOT use the bit to
         * avoid collision and false-positive PageTail().

> > If you can't be bothered to read, why should I write?
> 
> I don't think the punishment comes close to fitting the crime here :(

I felt I had to NACk it as quickly as possible so you didn't apply it.
Otherwise I'd've waited until I was back from holiday before replying
to the earlier patch.  But you applied that lickety-split, so clearly
I'm not allowed to take a week off.

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Andrew Morton]

On Sat, 8 Jan 2022 00:49:53 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> > > If you can't be bothered to read, why should I write?
> > 
> > I don't think the punishment comes close to fitting the crime here :(
> 
> I felt I had to NACk it as quickly as possible so you didn't apply it.
> Otherwise I'd've waited until I was back from holiday before replying
> to the earlier patch.  But you applied that lickety-split, so clearly
> I'm not allowed to take a week off.

https://lkml.kernel.org/r/20220104011734.21714-1-richard.weiyang@gmail.com ?

Yeah, I stashed that away on top of linux-next so it wouldn't fall
through the cracks.  But folios is Matthew stuff so I wouldn't expect
to be sending it upstream without you having commented on it.

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>> 
>> > > Hi, Matthew
>> > > 
>> > > Would you mind sharing some insight on this check?
>> > 
>> > It's right there in the comments.
>> 
>> Well I can't figure out which comment you're referring to?
>
>         * WARNING: bit 0 of the first word is used for PageTail(). That
>         * means the other users of this union MUST NOT use the bit to
>         * avoid collision and false-positive PageTail().
>

I know this requirement on bit 0 of first word. But I don't see the connection
between this and the check of page->compound_head and folio->lru.

This is more like a internal requirement on struct page. There are 8 struct in
this five words union. And this requirement apply to bit 0 of first word of
all those five struct.

To me, if folio has the same layout of page, folio meets this requirement. I
still not catch the point why we need this check here.

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Sat, Jan 08, 2022 at 08:13:40AM +0000, Wei Yang wrote:
>On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>>> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>>> 
>>> > > Hi, Matthew
>>> > > 
>>> > > Would you mind sharing some insight on this check?
>>> > 
>>> > It's right there in the comments.
>>> 
>>> Well I can't figure out which comment you're referring to?
>>
>>         * WARNING: bit 0 of the first word is used for PageTail(). That
>>         * means the other users of this union MUST NOT use the bit to
>>         * avoid collision and false-positive PageTail().
>>
>
>I know this requirement on bit 0 of first word. But I don't see the connection
>between this and the check of page->compound_head and folio->lru.
>
>This is more like a internal requirement on struct page. There are 8 struct in
>this five words union. And this requirement apply to bit 0 of first word of
>all those five struct.
>
>To me, if folio has the same layout of page, folio meets this requirement. I
>still not catch the point why we need this check here.
>

Hi, Matthew

Are you back from vocation? If you could give more insight on this check, I
would be appreciated.

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Vlastimil Babka]

On 1/23/22 02:38, Wei Yang wrote:
> On Sat, Jan 08, 2022 at 08:13:40AM +0000, Wei Yang wrote:
>>On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>>>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>>>> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>>>> 
>>>> > > Hi, Matthew
>>>> > > 
>>>> > > Would you mind sharing some insight on this check?
>>>> > 
>>>> > It's right there in the comments.
>>>> 
>>>> Well I can't figure out which comment you're referring to?
>>>
>>>         * WARNING: bit 0 of the first word is used for PageTail(). That
>>>         * means the other users of this union MUST NOT use the bit to
>>>         * avoid collision and false-positive PageTail().
>>>
>>
>>I know this requirement on bit 0 of first word. But I don't see the connection
>>between this and the check of page->compound_head and folio->lru.
>>
>>This is more like a internal requirement on struct page. There are 8 struct in
>>this five words union. And this requirement apply to bit 0 of first word of
>>all those five struct.
>>
>>To me, if folio has the same layout of page, folio meets this requirement. I
>>still not catch the point why we need this check here.
>>
> 
> Hi, Matthew
> 
> Are you back from vocation? If you could give more insight on this check, I
> would be appreciated.

I can offer my insight (which might be of course wrong). Ideally one day
page.lru will be gone and only folio will be used for LRU pages. Then there
won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
won't appear to be redundant anymore. lru is list_head so two pointers and
thus valid pointers are aligned in such a way they can't accidentaly set the
bit 0.

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
>On 1/23/22 02:38, Wei Yang wrote:
>> On Sat, Jan 08, 2022 at 08:13:40AM +0000, Wei Yang wrote:
>>>On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>>>>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>>>>> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>>>>> 
>>>>> > > Hi, Matthew
>>>>> > > 
>>>>> > > Would you mind sharing some insight on this check?
>>>>> > 
>>>>> > It's right there in the comments.
>>>>> 
>>>>> Well I can't figure out which comment you're referring to?
>>>>
>>>>         * WARNING: bit 0 of the first word is used for PageTail(). That
>>>>         * means the other users of this union MUST NOT use the bit to
>>>>         * avoid collision and false-positive PageTail().
>>>>
>>>
>>>I know this requirement on bit 0 of first word. But I don't see the connection
>>>between this and the check of page->compound_head and folio->lru.
>>>
>>>This is more like a internal requirement on struct page. There are 8 struct in
>>>this five words union. And this requirement apply to bit 0 of first word of
>>>all those five struct.
>>>
>>>To me, if folio has the same layout of page, folio meets this requirement. I
>>>still not catch the point why we need this check here.
>>>
>> 
>> Hi, Matthew
>> 
>> Are you back from vocation? If you could give more insight on this check, I
>> would be appreciated.
>
>I can offer my insight (which might be of course wrong). Ideally one day
>page.lru will be gone and only folio will be used for LRU pages. Then there
>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>won't appear to be redundant anymore. lru is list_head so two pointers and

Thanks for your comment.

I can't imagine the final result. If we would remove page.lru, we could remove
FOLIO_MATCH(lru, lru) and add FOLIO_MATCH(compound_head, lru) at that moment?

>thus valid pointers are aligned in such a way they can't accidentaly set the
>bit 0.
>

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Vlastimil Babka]

On 1/24/22 23:55, Wei Yang wrote:
> On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
>>On 1/23/22 02:38, Wei Yang wrote:
>>> On Sat, Jan 08, 2022 at 08:13:40AM +0000, Wei Yang wrote:
>>>>On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>>>>>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>>>>
>>>>To me, if folio has the same layout of page, folio meets this requirement. I
>>>>still not catch the point why we need this check here.
>>>>
>>> 
>>> Hi, Matthew
>>> 
>>> Are you back from vocation? If you could give more insight on this check, I
>>> would be appreciated.
>>
>>I can offer my insight (which might be of course wrong). Ideally one day
>>page.lru will be gone and only folio will be used for LRU pages. Then there
>>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>>won't appear to be redundant anymore. lru is list_head so two pointers and
> 
> Thanks for your comment.
> 
> I can't imagine the final result. If we would remove page.lru, we could remove
> FOLIO_MATCH(lru, lru) and add FOLIO_MATCH(compound_head, lru) at that moment?

Yes, or we could forget to do it. Adding it right now is another option that
Matthew has chosen and I don't see a strong reason to change it. Can you
measure a kernel build speedup thanks to removing the now redundant check?

>>thus valid pointers are aligned in such a way they can't accidentaly set the
>>bit 0.
>>
> 

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Tue, Jan 25, 2022 at 11:11:40AM +0100, Vlastimil Babka wrote:
>On 1/24/22 23:55, Wei Yang wrote:
>> On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
>>>On 1/23/22 02:38, Wei Yang wrote:
>>>> On Sat, Jan 08, 2022 at 08:13:40AM +0000, Wei Yang wrote:
>>>>>On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>>>>>>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>>>>>
>>>>>To me, if folio has the same layout of page, folio meets this requirement. I
>>>>>still not catch the point why we need this check here.
>>>>>
>>>> 
>>>> Hi, Matthew
>>>> 
>>>> Are you back from vocation? If you could give more insight on this check, I
>>>> would be appreciated.
>>>
>>>I can offer my insight (which might be of course wrong). Ideally one day
>>>page.lru will be gone and only folio will be used for LRU pages. Then there
>>>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>>>won't appear to be redundant anymore. lru is list_head so two pointers and
>> 
>> Thanks for your comment.
>> 
>> I can't imagine the final result. If we would remove page.lru, we could remove
>> FOLIO_MATCH(lru, lru) and add FOLIO_MATCH(compound_head, lru) at that moment?
>
>Yes, or we could forget to do it. Adding it right now is another option that
>Matthew has chosen and I don't see a strong reason to change it. Can you
>measure a kernel build speedup thanks to removing the now redundant check?
>

If we forget to do it, the compile would fail, right?

Put it here for a future reason is not persuasive.

>>>thus valid pointers are aligned in such a way they can't accidentaly set the
>>>bit 0.
>>>
>> 

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Vlastimil Babka]

On 1/27/22 02:10, Wei Yang wrote:
> On Tue, Jan 25, 2022 at 11:11:40AM +0100, Vlastimil Babka wrote:
>>On 1/24/22 23:55, Wei Yang wrote:
>>> On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
>>>>On 1/23/22 02:38, Wei Yang wrote:
>>>>
>>>>I can offer my insight (which might be of course wrong). Ideally one day
>>>>page.lru will be gone and only folio will be used for LRU pages. Then there
>>>>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>>>>won't appear to be redundant anymore. lru is list_head so two pointers and
>>> 
>>> Thanks for your comment.
>>> 
>>> I can't imagine the final result. If we would remove page.lru, we could remove
>>> FOLIO_MATCH(lru, lru) and add FOLIO_MATCH(compound_head, lru) at that moment?
>>
>>Yes, or we could forget to do it. Adding it right now is another option that
>>Matthew has chosen and I don't see a strong reason to change it. Can you
>>measure a kernel build speedup thanks to removing the now redundant check?
>>
> 
> If we forget to do it, the compile would fail, right?

No, FOLIO_MATCH is like a build-time assert. It can only fail if the assert
is there, and the condition evaluates to false.
If it's not there and the condition is false, it will instead mysteriously
crash on runtime, which is worse.

> Put it here for a future reason is not persuasive.
> 
>>>>thus valid pointers are aligned in such a way they can't accidentaly set the
>>>>bit 0.
>>>>
>>> 
> 

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Thu, Jan 27, 2022 at 04:42:10PM +0100, Vlastimil Babka wrote:
>On 1/27/22 02:10, Wei Yang wrote:
>> On Tue, Jan 25, 2022 at 11:11:40AM +0100, Vlastimil Babka wrote:
>>>On 1/24/22 23:55, Wei Yang wrote:
>>>> On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
>>>>>On 1/23/22 02:38, Wei Yang wrote:
>>>>>
>>>>>I can offer my insight (which might be of course wrong). Ideally one day
>>>>>page.lru will be gone and only folio will be used for LRU pages. Then there
>>>>>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>>>>>won't appear to be redundant anymore. lru is list_head so two pointers and
>>>> 
>>>> Thanks for your comment.
>>>> 
>>>> I can't imagine the final result. If we would remove page.lru, we could remove
>>>> FOLIO_MATCH(lru, lru) and add FOLIO_MATCH(compound_head, lru) at that moment?
>>>
>>>Yes, or we could forget to do it. Adding it right now is another option that
>>>Matthew has chosen and I don't see a strong reason to change it. Can you
>>>measure a kernel build speedup thanks to removing the now redundant check?
>>>
>> 
>> If we forget to do it, the compile would fail, right?
>
>No, FOLIO_MATCH is like a build-time assert. It can only fail if the assert
>is there, and the condition evaluates to false.

Currently we have this check

  FOLIO_MATCH(lru, lru)

Which checks page->lru and folio->lru.

As you mentioned page->lru would be gone. So this check would fail at compile?


-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Sat, Jan 08, 2022 at 12:49:53AM +0000, Matthew Wilcox wrote:
>On Fri, Jan 07, 2022 at 04:08:25PM -0800, Andrew Morton wrote:
>> On Fri, 7 Jan 2022 22:11:20 +0000 Matthew Wilcox <willy@infradead.org> wrote:
>> 
[...]
>> > > Hi, Matthew
>> > > 
>> > > Would you mind sharing some insight on this check?
>> > 
>> > It's right there in the comments.
>> 
>> Well I can't figure out which comment you're referring to?
>
>         * WARNING: bit 0 of the first word is used for PageTail(). That
>         * means the other users of this union MUST NOT use the bit to
>         * avoid collision and false-positive PageTail().
>
>> > If you can't be bothered to read, why should I write?
>> 

Hi, Matthew

This change is introduced in commit 1d798ca3f164 'mm: make compound_head()
robust'.

As mentioned in the changelog.

```
    That means page->compound_head shares storage space with:

    - page->lru.next;
    - page->next;
    - page->rcu_head.next;
```

We need to make sure those fields in page don't use bit 0 of the word.

So this is an internal guarantee in struct page. I don't see the reason to
compare page->compound_head and folio->lru here.

Maybe I miss something. If you would explain a little, I would appreciate
much.

-- 
Wei Yang
Help you, Help me

Re: [PATCH] mm: remove offset check on page->compound_head and folio->lru

[Wei Yang]

On Mon, Jan 24, 2022 at 11:30:10AM +0100, Vlastimil Babka wrote:
[...]
>> Hi, Matthew
>> 
>> Are you back from vocation? If you could give more insight on this check, I
>> would be appreciated.
>
>I can offer my insight (which might be of course wrong). Ideally one day
>page.lru will be gone and only folio will be used for LRU pages. Then there
>won't be a  FOLIO_MATCH(lru, lru); and FOLIO_MATCH(compound_head, lru);
>won't appear to be redundant anymore. lru is list_head so two pointers and
>thus valid pointers are aligned in such a way they can't accidentaly set the
>bit 0.
>

Hmm... I see your change on struct slab. I guess I got your point.

Seems we would shrink struct page...

-- 
Wei Yang
Help you, Help me