On 09/01/22 19:32, Matthew Wilcox wrote:
> 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.
Fair enough. No need to change. It just caught my eye.
> > 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.
I am terrible at names. hugetlb is storing pointers in the private fields.
FWICT, something like this would work.
>
> Does that sound sensible?
--
Mike Kravetz
