Re: [PATCH hmm 2/6] mm/hmm: return the fault type from hmm_pte_need_fault()

[Jason Gunthorpe <jgg@mellanox.com>]

On Sat, Mar 21, 2020 at 09:37:26AM +0100, Christoph Hellwig wrote:
> 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?

Yes, OK, the existing names are pretty generic
 
> >  	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.

Techincally yes, but gcc demands them:

mm/hmm.c:146:22: warning: suggest parentheses around comparison in operand of '|' [-Wparentheses]
   if (required_fault == NEED_FAULT | NEED_WRITE_FAULT)
       ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~

Probably because | vs || is a common confusion?

Actually this whole NEED_FAULT | WRITE_FAULT thing is silly, I'm going
to define NEED_WRITE_FAULT == NEED_FAULT | (1<<1) and add a
NEED_ALL_BITS to make this clear what this test is for (early loop
exit once there is no possible change to required_fault).

> > @@ -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:

This is more readable, I reworked the comment too

Jason