On Fri, Mar 20, 2020 at 01:49:01PM -0300, Jason Gunthorpe wrote:
> +enum {
> + NEED_FAULT = 1 << 0,
> + NEED_WRITE_FAULT = 1 << 1,
> +};
Maybe add a HMM_ prefix?
> for (i = 0; i < npages; ++i) {
> + required_fault |=
> + hmm_pte_need_fault(hmm_vma_walk, pfns[i], cpu_flags);
> + if (required_fault == (NEED_FAULT | NEED_WRITE_FAULT))
> + return required_fault;
No need for the inner braces.
> @@ -532,17 +515,15 @@ static int hmm_vma_walk_test(unsigned long start, unsigned long end,
> */
> if ((vma->vm_flags & (VM_IO | VM_PFNMAP | VM_MIXEDMAP)) ||
> !(vma->vm_flags & VM_READ)) {
> - bool fault, write_fault;
> -
No that there is no need for local variables I'd invert the test and
return early:
if ((vma->vm_flags & VM_READ) &&
!(vma->vm_flags & (VM_IO | VM_PFNMAP | VM_MIXEDMAP))
return 0;
> /*
> * Check to see if a fault is requested for any page in the
> * range.
> */
> - hmm_range_need_fault(hmm_vma_walk, range->pfns +
> - ((start - range->start) >> PAGE_SHIFT),
> - (end - start) >> PAGE_SHIFT,
> - 0, &fault, &write_fault);
> - if (fault || write_fault)
> + if (hmm_range_need_fault(hmm_vma_walk,
> + range->pfns +
> + ((start - range->start) >>
> + PAGE_SHIFT),
> + (end - start) >> PAGE_SHIFT, 0))
Which should help to make this a little more readable..
