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;
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
Thread-Topic: [PATCH net-next 4/6] devlink: Use xarray to store devlink
 instances
Thread-Index: AQHXkPLbdRN88+IU5E+IidkVhhnEw6t2qDWA
Date:   Mon, 16 Aug 2021 21:27:54 +0000
Message-ID: <CO1PR11MB50893EF541863B31DD3139A9D6FD9@CO1PR11MB5089.namprd11.prod.outlook.com>
References: <cover.1628933864.git.leonro@nvidia.com>
 <38c969b6f490c2b2c0959c017b4461f69ab96117.1628933864.git.leonro@nvidia.com>
In-Reply-To: <38c969b6f490c2b2c0959c017b4461f69ab96117.1628933864.git.leonro@nvidia.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.5.1.3
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?ebar/sxe50dcg9OszQpA5FFQT//U30qtRodRi7gHge9hBLHCU6rbJwKOYg2o?=
 =?us-ascii?Q?81G+/6fKowjPst99FIBobMyUjNmipLB7N02zoLewCtdWBr8bVJBbMG1xx2rC?=
 =?us-ascii?Q?TAnXRD+vvUl7SD6spQITlqwjIqKWkQ+o1MMUhkuzfz1sm6E2pYWho5I+mXyE?=
 =?us-ascii?Q?QNKILBlCeHkREvbejGDAfJCfAQbWX0ns71y2ZdKgCqYmsbqTY4wffCJ3Nh+N?=
 =?us-ascii?Q?QZM74fwLTefSyu3oTWh8fuYuygdNQqMdoJkbBw3eho+Gg7dFqeTf6ByUMcdY?=
 =?us-ascii?Q?1vk0ZdSFF/rapu3Qenp2bm9/19ULKRrZUn0QW88fOSog/smbFPdqdQMO2vJz?=
 =?us-ascii?Q?/pQfc5fHhYXymNPi3Rq5Wdy6wCO9VUT7UBrODBe2bdffx6NpToFZaChBOvBc?=
 =?us-ascii?Q?ACc6E4+K/s+4lIHbyEgouyEI6+stJtY8m511BKFZg3BY0do1FupTUxhRkBsb?=
 =?us-ascii?Q?C0NsQeAzJHbSecpjEwQoX6mjC/uE1s0z8Y9zyE+kbMLjidwUlUOXFckHwPIe?=
 =?us-ascii?Q?2/rcftD6ytqBY661RsnfodWFGL3zq7eh3gLUdzVz0f7tNkbe/+jR/NAz3+tT?=
 =?us-ascii?Q?eHVmrgryHTQByt9ayaQiOlgbiEoeNroDqDkNZsKGXoLm+eHIZvIyaOaYUjRR?=
 =?us-ascii?Q?Ktoo0GiAtCE30i6jqglhlrb8vPKKgg/FeLX+ieXo6vdkldJkW53G5ddJLxND?=
 =?us-ascii?Q?wYpWbVb1tKGbPGh1N07tHW3aOJbzJaDd7lgD9vN4qEw19GmwPomc8RPeSrml?=
 =?us-ascii?Q?G7bvteTiNk3i8xQBjAc9dNCbYmz+kwGU75HaAz1mzjVyj+4gPcwO3B62BaDf?=
 =?us-ascii?Q?hlVZ/AcBJ/AVFPzJeHNtK9hrB7uZapwKrFSIcsStgZ91k6ncFn/jWXwfEz17?=
 =?us-ascii?Q?fybw0sR3j9bGWDuSG+GHf8M0Xe0LwtVI5X4hj5ShGp5iCvfGZMi3PQWjwYYy?=
 =?us-ascii?Q?00i8Hinb+ljnJfpQFbkL3sDhu2Z8dHLjuEUrN0LPXrFdO66u03n3pvmlQyJu?=
 =?us-ascii?Q?8r8zxLw7dsJHUqZpo05dBzZVToeNgi3fhlJpns5p0nWjl7U38rX+tKvENjt1?=
 =?us-ascii?Q?6BSAqCAnU+yS49oAGICELErWPGIFwYd0YjHA0oQSvFXrVATLfy5boF5P+bNC?=
 =?us-ascii?Q?JNsE9r7IdOvI0t5syST8kQ0OgrA8pc3zahTRPIL8Wh2QweUiKSyK12TXDggJ?=
 =?us-ascii?Q?CVQnH5UU1cYqJD2Rwd8JdAooI/TP3qnAfnJ0x+oNm1NJko0foMPFaorL2YeF?=
 =?us-ascii?Q?zu+27gg8RZsAnWeD3Yly1wvutOJ5SIyimrUhaJ+D3Du8lqKapGd7XPs9FugW?=
 =?us-ascii?Q?mF4=3D?=
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CO1PR11MB5089.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d9fe7710-4045-44ac-c6f2-08d960fcb5a2
X-MS-Exchange-CrossTenant-originalarrivaltime: 16 Aug 2021 21:27:54.3411
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3X//QsXRJ12Z5XRQVVz1Zll8IrSG0n/6e94ONCKT1p5x9AQ7ApbrcPKsz+MPTFUfNhMdhCoWWBaUkttaC+cNpAPPxy34FK0mhFf0t73HChI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR11MB1278
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> -----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.o=
rg>
> 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 instan=
ces
>=20
> From: Leon Romanovsky <leonro@nvidia.com>
>=20
> 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.
>=20

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 dev=
links. 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(-)
>=20
> 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;
>=20
>  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),
>  };
>=20
> -static LIST_HEAD(devlink_list);
> +static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);
> +#define DEVLINK_REGISTERED XA_MARK_1
>=20
>  /* 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 =3D false;
>  	char *busname;
>  	char *devname;
> @@ -135,7 +137,7 @@ static struct devlink *devlink_get_from_attrs(struct =
net
> *net,
>=20
>  	lockdep_assert_held(&devlink_mutex);
>=20
> -	list_for_each_entry(devlink, &devlink_list, list) {
> +	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>  		if (strcmp(devlink->dev->bus->name, busname) =3D=3D 0 &&
>  		    strcmp(dev_name(devlink->dev), devname) =3D=3D 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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -1189,11 +1192,12 @@ static int devlink_nl_cmd_get_dumpit(struct sk_bu=
ff
> *msg,
>  {
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -5462,11 +5473,12 @@ static int devlink_nl_cmd_region_get_dumpit(struc=
t
> sk_buff *msg,
>  {
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -5995,11 +6007,12 @@ static int devlink_nl_cmd_info_get_dumpit(struct
> sk_buff *msg,
>  {
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err =3D 0;
>=20
>  	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;
>=20
> @@ -7176,11 +7189,12 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct
> sk_buff *msg,
>  	struct devlink_port *port;
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -7210,7 +7224,7 @@ devlink_nl_cmd_health_reporter_get_dumpit(struct
> sk_buff *msg,
>  		devlink_put(devlink);
>  	}
>=20
> -	list_for_each_entry(devlink, &devlink_list, list) {
> +	xa_for_each_marked(&devlinks, index, devlink, DEVLINK_REGISTERED) {
>  		if (!devlink_try_get(devlink))
>  			continue;
>=20
> @@ -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 =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -7997,11 +8012,12 @@ static int
> devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
>  	u32 portid =3D NETLINK_CB(cb->skb).portid;
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -8310,11 +8326,12 @@ static int
> devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
>  	u32 portid =3D NETLINK_CB(cb->skb).portid;
>  	struct devlink *devlink;
>  	int start =3D cb->args[0];
> +	unsigned long index;
>  	int idx =3D 0;
>  	int err;
>=20
>  	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;
>=20
> @@ -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;
>=20
>  	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;
>=20
> +	ret =3D xa_alloc_cyclic(&devlinks, &devlink->index, devlink, xa_limit_3=
1b,
> +			      &last_id, GFP_KERNEL);
> +	if (ret < 0) {
> +		kfree(devlink);
> +		return NULL;
> +	}
> +
>  	devlink->dev =3D dev;
>  	devlink->ops =3D 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));
>=20
>  	xa_destroy(&devlink->snapshot_ids);
> +	xa_erase(&devlinks, devlink->index);
>=20
>  	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;
>=20
>  	/* 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;
>=20
> --
> 2.31.1