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
