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
