Re: [PATCH v2 hmm 4/9] mm/hmm: remove HMM_FAULT_SNAPSHOT
On Fri, Mar 27, 2020 at 05:00:16PM -0300, Jason Gunthorpe wrote: > From: Jason Gunthorpe > > Now that flags are handled on a fine-grained per-page basis this global > flag is redundant and has a confusing overlap with the pfn_flags_mask and > default_flags. > > Normalize the HMM_FAULT_SNAPSHOT behavior into one place. Callers needing > the SNAPSHOT behavior should set a pfn_flags_mask and default_flags that > always results in a cleared HMM_PFN_VALID. Then no pages will be faulted, > and HMM_FAULT_SNAPSHOT is not a special flow that overrides the masking > mechanism. > > As this is the last flag, also remove the flags argument. If future flags > are needed they can be part of the struct hmm_range function arguments. > > Signed-off-by: Jason Gunthorpe > --- > Documentation/vm/hmm.rst| 12 +--- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 2 +- > drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +- > include/linux/hmm.h | 5 + > mm/hmm.c| 17 + > 5 files changed, 17 insertions(+), 21 deletions(-) Looks good, Reviewed-by: Christoph Hellwig ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 4/9] mm/hmm: remove HMM_FAULT_SNAPSHOT
On Tue, Mar 24, 2020 at 08:33:39AM +0100, Christoph Hellwig wrote:
> >
> > +/*
> > + * If the valid flag is masked off, and default_flags doesn't set valid,
> > then
> > + * hmm_pte_need_fault() always returns 0.
> > + */
> > +static bool hmm_can_fault(struct hmm_range *range)
> > +{
> > + return ((range->flags[HMM_PFN_VALID] & range->pfn_flags_mask) |
> > + range->default_flags) &
> > + range->flags[HMM_PFN_VALID];
> > +}
>
> So my idea behind the helper was to turn this into something readable :)
Well, it does help to give the expression a name :)
> E.g.
>
> /*
> * We only need to fault if either the default mask requires to fault all
> * pages, or at least the mask allows for individual pages to be faulted.
> */
> static bool hmm_can_fault(struct hmm_range *range)
> {
> return ((range->default_flags | range->pfn_flags_mask) &
> range->flags[HMM_PFN_VALID]);
> }
Okay, I find this as understandable and it is less cluttered. I think
the comment is good enough now.
Can we concur on this then:
static unsigned int
hmm_range_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
const uint64_t *pfns, unsigned long npages,
uint64_t cpu_flags)
{
+ struct hmm_range *range = hmm_vma_walk->range;
unsigned int required_fault = 0;
unsigned long i;
- if (hmm_vma_walk->flags & HMM_FAULT_SNAPSHOT)
+ /*
+* If the default flags do not request to fault pages, and the mask does
+* not allow for individual pages to be faulted, then
+* hmm_pte_need_fault() will always return 0.
+*/
+ if (!((range->default_flags | range->pfn_flags_mask) &
+ range->flags[HMM_PFN_VALID]))
return 0;
I think everything else is sorted now, so if yes I'll send this as v3.
Thanks,
Jason
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH v2 hmm 4/9] mm/hmm: remove HMM_FAULT_SNAPSHOT
>
> +/*
> + * If the valid flag is masked off, and default_flags doesn't set valid, then
> + * hmm_pte_need_fault() always returns 0.
> + */
> +static bool hmm_can_fault(struct hmm_range *range)
> +{
> + return ((range->flags[HMM_PFN_VALID] & range->pfn_flags_mask) |
> + range->default_flags) &
> +range->flags[HMM_PFN_VALID];
> +}
So my idea behind the helper was to turn this into something readable :)
E.g.
/*
* We only need to fault if either the default mask requires to fault all
* pages, or at least the mask allows for individual pages to be faulted.
*/
static bool hmm_can_fault(struct hmm_range *range)
{
return ((range->default_flags | range->pfn_flags_mask) &
range->flags[HMM_PFN_VALID]);
}
In fact now that I managed to destill it down to this I'm not even
sure we really even need the helper, although the comment really helps.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
