On Wed, Jan 29, 2020 at 10:44:50PM -0800, John Hubbard wrote: > On 1/29/20 5:51 AM, Kirill A. Shutemov wrote: > > > +/** > > > + * page_dma_pinned() - report if a page is pinned for DMA. > > > + * > > > + * This function checks if a page has been pinned via a call to > > > + * pin_user_pages*(). > > > + * > > > + * For non-huge pages, the return value is partially fuzzy: false is not fuzzy, > > > + * because it means "definitely not pinned for DMA", but true means "probably > > > + * pinned for DMA, but possibly a false positive due to having at least > > > + * GUP_PIN_COUNTING_BIAS worth of normal page references". > > > + * > > > + * False positives are OK, because: a) it's unlikely for a page to get that many > > > + * refcounts, and b) all the callers of this routine are expected to be able to > > > + * deal gracefully with a false positive. > > > > I wounder if we should reverse the logic and name -- page_not_dma_pinned() > > or something -- too emphasise that we can only know for sure when the page > > is not pinned, but not necessary when it is. > > > > This is an interesting point. I agree that it's worth maybe adding information > into the function name, but I'd like to keep the bool "positive", because there > will be a number of callers that ask "if it is possibly dma-pinned, then ...". > So combining that, how about this function name: > > page_maybe_dma_pinned() > > , which I could live with and I think would be acceptable? I would still prefer the negative version, but up to you. > > I see opportunity to split the patch further. > > > ah, OK. I wasn't sure how far to go before I get tagged for "excessive > patch splitting"! haha. Anyway, are you suggesting to put the > page_ref_sub_return() routine into it's own patch? > > Another thing to split out would be adding the flags to the remaining > functions, such as undo_dev_pagemap(). That burns quite a few lines of > diff. Anything else to split out? Nothing I see immediately. > > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > > index 0a55dec68925..b1079aaa6f24 100644 > > > --- a/mm/huge_memory.c > > > +++ b/mm/huge_memory.c > > > @@ -958,6 +958,11 @@ struct page *follow_devmap_pmd(struct vm_area_struct *vma, unsigned long addr, > > > */ > > > WARN_ONCE(flags & FOLL_COW, "mm: In follow_devmap_pmd with FOLL_COW set"); > > > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */ > > > + if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) == > > > + (FOLL_PIN | FOLL_GET))) > > > > Too many parentheses. > > > OK, I'll remove at least one. :) I see two. -- Kirill A. Shutemov
