On 9/22/20 8:17 AM, Peter Xu wrote:
On Mon, Sep 21, 2020 at 04:53:38PM -0700, John Hubbard wrote:
On 9/21/20 2:17 PM, Peter Xu wrote:
(Commit message collected from Jason Gunthorpe)
Reduce the chance of false positive from page_maybe_dma_pinned() by keeping
Not yet, it doesn't. :) More:
track if the mm_struct has ever been used with pin_user_pages(). mm_structs
that have never been passed to pin_user_pages() cannot have a positive
page_maybe_dma_pinned() by definition. This allows cases that might drive up
the page ref_count to avoid any penalty from handling dma_pinned pages.
Due to complexities with unpining this trivial version is a permanent sticky
bit, future work will be needed to make this a counter.
How about this instead:
Subsequent patches intend to reduce the chance of false positives from
page_maybe_dma_pinned(), by also considering whether or not a page has
even been part of an mm struct that has ever had pin_user_pages*()
applied to any of its pages.
In order to allow that, provide a boolean value (even though it's not
implemented exactly as a boolean type) within the mm struct, that is
simply set once and never cleared. This will suffice for an early, rough
implementation that fixes a few problems.
Future work is planned, to provide a more sophisticated solution, likely
involving a counter, and *not* involving something that is set and never
cleared.
This looks good, thanks. Though I think Jason's version is good too (as long
as we remove the confusing sentence, that's the one starting with "mm_structs
that have never been passed... "). Before I drop Jason's version, I think I'd
better figure out what's the major thing we missed so that maybe we can add
another paragraph. E.g., "future work will be needed to make this a counter"
already means "involving a counter, and *not* involving something that is set
and never cleared" to me... Because otherwise it won't be called a counter..
That's just a bit of harmless redundancy, intended to help clarify where this
is going. But if the redundancy isn't actually helping, you could simply
truncate it to the first half of the sentence, like this:
"Future work is planned, to provide a more sophisticated solution, likely
involving a counter."
thanks,
--
John Hubbard
NVIDIA
