Re: [PATCH RFC 7/9] mm/gup: Decrement head page once for group of subpages

[Jason Gunthorpe <jgg@ziepe.ca>]

On Tue, Dec 08, 2020 at 09:06:50PM -0800, John Hubbard wrote:
> > I suggest using clear language 'page' here should always be a compound
> > head called 'head' (or do we have another common variable name for
> > this?)
> 
> Agreed. Matthew's struct folio upgrade will allow us to really make
> things clear in a typesafe way, but meanwhile, it's probably good to use
> one of the following patterns:

Yes, this fits very well with the folio patches, and is much clearer

> page = compound_head(page); // at the very beginning of a routine

No, these routines really want to operate on head/folio's, that is the whole
point.
 
> do_things_to_this_single_page(page);
> 
> head = compound_head(page);
> do_things_to_this_compound_page(head);

Yes, but wordy though

> > Is it safe to call mod_node_page_state() after releasing the refcount?
> > This could race with hot-unplugging the struct pages so I think it is
> > wrong.
> 
> Yes, I think you are right! I wasn't in a hot unplug state of mind when I
> thought about the ordering there, but I should have been. :)

Ok

Hmm..

unpin_user_page() and put_compound_head() do exactly the same thing, 
and the latter gets it all right. I'll make a patch to fix this

> > And maybe you open code that iteration, but that basic idea to find a
> > compound_head and ntails should be computational work performed.
> > 
> > No reason not to fix set_page_dirty_lock() too while you are here.
> 
> Eh? What's wrong with set_page_dirty_lock() ?

Look at the code:

	for (index = 0; index < npages; index++) {
		struct page *page = compound_head(pages[index]);

		if (!PageDirty(page))
			set_page_dirty_lock(page);

So we really want

   set_folio_dirty_lock(folio, ntails)

Just like unpin_user_folio(folio, ntails)

(wow this is much clearer to explain using Matt's language)

set_page_dirty_lock does another wack of atomics so this should be
a healthy speedup on the large page benchmark.

Jason