LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Keller, Jacob E" <jacob.e.keller@intel.com>
To: Leon Romanovsky <leon@kernel.org>,
	"David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>
Cc: Leon Romanovsky <leonro@nvidia.com>,
	Guangbin Huang <huangguangbin2@huawei.com>,
	Jiri Pirko <jiri@nvidia.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	Salil Mehta <salil.mehta@huawei.com>,
	Shannon Nelson <snelson@pensando.io>,
	Yisen Zhuang <yisen.zhuang@huawei.com>,
	Yufeng Mo <moyufeng@huawei.com>
Subject: RE: [PATCH net-next 4/6] devlink: Use xarray to store devlink instances
Date: Mon, 16 Aug 2021 21:27:54 +0000	[thread overview]
Message-ID: <CO1PR11MB50893EF541863B31DD3139A9D6FD9@CO1PR11MB5089.namprd11.prod.outlook.com> (raw)
In-Reply-To: <38c969b6f490c2b2c0959c017b4461f69ab96117.1628933864.git.leonro@nvidia.com>



> -----Original Message-----
> From: Leon Romanovsky <leon@kernel.org>
> Sent: Saturday, August 14, 2021 2:57 AM
> To: David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>
> Cc: Leon Romanovsky <leonro@nvidia.com>; Guangbin Huang
> <huangguangbin2@huawei.com>; Keller, Jacob E <jacob.e.keller@intel.com>; Jiri
> Pirko <jiri@nvidia.com>; linux-kernel@vger.kernel.org; netdev@vger.kernel.org;
> Salil Mehta <salil.mehta@huawei.com>; Shannon Nelson
> <snelson@pensando.io>; Yisen Zhuang <yisen.zhuang@huawei.com>; Yufeng
> Mo <moyufeng@huawei.com>
> Subject: [PATCH net-next 4/6] devlink: Use xarray to store devlink instances
> 
> From: Leon Romanovsky <leonro@nvidia.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@intel.com>

Thanks,
Jake

> Signed-off-by: Leon Romanovsky <leonro@nvidia.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


  reply	other threads:[~2021-08-16 21:28 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14  9:57 [PATCH net-next 0/6] Devlink cleanup for delay event series Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 1/6] devlink: Simplify devlink_pernet_pre_exit call Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 2/6] devlink: Remove check of always valid devlink pointer Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 3/6] devlink: Count struct devlink consumers Leon Romanovsky
2021-08-16 15:47   ` Jakub Kicinski
2021-08-16 15:53     ` Leon Romanovsky
2021-08-16 16:07       ` Jakub Kicinski
2021-08-16 21:32         ` Keller, Jacob E
2021-08-18  8:12           ` Leon Romanovsky
2021-08-18 17:50             ` Keller, Jacob E
2021-08-20 13:06               ` Leon Romanovsky
2021-08-20 20:23                 ` Keller, Jacob E
2021-08-18  8:03         ` Leon Romanovsky
2021-08-14  9:57 ` [PATCH net-next 4/6] devlink: Use xarray to store devlink instances Leon Romanovsky
2021-08-16 21:27   ` Keller, Jacob E [this message]
2021-08-14  9:57 ` [PATCH net-next 5/6] devlink: Clear whole devlink_flash_notify struct Leon Romanovsky
2021-08-16 21:29   ` Keller, Jacob E
2021-08-14  9:57 ` [PATCH net-next 6/6] net: hns3: remove always exist devlink pointer check Leon Romanovsky
2021-08-14 13:10 ` [PATCH net-next 0/6] Devlink cleanup for delay event series patchwork-bot+netdevbpf

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CO1PR11MB50893EF541863B31DD3139A9D6FD9@CO1PR11MB5089.namprd11.prod.outlook.com \
    --to=jacob.e.keller@intel.com \
    --cc=davem@davemloft.net \
    --cc=huangguangbin2@huawei.com \
    --cc=jiri@nvidia.com \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=leonro@nvidia.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=moyufeng@huawei.com \
    --cc=netdev@vger.kernel.org \
    --cc=salil.mehta@huawei.com \
    --cc=snelson@pensando.io \
    --cc=yisen.zhuang@huawei.com \
    --subject='RE: [PATCH net-next 4/6] devlink: Use xarray to store devlink instances' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).