[PATCH] netdevsim: fib: Make use of rhashtable_iter

[PATCH] netdevsim: fib: Make use of rhashtable_iter

[Cai Huoqing]

Iterating 'fib_rt_ht' by rhashtable_walk_next and rhashtable_iter directly
instead of using list_for_each, because each entry of fib_rt_ht can be
found by rhashtable API. And remove fib_rt_list.

Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
---
 drivers/net/netdevsim/fib.c | 37 ++++++++++++++++++-------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
index a1f91ff8ec56..1a50c8e14665 100644
--- a/drivers/net/netdevsim/fib.c
+++ b/drivers/net/netdevsim/fib.c
@@ -48,7 +48,6 @@ struct nsim_fib_data {
 	struct nsim_per_fib_data ipv6;
 	struct nsim_fib_entry nexthops;
 	struct rhashtable fib_rt_ht;
-	struct list_head fib_rt_list;
 	struct mutex fib_lock; /* Protects FIB HT and list */
 	struct notifier_block nexthop_nb;
 	struct rhashtable nexthop_ht;
@@ -75,7 +74,6 @@ struct nsim_fib_rt_key {
 struct nsim_fib_rt {
 	struct nsim_fib_rt_key key;
 	struct rhash_head ht_node;
-	struct list_head list;	/* Member of fib_rt_list */
 };
 
 struct nsim_fib4_rt {
@@ -247,12 +245,6 @@ static void nsim_fib_rt_init(struct nsim_fib_data *data,
 	fib_rt->key.prefix_len = prefix_len;
 	fib_rt->key.family = family;
 	fib_rt->key.tb_id = tb_id;
-	list_add(&fib_rt->list, &data->fib_rt_list);
-}
-
-static void nsim_fib_rt_fini(struct nsim_fib_rt *fib_rt)
-{
-	list_del(&fib_rt->list);
 }
 
 static struct nsim_fib_rt *nsim_fib_rt_lookup(struct rhashtable *fib_rt_ht,
@@ -295,7 +287,6 @@ nsim_fib4_rt_create(struct nsim_fib_data *data,
 static void nsim_fib4_rt_destroy(struct nsim_fib4_rt *fib4_rt)
 {
 	fib_info_put(fib4_rt->fi);
-	nsim_fib_rt_fini(&fib4_rt->common);
 	kfree(fib4_rt);
 }
 
@@ -570,7 +561,6 @@ nsim_fib6_rt_create(struct nsim_fib_data *data,
 	for (i--; i >= 0; i--) {
 		nsim_fib6_rt_nh_del(fib6_rt, rt_arr[i]);
 	}
-	nsim_fib_rt_fini(&fib6_rt->common);
 	kfree(fib6_rt);
 	return ERR_PTR(err);
 }
@@ -582,7 +572,6 @@ static void nsim_fib6_rt_destroy(struct nsim_fib6_rt *fib6_rt)
 	list_for_each_entry_safe(iter, tmp, &fib6_rt->nh_list, list)
 		nsim_fib6_rt_nh_del(fib6_rt, iter->rt);
 	WARN_ON_ONCE(!list_empty(&fib6_rt->nh_list));
-	nsim_fib_rt_fini(&fib6_rt->common);
 	kfree(fib6_rt);
 }
 
@@ -1091,7 +1080,9 @@ static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
 {
 	struct nsim_fib_data *data = container_of(nb, struct nsim_fib_data,
 						  fib_nb);
-	struct nsim_fib_rt *fib_rt, *fib_rt_tmp;
+	struct nsim_fib_rt *fib_rt;
+	struct rhashtable_iter hti;
+	struct rhash_head *pos;
 
 	/* Flush the work to make sure there is no race with notifications. */
 	flush_work(&data->fib_event_work);
@@ -1099,9 +1090,12 @@ static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
 	/* The notifier block is still not registered, so we do not need to
 	 * take any locks here.
 	 */
-	list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
-		rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
+	rhashtable_walk_enter(&data->fib_rt_ht, &hti);
+	rhashtable_walk_start(&hti);
+	while ((pos = rhashtable_walk_next(&hti))) {
+		rhashtable_remove_fast(&data->fib_rt_ht, hti.p,
 				       nsim_fib_rt_ht_params);
+		fib_rt = rhashtable_walk_peek(&hti);
 		nsim_fib_rt_free(fib_rt, data);
 	}
 
@@ -1501,17 +1495,24 @@ static void nsim_fib_flush_work(struct work_struct *work)
 {
 	struct nsim_fib_data *data = container_of(work, struct nsim_fib_data,
 						  fib_flush_work);
-	struct nsim_fib_rt *fib_rt, *fib_rt_tmp;
+	struct nsim_fib_rt *fib_rt;
+	struct rhashtable_iter hti;
+	struct rhash_head *pos;
+
 
 	/* Process pending work. */
 	flush_work(&data->fib_event_work);
 
 	mutex_lock(&data->fib_lock);
-	list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
-		rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
+	rhashtable_walk_enter(&data->fib_rt_ht, &hti);
+	rhashtable_walk_start(&hti);
+	while ((pos = rhashtable_walk_next(&hti))) {
+		rhashtable_remove_fast(&data->fib_rt_ht, hti.p,
 				       nsim_fib_rt_ht_params);
+		fib_rt = rhashtable_walk_peek(&hti);
 		nsim_fib_rt_free(fib_rt, data);
 	}
+
 	mutex_unlock(&data->fib_lock);
 }
 
@@ -1571,7 +1572,6 @@ struct nsim_fib_data *nsim_fib_create(struct devlink *devlink,
 		goto err_debugfs_exit;
 
 	mutex_init(&data->fib_lock);
-	INIT_LIST_HEAD(&data->fib_rt_list);
 	err = rhashtable_init(&data->fib_rt_ht, &nsim_fib_rt_ht_params);
 	if (err)
 		goto err_rhashtable_nexthop_destroy;
@@ -1661,7 +1661,6 @@ void nsim_fib_destroy(struct devlink *devlink, struct nsim_fib_data *data)
 	rhashtable_free_and_destroy(&data->nexthop_ht, nsim_nexthop_free,
 				    data);
 	WARN_ON_ONCE(!list_empty(&data->fib_event_queue));
-	WARN_ON_ONCE(!list_empty(&data->fib_rt_list));
 	mutex_destroy(&data->fib_lock);
 	mutex_destroy(&data->nh_lock);
 	nsim_fib_debugfs_exit(data);
-- 
2.34.1

Re: [PATCH] netdevsim: fib: Make use of rhashtable_iter

[Jakub Kicinski]

On Tue, 25 Apr 2023 22:45:55 +0800 Cai Huoqing wrote:
> Iterating 'fib_rt_ht' by rhashtable_walk_next and rhashtable_iter directly
> instead of using list_for_each, because each entry of fib_rt_ht can be
> found by rhashtable API. And remove fib_rt_list.

## Form letter - net-next-closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after May 8th.

RFC patches sent for review only are obviously welcome at any time.
-- 
pw-bot: defer

Re: [PATCH] netdevsim: fib: Make use of rhashtable_iter

[Herbert Xu]

Cai Huoqing <cai.huoqing@linux.dev> wrote:
> Iterating 'fib_rt_ht' by rhashtable_walk_next and rhashtable_iter directly
> instead of using list_for_each, because each entry of fib_rt_ht can be
> found by rhashtable API. And remove fib_rt_list.
> 
> Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
> ---
> drivers/net/netdevsim/fib.c | 37 ++++++++++++++++++-------------------
> 1 file changed, 18 insertions(+), 19 deletions(-)

What is the rationale for this patch? Are you trying to save
memory?

> @@ -1099,9 +1090,12 @@ static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
>        /* The notifier block is still not registered, so we do not need to
>         * take any locks here.
>         */
> -       list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
> -               rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
> +       rhashtable_walk_enter(&data->fib_rt_ht, &hti);
> +       rhashtable_walk_start(&hti);
> +       while ((pos = rhashtable_walk_next(&hti))) {
> +               rhashtable_remove_fast(&data->fib_rt_ht, hti.p,
>                                       nsim_fib_rt_ht_params);
> +               fib_rt = rhashtable_walk_peek(&hti);
>                nsim_fib_rt_free(fib_rt, data);
>        }

In general rhashtable walks are not stable.  You may miss entries
or see entries twice.  They should be avoided unless absolutely
necessary.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] netdevsim: fib: Make use of rhashtable_iter

[Cai Huoqing]

On 26 4月 23 17:37:49, Herbert Xu wrote:
> Cai Huoqing <cai.huoqing@linux.dev> wrote:
> > Iterating 'fib_rt_ht' by rhashtable_walk_next and rhashtable_iter directly
> > instead of using list_for_each, because each entry of fib_rt_ht can be
> > found by rhashtable API. And remove fib_rt_list.
> > 
> > Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
> > ---
> > drivers/net/netdevsim/fib.c | 37 ++++++++++++++++++-------------------
> > 1 file changed, 18 insertions(+), 19 deletions(-)
> 
> What is the rationale for this patch? Are you trying to save
> memory?
Hi 
Thanks for your reply,

I think not need to use two structs to link fib_rt node, 
fib_rt_list is redundant.

Thanks,
Cai-

> 
> > @@ -1099,9 +1090,12 @@ static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
> >        /* The notifier block is still not registered, so we do not need to
> >         * take any locks here.
> >         */
> > -       list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
> > -               rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
> > +       rhashtable_walk_enter(&data->fib_rt_ht, &hti);
> > +       rhashtable_walk_start(&hti);
> > +       while ((pos = rhashtable_walk_next(&hti))) {
> > +               rhashtable_remove_fast(&data->fib_rt_ht, hti.p,
> >                                       nsim_fib_rt_ht_params);
> > +               fib_rt = rhashtable_walk_peek(&hti);
> >                nsim_fib_rt_free(fib_rt, data);
> >        }
> 
> In general rhashtable walks are not stable.  You may miss entries
> or see entries twice.  They should be avoided unless absolutely
> necessary.
Agree, but how about using rhashtable_free_and_destroy here
instead of rhashtable_walk_next in this patch.

> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] netdevsim: fib: Make use of rhashtable_iter

[Ido Schimmel]

On Fri, Apr 28, 2023 at 11:11:55PM +0800, Cai Huoqing wrote:
> On 26 4月 23 17:37:49, Herbert Xu wrote:
> > Cai Huoqing <cai.huoqing@linux.dev> wrote:
> > > Iterating 'fib_rt_ht' by rhashtable_walk_next and rhashtable_iter directly
> > > instead of using list_for_each, because each entry of fib_rt_ht can be
> > > found by rhashtable API. And remove fib_rt_list.
> > > 
> > > Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
> > > ---
> > > drivers/net/netdevsim/fib.c | 37 ++++++++++++++++++-------------------
> > > 1 file changed, 18 insertions(+), 19 deletions(-)
> > 
> > What is the rationale for this patch? Are you trying to save
> > memory?
> Hi 
> Thanks for your reply,
> 
> I think not need to use two structs to link fib_rt node, 
> fib_rt_list is redundant.

There are cases where we want to iterate over the objects without
destroying the hashtable itself.

> 
> Thanks,
> Cai-
> 
> > 
> > > @@ -1099,9 +1090,12 @@ static void nsim_fib_dump_inconsistent(struct notifier_block *nb)
> > >        /* The notifier block is still not registered, so we do not need to
> > >         * take any locks here.
> > >         */
> > > -       list_for_each_entry_safe(fib_rt, fib_rt_tmp, &data->fib_rt_list, list) {
> > > -               rhashtable_remove_fast(&data->fib_rt_ht, &fib_rt->ht_node,
> > > +       rhashtable_walk_enter(&data->fib_rt_ht, &hti);
> > > +       rhashtable_walk_start(&hti);
> > > +       while ((pos = rhashtable_walk_next(&hti))) {
> > > +               rhashtable_remove_fast(&data->fib_rt_ht, hti.p,
> > >                                       nsim_fib_rt_ht_params);
> > > +               fib_rt = rhashtable_walk_peek(&hti);
> > >                nsim_fib_rt_free(fib_rt, data);
> > >        }
> > 
> > In general rhashtable walks are not stable.  You may miss entries
> > or see entries twice.  They should be avoided unless absolutely
> > necessary.
> Agree, but how about using rhashtable_free_and_destroy here
> instead of rhashtable_walk_next in this patch.

We don't want to destroy the hashtable in this case, only free the
objects.