Message-ID: <YxD6215MS+L+tGLc@casper.infradead.org>
Date:   Thu, 1 Sep 2022 19:32:59 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Mike Kravetz <mike.kravetz@...cle.com>
Cc:     Sidhartha Kumar <sidhartha.kumar@...cle.com>,
        linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        akpm@...ux-foundation.org, songmuchun@...edance.com,
        vbabka@...e.cz, william.kucharski@...cle.com, dhowells@...hat.com,
        peterx@...hat.com, arnd@...db.de, ccross@...gle.com,
        hughd@...gle.com, ebiederm@...ssion.com
Subject: Re: [PATCH 2/7] mm: add private field of first tail to struct page
 and struct folio

On Thu, Sep 01, 2022 at 10:32:43AM -0700, Mike Kravetz wrote:
> Not really an issue with this patch, but it made me read more of this
> comment about folios.  It goes on to say ...
> 
>  * same power-of-two.  It is at least as large as %PAGE_SIZE.  If it is
>  * in the page cache, it is at a file offset which is a multiple of that
>  * power-of-two.  It may be mapped into userspace at an address which is
>  * at an arbitrary page offset, but its kernel virtual address is aligned
>  * to its size.
>  */
> 
> This series is to begin converting hugetlb code to folios.  Just want to
> note that 'hugetlb folios' have specific user space alignment restrictions.
> So, I do not think the comment about arbitrary page offset would apply to
> hugetlb.
> 
> Matthew, should we note that hugetlb is special in the comment?  Or, is it
> not worth updating?

I'm open to updating it if we can find good wording.  What I'm trying
to get across there is that when dealing with folios, you can assume
that they're naturally aligned physically, logically (in the file) and
virtually (kernel address), but not necessarily virtually (user
address).  Hugetlb folios are special in that they are guaranteed to
be virtually aligned in user space, but I don't know if here is the
right place to document that.  It's an additional restriction, so code
which handles generic folios doesn't need to know it.

> Also, folio_get_private_1 will be used for the hugetlb subpool pointer
> which resides in page[1].private.  This is used in the next patch of
> this series.  I'm sure you are aware that hugetlb also uses page private
> in sub pages 2 and 3.  Can/will/should this method of accessing private
> in sub pages be expanded to cover these as well?  Expansion can happen
> later, but if this can not be expanded perhaps we should come up with
> another scheme.

There's a few ways of tackling this.  What I'm currently thinking is
that we change how hugetlbfs uses struct page to store its extra data.
It would end up looking something like this (in struct page):

+++ b/include/linux/mm_types.h
@@ -147,9 +147,10 @@ struct page {
                };
                struct {        /* Second tail page of compound page */
                        unsigned long _compound_pad_1;  /* compound_head */
-                       unsigned long _compound_pad_2;
                        /* For both global and memcg */
                        struct list_head deferred_list;
+                       unsigned long hugetlbfs_private_2;
+                       unsigned long hugetlbfs_private_3;
                };
                struct {        /* Page table pages */
                        unsigned long _pt_pad_1;        /* compound_head */

although we could use better names and/or types?  I haven't looked to
see what you're storing here yet.  And then we can make the
corresponding change to struct folio to add these elements at the
right place.

Does that sound sensible?

Powered by blists - more mailing lists