Messages in this thread Patch in this message | | | Date | Mon, 10 Jan 2022 22:40:46 -0800 | Subject | Re: [PATCH v2 10/28] gup: Turn hpage_pincount_sub() into page_pincount_sub() | From | John Hubbard <> |
| |
On 1/9/22 20:23, Matthew Wilcox (Oracle) wrote: > Remove an unnecessary VM_BUG_ON by handling pages both with and without > a pincount field in page_pincount_sub(). > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/gup.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 3ed9907f3c8d..aed48de3912e 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -48,12 +48,15 @@ static void page_pincount_add(struct page *page, int refs) > page_ref_add(page, refs * (GUP_PIN_COUNTING_BIAS - 1)); > } > > -static void hpage_pincount_sub(struct page *page, int refs)
OK, this is a correct transformation. But it does make the function, as small as it is, rather hard to understand. It is now factored out at a strange place, as evidenced in the difficulty in documenting it. Here's my best attempt so far at documenting it here:
/* * Subtract @refs from the compound_pincount, if applicable. Otherwise, do * nothing. And in all cases, return the number of pages that should be passed * to put_page_refs(). */
...which I think is worth adding, if we can't find a better factoring.
Either way, it's correct, so:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks, -- John Hubbard NVIDIA
> +static int page_pincount_sub(struct page *page, int refs) > { > - VM_BUG_ON_PAGE(!hpage_pincount_available(page), page); > VM_BUG_ON_PAGE(page != compound_head(page), page); > > - atomic_sub(refs, compound_pincount_ptr(page)); > + if (hpage_pincount_available(page)) > + atomic_sub(refs, compound_pincount_ptr(page)); > + else > + refs *= GUP_PIN_COUNTING_BIAS; > + return refs; > } > > /* Equivalent to calling put_page() @refs times. */ > @@ -177,11 +180,7 @@ static void put_compound_head(struct page *page, int refs, unsigned int flags) > if (flags & FOLL_PIN) { > mod_node_page_state(page_pgdat(page), NR_FOLL_PIN_RELEASED, > refs); > - > - if (hpage_pincount_available(page)) > - hpage_pincount_sub(page, refs); > - else > - refs *= GUP_PIN_COUNTING_BIAS; > + refs = page_pincount_sub(page, refs); > } > > put_page_refs(page, refs);
| |