| 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
| ||
|
Message-ID: <CO1PR11MB50893EF541863B31DD3139A9D6FD9@CO1PR11MB5089.namprd11.prod.outlook.com>
Date: Mon, 16 Aug 2021 21:27:54 +0000
From: "Keller, Jacob E" <jacob.e.keller@...el.com>
To: Leon Romanovsky <leon@...nel.org>,
"David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>
CC: Leon Romanovsky <leonro@...dia.com>,
Guangbin Huang <huangguangbin2@...wei.com>,
Jiri Pirko <jiri@...dia.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Salil Mehta <salil.mehta@...wei.com>,
Shannon Nelson <snelson@...sando.io>,
Yisen Zhuang <yisen.zhuang@...wei.com>,
Yufeng Mo <moyufeng@...wei.com>
Subject: RE: [PATCH net-next 4/6] devlink: Use xarray to store devlink
instances
> -----Original Message-----
> From: Leon Romanovsky <leon@...nel.org>
> Sent: Saturday, August 14, 2021 2:57 AM
> To: David S . Miller <davem@...emloft.net>; Jakub Kicinski <kuba@...nel.org>
> Cc: Leon Romanovsky <leonro@...dia.com>; Guangbin Huang
> <huangguangbin2@...wei.com>; Keller, Jacob E <jacob.e.keller@...el.com>; Jiri
> Pirko <jiri@...dia.com>; linux-kernel@...r.kernel.org; netdev@...r.kernel.org;
> Salil Mehta <salil.mehta@...wei.com>; Shannon Nelson
> <snelson@...sando.io>; Yisen Zhuang <yisen.zhuang@...wei.com>; Yufeng
> Mo <moyufeng@...wei.com>
> Subject: [PATCH net-next 4/6] devlink: Use xarray to store devlink instances
>
> From: Leon Romanovsky <leonro@...dia.com>
>
> We can use xarray instead of linearly organized linked lists for the
> devlink instances. This will let us revise the locking scheme in favour
> of internal xarray locking that protects database.
>
Nice. Seems like an xarray makes quite a bit of sense here vs a linked list, and it's resizable. Plus we can use marks to loop over the registered devlinks. Ok.
The conversions to xa interfaces look correct to me.
Reviewed-by: Jacob Keller <jacob.e.keller@...el.com>
Thanks,
Jake
> Signed-off-by: Leon Romanovsky <leonro@...dia.com>
> ---
> include/net/devlink.h | 2 +-
> net/core/devlink.c | 70 ++++++++++++++++++++++++++++++-------------
> 2 files changed, 50 insertions(+), 22 deletions(-)
>
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index 4c60d61d92da..154cf0dbca37 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -32,7 +32,7 @@ struct devlink_dev_stats {
> struct devlink_ops;
>
> struct devlink {
> - struct list_head list;
> + u32 index;
> struct list_head port_list;
> struct list_head rate_list;
> struct list_head sb_list;
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index 76f459da6e05..d218f57ad8cf 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -92,7 +92,8 @@ static const struct nla_policy
> devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
> DEVLINK_PORT_FN_STATE_ACTIVE),
> };
>
> -static LIST_HEAD(devlink_list);
> +static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> +#define DEVLINK_REGISTERED XA_MARK_1
>
> /* devlink_mutex
> *
> @@ -123,6 +124,7 @@ static struct devlink *devlink_get_from_attrs(struct net
> *net,
> struct nlattr **attrs)
> {
> struct devlink *devlink;
> + unsigned long index;
> bool found = false;
> char *busname;
> char *devname;
> @@ -135,7 +137,7 @@ static struct devlink *devlink_get_from_attrs(struct net
> *net,
>
> lockdep_assert_held(&devlink_mutex);
>
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (strcmp(devlink->dev->bus->name, busname) == 0 &&
> strcmp(dev_name(devlink->dev), devname) == 0 &&
> net_eq(devlink_net(devlink), net)) {
> @@ -1087,11 +1089,12 @@ static int devlink_nl_cmd_rate_get_dumpit(struct
> sk_buff *msg,
> struct devlink_rate *devlink_rate;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -1189,11 +1192,12 @@ static int devlink_nl_cmd_get_dumpit(struct sk_buff
> *msg,
> {
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -1251,11 +1255,12 @@ static int devlink_nl_cmd_port_get_dumpit(struct
> sk_buff *msg,
> struct devlink *devlink;
> struct devlink_port *devlink_port;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -1916,11 +1921,12 @@ static int devlink_nl_cmd_sb_get_dumpit(struct
> sk_buff *msg,
> struct devlink *devlink;
> struct devlink_sb *devlink_sb;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -2067,11 +2073,12 @@ static int
> devlink_nl_cmd_sb_pool_get_dumpit(struct sk_buff *msg,
> struct devlink *devlink;
> struct devlink_sb *devlink_sb;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -2287,11 +2294,12 @@ static int
> devlink_nl_cmd_sb_port_pool_get_dumpit(struct sk_buff *msg,
> struct devlink *devlink;
> struct devlink_sb *devlink_sb;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -2535,11 +2543,12 @@ devlink_nl_cmd_sb_tc_pool_bind_get_dumpit(struct
> sk_buff *msg,
> struct devlink *devlink;
> struct devlink_sb *devlink_sb;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -4611,11 +4620,12 @@ static int devlink_nl_cmd_param_get_dumpit(struct
> sk_buff *msg,
> struct devlink_param_item *param_item;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -4886,11 +4896,12 @@ static int
> devlink_nl_cmd_port_param_get_dumpit(struct sk_buff *msg,
> struct devlink_port *devlink_port;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -5462,11 +5473,12 @@ static int devlink_nl_cmd_region_get_dumpit(struct
> sk_buff *msg,
> {
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -5995,11 +6007,12 @@ static int devlink_nl_cmd_info_get_dumpit(struct
> sk_buff *msg,
> {
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err = 0;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -7176,11 +7189,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct
> sk_buff *msg,
> struct devlink_port *port;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -7210,7 +7224,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct
> sk_buff *msg,
> devlink_put(devlink);
> }
>
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -7771,11 +7785,12 @@ static int devlink_nl_cmd_trap_get_dumpit(struct
> sk_buff *msg,
> struct devlink_trap_item *trap_item;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -7997,11 +8012,12 @@ static int
> devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
> u32 portid = NETLINK_CB(cb->skb).portid;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -8310,11 +8326,12 @@ static int
> devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
> u32 portid = NETLINK_CB(cb->skb).portid;
> struct devlink *devlink;
> int start = cb->args[0];
> + unsigned long index;
> int idx = 0;
> int err;
>
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> @@ -8899,6 +8916,8 @@ struct devlink *devlink_alloc_ns(const struct
> devlink_ops *ops,
> struct device *dev)
> {
> struct devlink *devlink;
> + static u32 last_id;
> + int ret;
>
> WARN_ON(!ops || !dev);
> if (!devlink_reload_actions_valid(ops))
> @@ -8908,6 +8927,13 @@ struct devlink *devlink_alloc_ns(const struct
> devlink_ops *ops,
> if (!devlink)
> return NULL;
>
> + ret = xa_alloc_cyclic(&devlinks, &devlink->index, devlink, xa_limit_31b,
> + &last_id, GFP_KERNEL);
> + if (ret < 0) {
> + kfree(devlink);
> + return NULL;
> + }
> +
> devlink->dev = dev;
> devlink->ops = ops;
> xa_init_flags(&devlink->snapshot_ids, XA_FLAGS_ALLOC);
> @@ -8940,7 +8966,7 @@ EXPORT_SYMBOL_GPL(devlink_alloc_ns);
> int devlink_register(struct devlink *devlink)
> {
> mutex_lock(&devlink_mutex);
> - list_add_tail(&devlink->list, &devlink_list);
> + xa_set_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> devlink_notify(devlink, DEVLINK_CMD_NEW);
> mutex_unlock(&devlink_mutex);
> return 0;
> @@ -8961,7 +8987,7 @@ void devlink_unregister(struct devlink *devlink)
> WARN_ON(devlink_reload_supported(devlink->ops) &&
> devlink->reload_enabled);
> devlink_notify(devlink, DEVLINK_CMD_DEL);
> - list_del(&devlink->list);
> + xa_clear_mark(&devlinks, devlink->index, DEVLINK_REGISTERED);
> mutex_unlock(&devlink_mutex);
> }
> EXPORT_SYMBOL_GPL(devlink_unregister);
> @@ -9023,6 +9049,7 @@ void devlink_free(struct devlink *devlink)
> WARN_ON(!list_empty(&devlink->port_list));
>
> xa_destroy(&devlink->snapshot_ids);
> + xa_erase(&devlinks, devlink->index);
>
> kfree(devlink);
> }
> @@ -11497,13 +11524,14 @@ static void __net_exit
> devlink_pernet_pre_exit(struct net *net)
> {
> struct devlink *devlink;
> u32 actions_performed;
> + unsigned long index;
> int err;
>
> /* In case network namespace is getting destroyed, reload
> * all devlink instances from this namespace into init_net.
> */
> mutex_lock(&devlink_mutex);
> - list_for_each_entry(devlink, &devlink_list, list) {
> + xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
> if (!devlink_try_get(devlink))
> continue;
>
> --
> 2.31.1
Powered by blists - more mailing lists