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 <[email protected]>
---
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
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
Cai Huoqing <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On 26 4月 23 17:37:49, Herbert Xu wrote:
> Cai Huoqing <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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 <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.