Re: [PATCH] rculist: move hlist_del_init_rcu to a proper place

[Josh Triplett]

On January 15, 2016 4:50:01 PM PST, "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
>On Tue, Jan 12, 2016 at 11:08:30PM +0800, Geliang Tang wrote:
>> hlist_del_init_rcu() should not behind list_del_rcu(), should behind
>> hlist_del_rcu().
>> 
>> Signed-off-by: Geliang Tang <geliangtang@xxxxxxx>
>
>Can't say I am all that excited by this one.  Anyone on CC willing to
>speak up for it?

In the absence of a clear explanation and benefit, I don't think this kind of code motion makes sense.

>
>> ---
>>  include/linux/rculist.h | 56
>++++++++++++++++++++++++-------------------------
>>  1 file changed, 28 insertions(+), 28 deletions(-)
>> 
>> diff --git a/include/linux/rculist.h b/include/linux/rculist.h
>> index 14ec165..99bed04 100644
>> --- a/include/linux/rculist.h
>> +++ b/include/linux/rculist.h
>> @@ -133,34 +133,6 @@ static inline void list_del_rcu(struct list_head
>*entry)
>>  }
>> 
>>  /**
>> - * hlist_del_init_rcu - deletes entry from hash list with
>re-initialization
>> - * @n: the element to delete from the hash list.
>> - *
>> - * Note: list_unhashed() on the node return true after this. It is
>> - * useful for RCU based read lockfree traversal if the writer side
>> - * must know if the list entry is still hashed or already unhashed.
>> - *
>> - * In particular, it means that we can not poison the forward
>pointers
>> - * that may still be used for walking the hash list and we can only
>> - * zero the pprev pointer so list_unhashed() will return true after
>> - * this.
>> - *
>> - * The caller must take whatever precautions are necessary (such as
>> - * holding appropriate locks) to avoid racing with another
>> - * list-mutation primitive, such as hlist_add_head_rcu() or
>> - * hlist_del_rcu(), running on this same list.  However, it is
>> - * perfectly legal to run concurrently with the _rcu list-traversal
>> - * primitives, such as hlist_for_each_entry_rcu().
>> - */
>> -static inline void hlist_del_init_rcu(struct hlist_node *n)
>> -{
>> -	if (!hlist_unhashed(n)) {
>> -		__hlist_del(n);
>> -		n->pprev = NULL;
>> -	}
>> -}
>> -
>> -/**
>>   * list_replace_rcu - replace old entry by new one
>>   * @old : the element to be replaced
>>   * @new : the new element to insert
>> @@ -409,6 +381,34 @@ static inline void hlist_del_rcu(struct
>hlist_node *n)
>>  }
>> 
>>  /**
>> + * hlist_del_init_rcu - deletes entry from hash list with
>re-initialization
>> + * @n: the element to delete from the hash list.
>> + *
>> + * Note: list_unhashed() on the node return true after this. It is
>> + * useful for RCU based read lockfree traversal if the writer side
>> + * must know if the list entry is still hashed or already unhashed.
>> + *
>> + * In particular, it means that we can not poison the forward
>pointers
>> + * that may still be used for walking the hash list and we can only
>> + * zero the pprev pointer so list_unhashed() will return true after
>> + * this.
>> + *
>> + * The caller must take whatever precautions are necessary (such as
>> + * holding appropriate locks) to avoid racing with another
>> + * list-mutation primitive, such as hlist_add_head_rcu() or
>> + * hlist_del_rcu(), running on this same list.  However, it is
>> + * perfectly legal to run concurrently with the _rcu list-traversal
>> + * primitives, such as hlist_for_each_entry_rcu().
>> + */
>> +static inline void hlist_del_init_rcu(struct hlist_node *n)
>> +{
>> +	if (!hlist_unhashed(n)) {
>> +		__hlist_del(n);
>> +		n->pprev = NULL;
>> +	}
>> +}
>> +
>> +/**
>>   * hlist_replace_rcu - replace old entry by new one
>>   * @old : the element to be replaced
>>   * @new : the new element to insert
>> -- 
>> 2.5.0
>> 
>>