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
