From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex Vainman Subject: Re: [PATCH] libibverbs: Undo changes in memory range tree when madvise() fails Date: Sun, 17 Jan 2010 10:15:41 +0200 Message-ID: <4B52C72D.1000708@gmail.com> References: <4B12A679.3000800@gmail.com> Reply-To: alexv-smomgflXvOZWk0Htik3J/w@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Roland Dreier Cc: alexv-smomgflXvOZWk0Htik3J/w@public.gmane.org, roland , linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-rdma@vger.kernel.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