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 [email protected]
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
