Devlink cleanup for delay event series

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

The reference counting provides us a way to ensure that no new user space commands success to grab devlink instance which is going to be destroyed makes it is safe to access it without lock.

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely know this but I thought I'd comment on an off chance it helps.

Quoted 4 lines

The reference counting provides us a way to ensure that no new user space commands success to grab devlink instance which is going to be destroyed makes it is safe to access it without lock.

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 8 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely know this but I thought I'd comment on an off chance it helps.

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Monday, August 16, 2021 9:07 AM
To: Leon Romanovsky <[email protected]>
Cc: David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 18 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 8 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely know this but I thought I'd comment on an off chance it helps.

Quoted 6 lines

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

Quoted 40 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Monday, August 16, 2021 9:07 AM
To: Leon Romanovsky <[email protected]>
Cc: David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 24 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 8 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely know this but I thought I'd comment on an off chance it helps.

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

Quoted 3 lines

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

Also that I think we have some cases where we want to allow the driver to allocate new devlink objects in response to adding a port, but still want to block other global operations from running?

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Wednesday, August 18, 2021 1:12 AM
To: Keller, Jacob E <[email protected]>
Cc: Jakub Kicinski <[email protected]>; David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 8 lines · Jacob E

Quoted 8 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Monday, August 16, 2021 9:07 AM
To: Leon Romanovsky <[email protected]>
Cc: David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>;

Jiri

Quoted 1 line

Quoted 1 line

Pirko <[email protected]>; [email protected];

[email protected];

Quoted 15 lines

Quoted 15 lines

Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 9 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely

know

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

this but I thought I'd comment on an off chance it helps.

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

Quoted 5 lines

Quoted 3 lines

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

Also that I think we have some cases where we want to allow the driver to

allocate new devlink objects in response to adding a port, but still want to block other global operations from running?

I don't see the flow where operations on devlink_A should block devlink_B. Only in such flows we will need global lock like we have now - devlink->lock. In all other flows, write lock of devlink instance will protect from parallel execution.

Thanks

Quoted 70 lines

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Wednesday, August 18, 2021 1:12 AM
To: Keller, Jacob E <[email protected]>
Cc: Jakub Kicinski <[email protected]>; David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 8 lines · Jacob E

Quoted 8 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Monday, August 16, 2021 9:07 AM
To: Leon Romanovsky <[email protected]>
Cc: David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>;

Jiri

Quoted 1 line

Quoted 1 line

Pirko <[email protected]>; [email protected];

[email protected];

Quoted 15 lines

Quoted 15 lines

Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 9 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely

know

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 1 line

this but I thought I'd comment on an off chance it helps.

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

Quoted 5 lines

Quoted 3 lines

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

Also that I think we have some cases where we want to allow the driver to

allocate new devlink objects in response to adding a port, but still want to block other global operations from running?

I don't see the flow where operations on devlink_A should block devlink_B. Only in such flows we will need global lock like we have now - devlink->lock. In all other flows, write lock of devlink instance will protect from parallel execution.

Quoted 1 line

Thanks

But how do we handle what is essentially recursion?

If we add a port on the devlink A:

userspace sends PORT_ADD for devlink A
driver responds by creating a port
adding a port causes driver to add a region, or other devlink object

In the current design, if I understand correctly, we hold the global lock but *not* the instance lock. We can't hold the instance lock while adding port without breaking a bunch of drivers that add many devlink objects in response to port creation.. because they'll deadlock when going to add the sub objects.

But if we don't hold the global lock, then in theory another userspace program could attempt to do something inbetween PORT_ADD starting and finishing which might not be desirable. (Remember, we had to drop the instance lock otherwise drivers get stuck when trying to add many subobjects)

Thanks,
Jake

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Friday, August 20, 2021 6:07 AM
To: Keller, Jacob E <[email protected]>
Cc: Jakub Kicinski <[email protected]>; David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 7 lines · Jacob E

Quoted 7 lines

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Wednesday, August 18, 2021 1:12 AM
To: Keller, Jacob E <[email protected]>
Cc: Jakub Kicinski <[email protected]>; David S . Miller

<[email protected]>;

Quoted 1 line

Quoted 1 line

Guangbin Huang <[email protected]>; Jiri Pirko

<[email protected]>;

Quoted 8 lines

Quoted 8 lines

[email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 2 lines · Jacob E

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

-----Original Message-----
From: Jakub Kicinski <[email protected]>
Sent: Monday, August 16, 2021 9:07 AM
To: Leon Romanovsky <[email protected]>
Cc: David S . Miller <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E

<[email protected]>;

Quoted 5 lines

Quoted 5 lines

Jiri

Quoted 1 line

Quoted 1 line

Pirko <[email protected]>; [email protected];

[email protected];

Quoted 2 lines

Quoted 2 lines

Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>;

Yufeng

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Quoted 20 lines

Mo <[email protected]>
Subject: Re: [PATCH net-next 3/6] devlink: Count struct devlink consumers

Quoted 16 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 15 lines · Jakub Kicinski

Quoted 10 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely

Quoted 6 lines

Quoted 6 lines

know

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 1 line

this but I thought I'd comment on an off chance it helps.

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past

devlink_try_get)

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

3. Convert devlink->lock to be read/write lock to make sure that we can

run

Quoted 9 lines

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

Also that I think we have some cases where we want to allow the driver to

allocate new devlink objects in response to adding a port, but still want to

block

Quoted 7 lines

Quoted 7 lines

other global operations from running?

I don't see the flow where operations on devlink_A should block devlink_B. Only in such flows we will need global lock like we have now - devlink->lock. In all other flows, write lock of devlink instance will protect from parallel execution.

Quoted 4 lines

Quoted 1 line

Thanks

But how do we handle what is essentially recursion?

Let's wait till implementation, I promise it will be covered :).

Quoted 8 lines

If we add a port on the devlink A:

userspace sends PORT_ADD for devlink A
driver responds by creating a port
adding a port causes driver to add a region, or other devlink object

In the current design, if I understand correctly, we hold the global lock but

*not* the instance lock. We can't hold the instance lock while adding port without breaking a bunch of drivers that add many devlink objects in response to port creation.. because they'll deadlock when going to add the sub objects.

Quoted 2 lines

But if we don't hold the global lock, then in theory another userspace program

could attempt to do something inbetween PORT_ADD starting and finishing which might not be desirable. (Remember, we had to drop the instance lock otherwise drivers get stuck when trying to add many subobjects)

You just surfaced my main issue with the current devlink implementation - the purpose of devlink_lock. Over the years devlink code lost clear separation between user space flows and kernel flows.

Thanks

Quoted 3 lines

Thanks,
Jake

Quoted 18 lines · On Mon, 16 Aug 2021 18:53:45 +0300 Leon Romanovsky wrote:

Quoted 8 lines · Jakub Kicinski

Quoted 7 lines · On Sat, 14 Aug 2021 12:57:28 +0300 Leon Romanovsky wrote:

From: Leon Romanovsky <[email protected]>

The struct devlink itself is protected by internal lock and doesn't need global lock during operation. That global lock is used to protect addition/removal new devlink instances from the global list in use by all devlink consumers in the system.

Quoted 8 lines

Quoted 3 lines

The future conversion of linked list to be xarray will allow us to actually delete that lock, but first we need to count all struct devlink users.

Not a problem with this set but to state the obvious the global devlink lock also protects from concurrent execution of all the ops which don't take the instance lock (DEVLINK_NL_FLAG_NO_LOCK). You most likely know this but I thought I'd comment on an off chance it helps.

Quoted 6 lines

The end goal will be something like that:
1. Delete devlink lock
2. Rely on xa_lock() while grabbing devlink instance (past devlink_try_get)
3. Convert devlink->lock to be read/write lock to make sure that we can run get query in parallel.
4. Open devlink netlink to parallel ops, ".parallel_ops = true".

IIUC that'd mean setting eswitch mode would hold write lock on the dl instance. What locks does e.g. registering a dl port take then?

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Saturday, August 14, 2021 2:57 AM
To: David S . Miller <[email protected]>; Jakub Kicinski <[email protected]>
Cc: Leon Romanovsky <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: [PATCH net-next 4/6] devlink: Use xarray to store devlink instances

From: Leon Romanovsky <[email protected]>

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.

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/net/devlink.h |  2 +-
 net/core/devlink.c    | 70 ++++++++++++++++++++++++++++++-------------
 2 files changed, 50 insertions(+), 22 deletions(-)

include/net/devlink.h +1 -1

index 4c60d61d92da..154cf0dbca37 100644
@@ -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;

net/core/devlink.c +0 -0

index 76f459da6e05..d218f57ad8cf 100644
@@ -92,7 +92,8 @@ static const struct nla_policy

devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_

net/core/devlink.c +3 -1

 				 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;

net/core/devlink.c +1 -1

 	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/core/devlink.c +33 -16

 		    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);

net/core/devlink.c +7 -0

 	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;

net/core/devlink.c +5 -4

 	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

-----Original Message-----
From: Leon Romanovsky <[email protected]>
Sent: Saturday, August 14, 2021 2:58 AM
To: David S . Miller <[email protected]>; Jakub Kicinski <[email protected]>
Cc: Leon Romanovsky <[email protected]>; Guangbin Huang <[email protected]>; Keller, Jacob E <[email protected]>; Jiri Pirko <[email protected]>; [email protected]; [email protected]; Salil Mehta <[email protected]>; Shannon Nelson <[email protected]>; Yisen Zhuang <[email protected]>; Yufeng Mo <[email protected]>
Subject: [PATCH net-next 5/6] devlink: Clear whole devlink_flash_notify struct

From: Leon Romanovsky <[email protected]>

The { 0 } doesn't clear all fields in the struct, but tells to the compiler to set all fields to zero and doesn't touch any sub-fields if they exists.

The {} is an empty initialiser that instructs to fully initialize whole struct including sub-fields, which is error-prone for future devlink_flash_notify extensions.

Fixes 6700acc5f1fe ("devlink: collect flash notify params into a struct")

Signed-off-by Leon Romanovsky <[email protected]>

---
 net/core/devlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/core/devlink.c +1 -1

index d218f57ad8cf..a856ae401ea5 100644
@@ -4169,7 +4169,7 @@ static void __devlink_flash_update_notify(struct
 devlink *devlink,
 
 static void devlink_flash_update_begin_notify(struct devlink *devlink)
 {
-	struct devlink_flash_notify params = { 0 };
+	struct devlink_flash_notify params = {};
 
 	__devlink_flash_update_notify(devlink,

net/core/devlink.c +1 -1

 				      DEVLINK_CMD_FLASH_UPDATE,

@@ -4178,7 +4178,7 @@ static void devlink_flash_update_begin_notify(struct
 devlink *devlink)
 
 static void devlink_flash_update_end_notify(struct devlink *devlink)
 {
-	struct devlink_flash_notify params = { 0 };
+	struct devlink_flash_notify params = {};
 
 	__devlink_flash_update_notify(devlink,

DEVLINK_CMD_FLASH_UPDATE_END,
--
2.31.1