Hi Shoaib,
Good to see you set out a patchset ;-)
On Tue, Jan 02, 2018 at 02:49:25PM -0800, Rao Shoaib wrote:
>
>
> On 01/02/2018 02:23 PM, Matthew Wilcox wrote:
> > On Tue, Jan 02, 2018 at 12:11:37PM -0800, rao.shoaib@xxxxxxxxxx wrote:
> > > -#define kfree_rcu(ptr, rcu_head) \
> > > - __kfree_rcu(&((ptr)->rcu_head), offsetof(typeof(*(ptr)), rcu_head))
> > > +#define kfree_rcu(ptr, rcu_head_name) \
> > > + do { \
> > > + typeof(ptr) __ptr = ptr; \
> > > + unsigned long __off = offsetof(typeof(*(__ptr)), \
> > > + rcu_head_name); \
> > > + struct rcu_head *__rptr = (void *)__ptr + __off; \
> > > + __kfree_rcu(__rptr, __off); \
> > > + } while (0)
> > I feel like you're trying to help people understand the code better,
> > but using longer names can really work against that. Reverting to
> > calling the parameter 'rcu_head' lets you not split the line:
> I think it is a matter of preference, what is the issue with line splitting
> ?
> Coming from a background other than Linux I find it very annoying that Linux
> allows variables names that are meaning less. Linux does not even enforce
> adding a prefix for structure members, so trying to find out where a member
> is used or set is impossible using cscope.
> I can not change the Linux requirements so I will go ahead and make the
> change in the next rev.
>
> >
> > +#define kfree_rcu(ptr, rcu_head) \
> > + do { \
> > + typeof(ptr) __ptr = ptr; \
> > + unsigned long __off = offsetof(typeof(*(__ptr)), rcu_head); \
> > + struct rcu_head *__rptr = (void *)__ptr + __off; \
> > + __kfree_rcu(__rptr, __off); \
> > + } while (0)
> >
> > Also, I don't understand why you're bothering to create __ptr here.
> > I understand the desire to not mention the same argument more than once,
> > but you have 'ptr' twice anyway.
> >
> > And it's good practice to enclose macro arguments in parentheses in case
> > the user has done something really tricksy like pass in "p + 1".
> >
> > In summary, I don't see anything fundamentally better in your rewrite
> > of kfree_rcu(). The previous version is more succinct, and to my
> > mind, easier to understand.
> I did not want to make thins change but it is required due to the new tests
> added for macro expansion where the same name as in the macro can not be
> used twice. It takes care of the 'p + 1' hazard that you refer to above.
> >
> > > +void call_rcu_lazy(struct rcu_head *head, rcu_callback_t func)
> > > +{
> > > + __call_rcu(head, func, &rcu_sched_state, -1, 1);
> > > +}
> > > -void kfree_call_rcu(struct rcu_head *head,
> > > - rcu_callback_t func)
> > > -{
> > > - __call_rcu(head, func, rcu_state_p, -1, 1);
> > > -}
> > You've silently changed this. Why? It might well be the right change,
> > but it at least merits mentioning in the changelog.
> This was to address a comment about me not changing the tiny implementation
> to be same as the tree implementation.
>
But you introduced a bug here, you should use rcu_state_p instead of
&rcu_sched_state as the third parameter for __call_rcu().
Please re-read:
https://marc.info/?l=linux-mm&m=151390529209639
, and there are other comments, which you still haven't resolved in this
version. You may want to write a better commit log to explain the
reasons of each modifcation and fix bugs or typos in your previous
version. That's how review process works ;-)
Regards,
Boqun
> Shoaib
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxx. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
Attachment:
signature.asc
Description: PGP signature
