On Tue, Sep 22, 2020 at 11:02:03AM -0700, John Hubbard wrote: > 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." Will do. Thanks. -- Peter Xu
