Re: [PATCH v2 1/3] mm/hmm: make full use of walk_page_range()

[Ralph Campbell <rcampbell@nvidia.com>]

On 10/21/19 11:32 AM, Jason Gunthorpe wrote:
> On Tue, Oct 15, 2019 at 01:48:12PM -0700, Ralph Campbell wrote:
> 
>> +static bool hmm_range_needs_fault(unsigned long addr, unsigned long end,
>> +				  const struct hmm_vma_walk *hmm_vma_walk)
> 
> This has a very similar name to hmm_range_need_fault(), and seems like
> it does the same thing?

The two functions are very similar but not identical.
I guess I could resolve the differences and use one function.

>> +static int hmm_vma_walk_test(unsigned long start, unsigned long end,
>> +			     struct mm_walk *walk)
>> +{
>> +	struct hmm_vma_walk *hmm_vma_walk = walk->private;
>> +	struct hmm_range *range = hmm_vma_walk->range;
>> +	struct vm_area_struct *vma = walk->vma;
>> +
>> +	/* If range is no longer valid, force retry. */
>> +	if (!range->valid)
>> +		return -EBUSY;
>> +
>> +	/*
>> +	 * Skip vma ranges that don't have struct page backing them or
>> +	 * map I/O devices directly.
>> +	 */
>> +	if (vma->vm_flags & (VM_IO | VM_PFNMAP | VM_MIXEDMAP))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * If the vma does not allow read access, then assume that it does not
>> +	 * allow write access either. HMM does not support architectures
>> +	 * that allow write without read.
>> +	 */
>> +	if (!(vma->vm_flags & VM_READ)) {
>> +		/*
>> +		 * Check to see if a fault is requested for any page in the
>> +		 * range.
>> +		 */
>> +		if (hmm_range_needs_fault(start, end, hmm_vma_walk))
>> +			return -EFAULT;
> 
> Is this change to call hmm_range_needs_fault another bug fix?
> 
> Jason

Yes. If the HMM_FAULT_SNAPSHOT is specified, there shouldn't be any
error return code. If it is not specified, then the range->pfns[] array,
on input, holds flags indicating which pages the driver wants populated
if the page is not already present. The hmm_range_needs_fault() checks
for this and hmm_vma_walk_test() returns -EFAULT.

I guess I could include this in the change log.