Re: [PATCH 1/2] Move kfree_call_rcu() to slab_common.c

[Boqun Feng]

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