Re: [PATCH] libibverbs: Undo changes in memory range tree when madvise() fails

[Roland Dreier <rdreier-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>]

This looks pretty good overall -- thanks for taking this on, I've been
meaning to fix this for a long time, but never got around to it.

However, I'm having a bit of a hard time following the cleanups vs. real
changes in this patch -- could you break it up into (at least) two
steps?  ie one patch that just factors code out into merge_ranges()
etc. and then a second patch that makes the fixes for handling madvise
failures?  I think that would make the second patch much easier to
understand.  And if you can split the second patch further, that might
make it even easier to review.

Also some specific comments:

 > +static struct ibv_mem_node *merge_ranges(struct ibv_mem_node *node,
 > +					 struct ibv_mem_node *prev)
 > +{
 > +	struct ibv_mem_node *new_node = NULL;
 > +
 > +	prev->end = node->end;
 > +	prev->refcnt = node->refcnt;
 > +	__mm_remove(node);
 > +	new_node = prev;
 > +
 > +	return new_node;
 > +}

why do you have the new_node variable at all?  why can't this just be
written as:

+static struct ibv_mem_node *merge_ranges(struct ibv_mem_node *node,
+					 struct ibv_mem_node *prev)
+{
+	prev->end = node->end;
+	prev->refcnt = node->refcnt;
+	__mm_remove(node);
+	return prev;
+}

 > +	else{

should be a space after the "else" -- please make sure all the
trivial formatting is OK.

 > +	/*
 > +	 * This condition can be true only if we merged node which begins at start
 > +	 * and ends at node->end with previous node which begins at node->start
 > +	 * and ends at start - 1
 > +	 */

these and a few other comments make pretty long lines for no really good
reason -- please try to end comments before, say, column 75.

 > +						 uintptr_t *p_end,
 > +						 int *p_inc,
 > +						 int *p_advice)

I prefer not to use hungarian notation for variable names.

 > +						node = prepare_to_roll_back(node, start, &end, &inc, &advice);

I'm OK with lines over 80 characters, but 110 is a bit too
much... please try to split the function up a bit so this isn't indented
by 6 tabs.  (This over-long line is a symptom of the fact that things
are too deeply nested here)

 > @@ -568,3 +665,5 @@ int ibv_dofork_range(void *base, size_t size)
 >  		return 0;
 >  	}
 >  }
 > +
 > +

extra chunk, just get rid of this change.

Thanks,
  Roland

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html