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