Re: [PATCH v3 4/6] mm: Introduce a pageflag for partially mapped folios

[Matthew Wilcox <willy@infradead.org>]

On Fri, Aug 16, 2024 at 05:08:35PM +0100, Usama Arif wrote:
> On 16/08/2024 16:44, Matthew Wilcox wrote:
> > On Tue, Aug 13, 2024 at 01:02:47PM +0100, Usama Arif wrote:
> >> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> >> index a0a29bd092f8..cecc1bad7910 100644
> >> --- a/include/linux/page-flags.h
> >> +++ b/include/linux/page-flags.h
> >> @@ -182,6 +182,7 @@ enum pageflags {
> >>  	/* At least one page in this folio has the hwpoison flag set */
> >>  	PG_has_hwpoisoned = PG_active,
> >>  	PG_large_rmappable = PG_workingset, /* anon or file-backed */
> >> +	PG_partially_mapped, /* was identified to be partially mapped */
> > 
> > No, you can't do this.  You have to be really careful when reusing page
> > flags, you can't just take the next one.  What made you think it would
> > be this easy?
> > 
> > I'd suggest using PG_reclaim.  You also need to add PG_partially_mapped
> > to PAGE_FLAGS_SECOND.  You might get away without that if you're
> > guaranteeing it'll always be clear when you free the folio; I don't
> > understand this series so I don't know if that's true or not.
> 
> I am really not sure what the issue is over here.

You've made the code more fragile.  It might happen to work today, but
you've either done something which is subtly broken today, or might
break tomorrow when somebody else rearranges the flags without knowing
about your fragility.

> >From what I see, bits 0-7 of folio->_flags_1 are used for storing folio order, bit 8 for PG_has_hwpoisoned and bit 9 for PG_large_rmappable.
> Bits 10 and above of folio->_flags_1 are not used any anywhere in the kernel. I am not reusing a page flag of folio->_flags_1, just taking an unused one.

No, wrong.  PG_anon_exclusive is used on every page, including tail
pages, and that's above bit 10.

> Please have a look at the next few lines of the patch. I have defined the functions as FOLIO_FLAG(partially_mapped, FOLIO_SECOND_PAGE). I believe thats what you are saying in your second paragraph?
> I am not sure what you meant by using PG_reclaim.

I mean:

-	PG_usama_new_thing,
+	PG_usama_new_thing = PG_reclaim,