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.
