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