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.