Re: [PATCH v2 hmm 4/9] mm/hmm: remove HMM_FAULT_SNAPSHOT

From: Jason Gunthorpe <jgg@mellanox.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Philip Yang <Philip.Yang@amd.com>,
	Ralph Campbell <rcampbell@nvidia.com>,
	John Hubbard <jhubbard@nvidia.com>,
	Felix.Kuehling@amd.com, dri-devel@lists.freedesktop.org,
	linux-mm@kvack.org, Jerome Glisse <jglisse@redhat.com>,
	amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 hmm 4/9] mm/hmm: remove HMM_FAULT_SNAPSHOT
Date: Tue, 24 Mar 2020 16:31:16 -0300	[thread overview]
Message-ID: <20200324193116.GO13183@mellanox.com> (raw)
In-Reply-To: <20200324073339.GC23447@lst.de>

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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx