Re: [PATCH 1/5] mm: Introduce mm_struct.has_pinned

[John Hubbard]

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