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

[Alex Vainman <alexonlists-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

Hi Roland,

Thanks for the review.
I would break the patch as you requested and make the changes 
according to your comments.
Currently I am working on another assignment, so I would send
you the fixed patches till the beginning of the next week.

Thanks,
AlexV

Roland Dreier Wrote:
> 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
> 


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