lkml.org 
[lkml]   [2018]   [Jan]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 1/2] Move kfree_call_rcu() to slab_common.c
From
Date


On 01/02/2018 02:23 PM, Matthew Wilcox wrote:
> On Tue, Jan 02, 2018 at 12:11:37PM -0800, rao.shoaib@oracle.com 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.

Shoaib
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

\
 
 \ /
  Last update: 2018-01-02 23:51    [W:0.036 / U:1.668 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site