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;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Wei Yang" <richard.weiyang@gmail.com>
Cc:     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()
In-reply-to: <20211007003058.uj35ekwibbrxqzku@master>
References: <20211006152100.17795-1-richard.weiyang@gmail.com>,
 <163355497171.31063.8329134032738647570@noble.neil.brown.name>,
 <20211007003058.uj35ekwibbrxqzku@master>
Date:   Thu, 07 Oct 2021 11:50:22 +1100
Message-id: <163356782206.31063.3710696596883334978@noble.neil.brown.name>
Precedence: bulk

On Thu, 07 Oct 2021, Wei Yang wrote:
>=20
> Here is a draft patch based on you comment:
>=20
> 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 *nod=
e)
>  	hlist_del_init_rcu(node);
>  }
> =20
> +/**
> + * Note: the following three hash_for_each[_xxx] helpers introduce a new l=
oop
> + * 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 =3D=3D NUL=
L' test
> + * ensures that when the inner loop is broken, the outer loop will break t=
oo.
> + */
> +
>  /**
>   * hash_for_each - iterate over a hashtable
>   * @name: hashtable to iterate
>=20
>=20
> If you feel good, I would like to add=20
>=20
> 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>

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 =3D=3D 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) =3D 0, obj =3D NULL; obj =3D=3D 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) =3D 0, obj =3D NULL; obj =3D=3D 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) =3D 0, obj =3D NULL; obj =3D=3D NULL && (bkt) < HASH_SIZE(name);\