On Tue, Mar 24, 2020 at 08:37:46AM +0100, Christoph Hellwig wrote:

On Mon, Mar 23, 2020 at 10:14:55PM -0300, Jason Gunthorpe wrote:

...

if (pte_none(pte)) { required_fault = hmm_pte_need_fault(hmm_vma_walk, orig_pfn, 0); if (required_fault) goto fault;

• *pfn = range->values[HMM_PFN_NONE];
  

  return 0; }

@@ -274,8 +274,10 @@ static int hmm_vma_handle_pte(struct mm_walk *walk, unsigned long addr, }

required_fault = hmm_pte_need_fault(hmm_vma_walk, orig_pfn, 0);

• if (!required_fault)
  

• if (!required_fault) {
  

• 	*pfn = range->values[HMM_PFN_NONE];
  return 0;
  

• }
  

Maybe throw in a goto hole to consolidaste the set PFN and return 0 cases?

Then we have goto fault and goto none both ending in returns. I generally prefer the goto labels to have a single return

The pte_unmap() before faulting makes this routine twisty and I haven't thought of a good way to untwist it

Thanks, Jason