Re: [PATCH 04/15] mm: remove the pgmap field from struct hmm_vma_walk

[Christoph Hellwig]

On Wed, Aug 07, 2019 at 11:47:22AM -0700, Dan Williams wrote:
> > Unrelated to this patch, but what is the point of getting checking
> > that the pgmap exists for the page and then immediately releasing it?
> > This code has this pattern in several places.
> >
> > It feels racy
> 
> Agree, not sure what the intent is here. The only other reason call
> get_dev_pagemap() is to just check in general if the pfn is indeed
> owned by some ZONE_DEVICE instance, but if the intent is to make sure
> the device is still attached/enabled that check is invalidated at
> put_dev_pagemap().
> 
> If it's the former case, validating ZONE_DEVICE pfns, I imagine we can
> do something cheaper with a helper that is on the order of the same
> cost as pfn_valid(). I.e. replace PTE_DEVMAP with a mem_section flag
> or something similar.

The hmm literally never dereferences the pgmap, so validity checking is
the only explanation for it.

> > +               /*
> > +                * We do put_dev_pagemap() here so that we can leverage
> > +                * get_dev_pagemap() optimization which will not re-take a
> > +                * reference on a pgmap if we already have one.
> > +                */
> > +               if (hmm_vma_walk->pgmap)
> > +                       put_dev_pagemap(hmm_vma_walk->pgmap);
> > +
> 
> Seems ok, but only if the caller is guaranteeing that the range does
> not span outside of a single pagemap instance. If that guarantee is
> met why not just have the caller pass in a pinned pagemap? If that
> guarantee is not met, then I think we're back to your race concern.

It iterates over multiple ptes in a non-huge pmd.  Is there any kind of
limitations on different pgmap instances inside a pmd?  I can't think
of one, so this might actually be a bug.