linux-kernel - Re: [PATCH] rculist: move hlist_del_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Fri, 15 Jan 2016 17:59:53 -0800
From:	Josh Triplett <josh@...htriplett.org>
To:	paulmck@...ux.vnet.ibm.com, Geliang Tang <geliangtang@....com>
CC:	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rculist: move hlist_del_init_rcu to a proper place

On January 15, 2016 4:50:01 PM PST, "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com> 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@....com>
>
>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
>> 
>>