Re: [RFC PATCH 1/5] mm/mlock: rework mlock_count to use _mapcount for order-0 folios

[Yosry Ahmed]

On Sun, Jun 18, 2023 at 4:20 AM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> On Sun, Jun 18, 2023 at 06:57:44AM +0000, Yosry Ahmed wrote:
> > @@ -337,6 +318,7 @@ struct folio {
> >                       atomic_t _entire_mapcount;
> >                       atomic_t _nr_pages_mapped;
> >                       atomic_t _pincount;
> > +                     atomic_t _mlock_count;
> >  #ifdef CONFIG_64BIT
> >                       unsigned int _folio_nr_pages;
> >  #endif
>
> You can't quite do this.  On 32-bt systems (I know, I know ...),
> we have:
>
> offset  page 0          page 1
> 0       flags           flags
> 4       lru             head
> 8       lru             dtor+order
> 12      mapping         entire_mapcount
> 16      index           nr_pages_napped
> 20      private         pincount
> 24      mapcount
> 28      refcount
>
> so it actually ends up overlapping page->mapcount on the second page,
> which is of course used for counting the number of PTEs which map
> that specific page.

Ah yeah, of course. In retrospect, it was very silly of me not to
notice given that I was modifying the mapcount handling code.

Thanks for pointing this out.

>
> I don't have a scenario where this would matter, but we are quite
> careful to only allocate order-2+ large folios, so I'd suggest putting
> it in page 2 instead of page 1.

Can you point me to the code that does so?

>
> I should probably add a comment to struct folio warning of this dragon.
> I thought the #ifdef CONFIG_64BIT would be enough to warn unwary
> passers-by of its presence, but a more explicit sign must be in order.
>

I saw the CONFIG_64BIT and spent some time trying to figure out why we
need it. I added the size of all explicit fields on 32-bit and they
were less than 32 bytes, so I was confused. I looked at the commit log
with no luck. I should have realized we should not overlay mapcount or
refcount. The presence of _mapcount_1/_refcount_1 fields could have
helped.

Anyway, we can also put it in page 1 inside #ifdef CONFIG_64BIT. We
will need a few extra #ifdef's in mm/mlock.c, and I think that's
pretty much it. For 32-bit, we can just keep piggybacking on mapcount
for all folios, I doubt a huge number of mappings is a concern for
32-bit anyway. I don't have a strong preference.