Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 7 Oct 2021 23:40:51 +0000
From:   Wei Yang <richard.weiyang@gmail.com>
To:     NeilBrown <neilb@suse.de>
Cc:     Wei Yang <richard.weiyang@gmail.com>, kuba@kernel.org,
        gregkh@linuxfoundation.org, mojha@codeaurora.org, jkosina@suse.cz,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] hashtable: remove a redundant check in
 hash_for_each_xxx()
Message-ID: <20211007234051.km5w3o4err7vrpqm@master>
Reply-To: Wei Yang <richard.weiyang@gmail.com>
References: <20211006152100.17795-1-richard.weiyang@gmail.com>
 <163355497171.31063.8329134032738647570@noble.neil.brown.name>
 <20211007003058.uj35ekwibbrxqzku@master>
 <163356782206.31063.3710696596883334978@noble.neil.brown.name>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <163356782206.31063.3710696596883334978@noble.neil.brown.name>
User-Agent: NeoMutt/20170113 (1.7.2)
Precedence: bulk

On Thu, Oct 07, 2021 at 11:50:22AM +1100, NeilBrown wrote:
>On Thu, 07 Oct 2021, Wei Yang wrote:
>> 
>> Here is a draft patch based on you comment:
>> 
>> diff --git a/include/linux/hashtable.h b/include/linux/hashtable.h
>> index f6c666730b8c..2ff4cb5e6a22 100644
>> --- a/include/linux/hashtable.h
>> +++ b/include/linux/hashtable.h
>> @@ -116,6 +116,13 @@ static inline void hash_del_rcu(struct hlist_node *node)
>>  	hlist_del_init_rcu(node);
>>  }
>>  
>> +/**
>> + * Note: the following three hash_for_each[_xxx] helpers introduce a new loop
>> + * command that is constructed from 2 nested loops. It is safe to 'break' out
>> + * of this loop even though it is a two nested loops.  The 'obj == NULL' test
>> + * ensures that when the inner loop is broken, the outer loop will break too.
>> + */
>> +
>>  /**
>>   * hash_for_each - iterate over a hashtable
>>   * @name: hashtable to iterate
>> 
>> 
>> If you feel good, I would like to add 
>> 
>> Sugguested-by: NeilBrown <neilb@suse.de>
>
>That's definitely an improvement.
>
>I'd probably put it in the kernel-doc comment for hash_for_each,
>then in the other two just put the "it is safe" bit.  Something like
>the following.  But I don't feel strongly about it.
>I'm happy to say
>  Reviewed-by: NeilBrown <neilb@suse.de>
>

Thanks for your detailed instruction :-)

>for your patch.
>
>Thanks,
>NeilBrown
>
>
>diff --git a/include/linux/hashtable.h b/include/linux/hashtable.h
>index f6c666730b8c..61db940c9501 100644
>--- a/include/linux/hashtable.h
>+++ b/include/linux/hashtable.h
>@@ -122,6 +122,10 @@ static inline void hash_del_rcu(struct hlist_node *node)
>  * @bkt: integer to use as bucket loop cursor
>  * @obj: the type * to use as a loop cursor for each entry
>  * @member: the name of the hlist_node within the struct
>+ *
>+ * Note: It is safe to 'break' out of this loop even though it is a two
>+ * nested loops.  The 'obj == NULL' test ensures that when the inner loop
>+ * is broken, the outer loop will break too.
>  */
> #define hash_for_each(name, bkt, obj, member)				\
> 	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
>@@ -134,6 +138,8 @@ static inline void hash_del_rcu(struct hlist_node *node)
>  * @bkt: integer to use as bucket loop cursor
>  * @obj: the type * to use as a loop cursor for each entry
>  * @member: the name of the hlist_node within the struct
>+ *
>+ * It is safe to 'break' out of this loop.
>  */
> #define hash_for_each_rcu(name, bkt, obj, member)			\
> 	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\
>@@ -148,6 +154,8 @@ static inline void hash_del_rcu(struct hlist_node *node)
>  * @tmp: a &struct hlist_node used for temporary storage
>  * @obj: the type * to use as a loop cursor for each entry
>  * @member: the name of the hlist_node within the struct
>+ *
>+ * It is safe to 'break' out of this loop.
>  */
> #define hash_for_each_safe(name, bkt, tmp, obj, member)			\
> 	for ((bkt) = 0, obj = NULL; obj == NULL && (bkt) < HASH_SIZE(name);\

-- 
Wei Yang
Help you, Help me