Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Hugh Dickins]

On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:

> From: Hugh Dickins <hughd@google.com>
> 
> commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
> 
> PageDoubleMap is maintained differently for anon and for shmem+file: the
> shmem+file one was never cleared, because a safe place to do so could
> not be found; so it would blight future use of the cached hugepage until
> evicted.
> 
> See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
> 
> But page_add_file_rmap() does provide a safe place to do so (though later
> than one might wish): allowing testing to return to an initial state
> without a damaging drop_caches.
> 
> Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
> Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> Reviewed-by: Yang Shi <shy828301@gmail.com>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

NAK.

I thought we had a long-standing agreement that AUTOSEL does not try
to add patches from akpm's tree which had not been marked for stable.

(Whereas, if a developer asks for such a patch to be added to stable
later, and verifies the result, that's of course a different matter.)

I've chosen to answer to this patch of my 3 in your 14 AUTOSELs,
because this one is just an improvement, not at all a bugfix needed
for stable (maybe AUTOSEL noticed "racy" or "safely" in the comments,
and misunderstood).  The "Fixes" was intended to help any humans who
wanted to backport into their trees.

I do recall that this 13/14, and 14/14, are mods to mm/rmap.c
which followed other (mm/munlock) mods to mm/rmap.c in 5.18-rc1,
which affected the out path of the function involved, and somehow
made 14/14 a little cleaner.  I'm sorry, but I just don't rate it
worth my time at the moment, to verify whether 14/14 happens to
have ended up as a correct patch or not.

And nobody can verify them without these AUTOSELs saying to which
tree they are targeted - 5.17 I suppose.

Hugh

> ---
>  mm/rmap.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9e27f9f038d3..444d0d958aff 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1252,6 +1252,17 @@ void page_add_file_rmap(struct page *page, bool compound)
>  		}
>  		if (!atomic_inc_and_test(compound_mapcount_ptr(page)))
>  			goto out;
> +
> +		/*
> +		 * It is racy to ClearPageDoubleMap in page_remove_file_rmap();
> +		 * but page lock is held by all page_add_file_rmap() compound
> +		 * callers, and SetPageDoubleMap below warns if !PageLocked:
> +		 * so here is a place that DoubleMap can be safely cleared.
> +		 */
> +		VM_WARN_ON_ONCE(!PageLocked(page));
> +		if (nr == nr_pages && PageDoubleMap(page))
> +			ClearPageDoubleMap(page);
> +
>  		if (PageSwapBacked(page))
>  			__mod_lruvec_page_state(page, NR_SHMEM_PMDMAPPED,
>  						nr_pages);
> -- 
> 2.36.0

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Greg Kroah-Hartman]

On Thu, Apr 28, 2022 at 09:51:58AM -0700, Hugh Dickins wrote:
> On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> 
> > From: Hugh Dickins <hughd@google.com>
> > 
> > commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
> > 
> > PageDoubleMap is maintained differently for anon and for shmem+file: the
> > shmem+file one was never cleared, because a safe place to do so could
> > not be found; so it would blight future use of the cached hugepage until
> > evicted.
> > 
> > See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
> > 
> > But page_add_file_rmap() does provide a safe place to do so (though later
> > than one might wish): allowing testing to return to an initial state
> > without a damaging drop_caches.
> > 
> > Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
> > Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
> > Signed-off-by: Hugh Dickins <hughd@google.com>
> > Reviewed-by: Yang Shi <shy828301@gmail.com>
> > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> NAK.
> 
> I thought we had a long-standing agreement that AUTOSEL does not try
> to add patches from akpm's tree which had not been marked for stable.

True, this was my attempt at saying "hey these all look like they should
go to stable trees, why not?"

> I've chosen to answer to this patch of my 3 in your 14 AUTOSELs,
> because this one is just an improvement, not at all a bugfix needed
> for stable (maybe AUTOSEL noticed "racy" or "safely" in the comments,
> and misunderstood).  The "Fixes" was intended to help any humans who
> wanted to backport into their trees.

This all was off of the Fixes: tag.  Again, if these commits fix
something why are they not for stable?  I'm a human asking to backport
these into the stable trees based on that :)

> I do recall that this 13/14, and 14/14, are mods to mm/rmap.c
> which followed other (mm/munlock) mods to mm/rmap.c in 5.18-rc1,
> which affected the out path of the function involved, and somehow
> made 14/14 a little cleaner.  I'm sorry, but I just don't rate it
> worth my time at the moment, to verify whether 14/14 happens to
> have ended up as a correct patch or not.
> 
> And nobody can verify them without these AUTOSELs saying to which
> tree they are targeted - 5.17 I suppose.

5.17 to start with, older ones based on where the Fixes: tags went to.

So do you really want me to drop these?  I will but why are you adding
fixes: tags if you don't want people to take them?

thanks,

greg k-h

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Hugh Dickins]

On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> On Thu, Apr 28, 2022 at 09:51:58AM -0700, Hugh Dickins wrote:
> > On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> > 
> > > From: Hugh Dickins <hughd@google.com>
> > > 
> > > commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
> > > 
> > > PageDoubleMap is maintained differently for anon and for shmem+file: the
> > > shmem+file one was never cleared, because a safe place to do so could
> > > not be found; so it would blight future use of the cached hugepage until
> > > evicted.
> > > 
> > > See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
> > > 
> > > But page_add_file_rmap() does provide a safe place to do so (though later
> > > than one might wish): allowing testing to return to an initial state
> > > without a damaging drop_caches.
> > > 
> > > Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
> > > Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
> > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > Reviewed-by: Yang Shi <shy828301@gmail.com>
> > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > 
> > NAK.
> > 
> > I thought we had a long-standing agreement that AUTOSEL does not try
> > to add patches from akpm's tree which had not been marked for stable.
> 
> True, this was my attempt at saying "hey these all look like they should
> go to stable trees, why not?"

Okay, it seems I should have read "AUTOSEL" as "Hey, GregKH here,
these all look like they should go to stable trees, why not?",
which would have drawn a friendlier response.

The answer is that I considered stable at the time, and akpm did too,
and none of my three (I've not looked through the other 11) are serious
enough to be needed in stable; and I'm cautious about backports, because
I know that the tree they went on top of differs thereabouts from 5.17.

Of course I think the patches in 5.18-rc are good, and yes, they're
things I've thought worthwhile enough for me personally to port forward
over several releases until I had time to send in.  But that doesn't
make them safe stable candidates, without someone to verify and vouch
for the results in this or that tree - I run on a much slower clock
than you and most around here, I do not have time for that at present
(and would prefer not even to be having this conversation).

But I'm happily overruled if any mm guys think they are worth that
extra effort, and will verify and vouch for them.

> 
> > I've chosen to answer to this patch of my 3 in your 14 AUTOSELs,
> > because this one is just an improvement, not at all a bugfix needed
> > for stable (maybe AUTOSEL noticed "racy" or "safely" in the comments,
> > and misunderstood).  The "Fixes" was intended to help any humans who
> > wanted to backport into their trees.
> 
> This all was off of the Fixes: tag.  Again, if these commits fix
> something why are they not for stable?  I'm a human asking to backport
> these into the stable trees based on that :)

Your humanity is not in doubt :)  But I think we've gone over this
too many times - each year?  There's a "Fixes:" tag and "Cc: stable"
tag, and in akpm's tree we prefer to be able to specify "Fixes:" to
help each other, without that automatically implying "Cc: stable".
Andrew goes to considerable trouble to determine when "Cc: stable"
is appropriate.

> 
> > I do recall that this 13/14, and 14/14, are mods to mm/rmap.c
> > which followed other (mm/munlock) mods to mm/rmap.c in 5.18-rc1,
> > which affected the out path of the function involved, and somehow
> > made 14/14 a little cleaner.  I'm sorry, but I just don't rate it
> > worth my time at the moment, to verify whether 14/14 happens to
> > have ended up as a correct patch or not.
> > 
> > And nobody can verify them without these AUTOSELs saying to which
> > tree they are targeted - 5.17 I suppose.
> 
> 5.17 to start with, older ones based on where the Fixes: tags went to.
> 
> So do you really want me to drop these?  I will but why are you adding
> fixes: tags if you don't want people to take them?

Yes, please drop them - thanks.  As to the other 11: I hope authors
will speak up one way or the other, but I'll drop out now.

Hugh

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Sean Christopherson]

+Sasha and Paolo

On Thu, Apr 28, 2022, Hugh Dickins wrote:
> On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> > On Thu, Apr 28, 2022 at 09:51:58AM -0700, Hugh Dickins wrote:
> > > On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> > > 
> > > > From: Hugh Dickins <hughd@google.com>
> > > > 
> > > > commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
> > > > 
> > > > PageDoubleMap is maintained differently for anon and for shmem+file: the
> > > > shmem+file one was never cleared, because a safe place to do so could
> > > > not be found; so it would blight future use of the cached hugepage until
> > > > evicted.
> > > > 
> > > > See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
> > > > 
> > > > But page_add_file_rmap() does provide a safe place to do so (though later
> > > > than one might wish): allowing testing to return to an initial state
> > > > without a damaging drop_caches.
> > > > 
> > > > Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
> > > > Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
> > > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > > Reviewed-by: Yang Shi <shy828301@gmail.com>
> > > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > 
> > > NAK.
> > > 
> > > I thought we had a long-standing agreement that AUTOSEL does not try
> > > to add patches from akpm's tree which had not been marked for stable.
> > 
> > True, this was my attempt at saying "hey these all look like they should
> > go to stable trees, why not?"
> 
> Okay, it seems I should have read "AUTOSEL" as "Hey, GregKH here,
> these all look like they should go to stable trees, why not?",
> which would have drawn a friendlier response.

FWIW, Sasha has been using MANUALSEL for the KVM tree to solicit an explicit ACK
from Paolo for these types of patches.  AFAICT, it has been working quite well.

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Greg Kroah-Hartman]

On Thu, Apr 28, 2022 at 10:45:18PM +0000, Sean Christopherson wrote:
> +Sasha and Paolo
> 
> On Thu, Apr 28, 2022, Hugh Dickins wrote:
> > On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> > > On Thu, Apr 28, 2022 at 09:51:58AM -0700, Hugh Dickins wrote:
> > > > On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
> > > > 
> > > > > From: Hugh Dickins <hughd@google.com>
> > > > > 
> > > > > commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
> > > > > 
> > > > > PageDoubleMap is maintained differently for anon and for shmem+file: the
> > > > > shmem+file one was never cleared, because a safe place to do so could
> > > > > not be found; so it would blight future use of the cached hugepage until
> > > > > evicted.
> > > > > 
> > > > > See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
> > > > > 
> > > > > But page_add_file_rmap() does provide a safe place to do so (though later
> > > > > than one might wish): allowing testing to return to an initial state
> > > > > without a damaging drop_caches.
> > > > > 
> > > > > Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
> > > > > Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
> > > > > Signed-off-by: Hugh Dickins <hughd@google.com>
> > > > > Reviewed-by: Yang Shi <shy828301@gmail.com>
> > > > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > > > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> > > > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> > > > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > 
> > > > NAK.
> > > > 
> > > > I thought we had a long-standing agreement that AUTOSEL does not try
> > > > to add patches from akpm's tree which had not been marked for stable.
> > > 
> > > True, this was my attempt at saying "hey these all look like they should
> > > go to stable trees, why not?"
> > 
> > Okay, it seems I should have read "AUTOSEL" as "Hey, GregKH here,
> > these all look like they should go to stable trees, why not?",
> > which would have drawn a friendlier response.
> 
> FWIW, Sasha has been using MANUALSEL for the KVM tree to solicit an explicit ACK
> from Paolo for these types of patches.  AFAICT, it has been working quite well.

Yes, that is what I should have put here, sorry about that.  These were
manually picked by me and I am asking if they should be included or not.
I'll resend after dropping Hugh's patches from the series.

thanks,

greg k-h

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Sasha Levin]

On Thu, Apr 28, 2022 at 12:27:40PM -0700, Hugh Dickins wrote:
>On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
>> On Thu, Apr 28, 2022 at 09:51:58AM -0700, Hugh Dickins wrote:
>> > On Thu, 28 Apr 2022, Greg Kroah-Hartman wrote:
>> >
>> > > From: Hugh Dickins <hughd@google.com>
>> > >
>> > > commit bd55b0c2d64e84a75575f548a33a3dfecc135b65 upstream.
>> > >
>> > > PageDoubleMap is maintained differently for anon and for shmem+file: the
>> > > shmem+file one was never cleared, because a safe place to do so could
>> > > not be found; so it would blight future use of the cached hugepage until
>> > > evicted.
>> > >
>> > > See https://lore.kernel.org/lkml/1571938066-29031-1-git-send-email-yang.shi@linux.alibaba.com/
>> > >
>> > > But page_add_file_rmap() does provide a safe place to do so (though later
>> > > than one might wish): allowing testing to return to an initial state
>> > > without a damaging drop_caches.
>> > >
>> > > Link: https://lkml.kernel.org/r/61c5cf99-a962-9a25-597a-53ab1bd8fbc0@google.com
>> > > Fixes: 9a73f61bdb8a ("thp, mlock: do not mlock PTE-mapped file huge pages")
>> > > Signed-off-by: Hugh Dickins <hughd@google.com>
>> > > Reviewed-by: Yang Shi <shy828301@gmail.com>
>> > > Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>> > > Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>> > > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> > > Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> >
>> > NAK.
>> >
>> > I thought we had a long-standing agreement that AUTOSEL does not try
>> > to add patches from akpm's tree which had not been marked for stable.

I guess it was only between myself and mm/ :p

>> True, this was my attempt at saying "hey these all look like they should
>> go to stable trees, why not?"
>
>Okay, it seems I should have read "AUTOSEL" as "Hey, GregKH here,
>these all look like they should go to stable trees, why not?",
>which would have drawn a friendlier response.

FRIENDLYGREGBOT :)

>The answer is that I considered stable at the time, and akpm did too,
>and none of my three (I've not looked through the other 11) are serious
>enough to be needed in stable; and I'm cautious about backports, because
>I know that the tree they went on top of differs thereabouts from 5.17.
>
>Of course I think the patches in 5.18-rc are good, and yes, they're
>things I've thought worthwhile enough for me personally to port forward
>over several releases until I had time to send in.  But that doesn't
>make them safe stable candidates, without someone to verify and vouch
>for the results in this or that tree - I run on a much slower clock
>than you and most around here, I do not have time for that at present
>(and would prefer not even to be having this conversation).
>
>But I'm happily overruled if any mm guys think they are worth that
>extra effort, and will verify and vouch for them.

What's the extra effort here? We're seeing so many cases where we see
issues with LTS kernels and we end up spending so much time triaging and
diagnosing them only to find out that they've already been fixed.

Honesly, having them in -stable seems like *less* effort to me.

-- 
Thanks,
Sasha

Re: [PATCH AUTOSEL 13/14] mm/thp: ClearPageDoubleMap in first page_add_file_rmap()

[Pavel Machek]

Hi!

> > I've chosen to answer to this patch of my 3 in your 14 AUTOSELs,
> > because this one is just an improvement, not at all a bugfix needed
> > for stable (maybe AUTOSEL noticed "racy" or "safely" in the comments,
> > and misunderstood).  The "Fixes" was intended to help any humans who
> > wanted to backport into their trees.
> 
> This all was off of the Fixes: tag.  Again, if these commits fix
> something why are they not for stable?  I'm a human asking to backport
> these into the stable trees based on that :)

I see this as a repeated pattern: People add Fixes: tag for trivial
things that should not really go to stable (typo in comment?) and
stable takes it is a serious bug that needs to be fixed in stable.

Best regards,
								Pavel