| Message ID | 20231220014747.1508581-2-dw@davidwei.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | netdevsim: link and forward skbs between ports | expand |
Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote: >This patch adds a linked list nsim_dev_list of probed netdevsims, added >during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >nsim_dev_list_lock protects the list. In the commit message, you should use imperative mood, command the codebase what to do: https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes > >Signed-off-by: David Wei <dw@davidwei.uk> >--- > drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ > drivers/net/netdevsim/netdevsim.h | 1 + > 2 files changed, 18 insertions(+) > >diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >index b4d3b9cde8bd..e30a12130e07 100644 >--- a/drivers/net/netdevsim/dev.c >+++ b/drivers/net/netdevsim/dev.c >@@ -35,6 +35,9 @@ > > #include "netdevsim.h" > >+static LIST_HEAD(nsim_dev_list); >+static DEFINE_MUTEX(nsim_dev_list_lock); >+ > static unsigned int > nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) > { >@@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > nsim_bus_dev->initial_net, &nsim_bus_dev->dev); > if (!devlink) > return -ENOMEM; >+ mutex_lock(&nsim_dev_list_lock); I don't follow. You claim you use this mutex to protect the list. a) why don't you use spin-lock? b) why don't don't you take the lock just for list manipulation? > devl_lock(devlink); > nsim_dev = devlink_priv(devlink); > nsim_dev->nsim_bus_dev = nsim_bus_dev; >@@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > spin_lock_init(&nsim_dev->fa_cookie_lock); > > dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >+ list_add(&nsim_dev->list, &nsim_dev_list); > > nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, > sizeof(struct nsim_vf_config), >@@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > > nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; > devl_unlock(devlink); >+ mutex_unlock(&nsim_dev_list_lock); > return 0; > > err_hwstats_exit: >@@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) > { > struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); > struct devlink *devlink = priv_to_devlink(nsim_dev); >+ struct nsim_dev *pos, *tmp; > >+ mutex_lock(&nsim_dev_list_lock); > devl_lock(devlink); >+ >+ list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { >+ if (pos == nsim_dev) { >+ list_del(&nsim_dev->list); >+ break; >+ } >+ } >+ > nsim_dev_reload_destroy(nsim_dev); > > nsim_bpf_dev_exit(nsim_dev); >@@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) > kfree(nsim_dev->vfconfigs); > kfree(nsim_dev->fa_cookie); > devl_unlock(devlink); >+ mutex_unlock(&nsim_dev_list_lock); > devlink_free(devlink); > dev_set_drvdata(&nsim_bus_dev->dev, NULL); > } >diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h >index 028c825b86db..babb61d7790b 100644 >--- a/drivers/net/netdevsim/netdevsim.h >+++ b/drivers/net/netdevsim/netdevsim.h >@@ -277,6 +277,7 @@ struct nsim_vf_config { > > struct nsim_dev { > struct nsim_bus_dev *nsim_bus_dev; >+ struct list_head list; > struct nsim_fib_data *fib_data; > struct nsim_trap_data *trap_data; > struct dentry *ddir; >-- >2.39.3 >
On Tue, Dec 19, 2023 at 05:47:43PM -0800, David Wei wrote: > This patch adds a linked list nsim_dev_list of probed netdevsims, added > during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex > nsim_dev_list_lock protects the list. > > Signed-off-by: David Wei <dw@davidwei.uk> > --- > drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ > drivers/net/netdevsim/netdevsim.h | 1 + > 2 files changed, 18 insertions(+) > > diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c > index b4d3b9cde8bd..e30a12130e07 100644 > --- a/drivers/net/netdevsim/dev.c > +++ b/drivers/net/netdevsim/dev.c > @@ -35,6 +35,9 @@ > > #include "netdevsim.h" > > +static LIST_HEAD(nsim_dev_list); > +static DEFINE_MUTEX(nsim_dev_list_lock); > + > static unsigned int > nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) > { > @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > nsim_bus_dev->initial_net, &nsim_bus_dev->dev); > if (!devlink) > return -ENOMEM; > + mutex_lock(&nsim_dev_list_lock); > devl_lock(devlink); > nsim_dev = devlink_priv(devlink); > nsim_dev->nsim_bus_dev = nsim_bus_dev; > @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > spin_lock_init(&nsim_dev->fa_cookie_lock); > > dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); > + list_add(&nsim_dev->list, &nsim_dev_list); > > nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, > sizeof(struct nsim_vf_config), > @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) > > nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; > devl_unlock(devlink); > + mutex_unlock(&nsim_dev_list_lock); > return 0; > Hi David, I see Jiri has asked about the scope and type of this lock. And updates to address those questions may obviate my observation. But it is that mutex_unlock(&nsim_dev_list_lock); needs to be added to the unwind ladder: ... err_devlink_unlock: devl_unlock(devlink); mutex_unlock(&nsim_dev_list_lock); ... ... Flagged by Smatch.
On 2023-12-20 00:57, Jiri Pirko wrote: > Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote: >> This patch adds a linked list nsim_dev_list of probed netdevsims, added >> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >> nsim_dev_list_lock protects the list. > > In the commit message, you should use imperative mood, command > the codebase what to do: > https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes Thanks, I didn't know about this. Will edit the commit messages. > > >> >> Signed-off-by: David Wei <dw@davidwei.uk> >> --- >> drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ >> drivers/net/netdevsim/netdevsim.h | 1 + >> 2 files changed, 18 insertions(+) >> >> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >> index b4d3b9cde8bd..e30a12130e07 100644 >> --- a/drivers/net/netdevsim/dev.c >> +++ b/drivers/net/netdevsim/dev.c >> @@ -35,6 +35,9 @@ >> >> #include "netdevsim.h" >> >> +static LIST_HEAD(nsim_dev_list); >> +static DEFINE_MUTEX(nsim_dev_list_lock); >> + >> static unsigned int >> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) >> { >> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> nsim_bus_dev->initial_net, &nsim_bus_dev->dev); >> if (!devlink) >> return -ENOMEM; >> + mutex_lock(&nsim_dev_list_lock); > > I don't follow. You claim you use this mutex to protect the list. > a) why don't you use spin-lock? I'm using a mutex unless I know (or someone else who knows better point out) that a spinlock is better. It is simple, there are fewer gotchas, and I anticipate actual contention here to be near 0. The nsim_bus_dev_list is also protected by a mutex. Is a spinlock better here and if so why? > b) why don't don't you take the lock just for list manipulation? Many code paths interact here, touching drivers and netdevs. There is an ordering of locks being taken: 1. nsim_bus_dev->dev.mutex 2. devlink->lock 3. rtnl_lock I was careful to avoid deadlocking by acquiring locks in the same order. But looking at it again, I can reduce the critical section by acquiring nsim_dev_list_lock after devlink->lock, thanks. > > >> devl_lock(devlink); >> nsim_dev = devlink_priv(devlink); >> nsim_dev->nsim_bus_dev = nsim_bus_dev; >> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> spin_lock_init(&nsim_dev->fa_cookie_lock); >> >> dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >> + list_add(&nsim_dev->list, &nsim_dev_list); >> >> nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, >> sizeof(struct nsim_vf_config), >> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> >> nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; >> devl_unlock(devlink); >> + mutex_unlock(&nsim_dev_list_lock); >> return 0; >> >> err_hwstats_exit: >> @@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >> { >> struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); >> struct devlink *devlink = priv_to_devlink(nsim_dev); >> + struct nsim_dev *pos, *tmp; >> >> + mutex_lock(&nsim_dev_list_lock); >> devl_lock(devlink); >> + >> + list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { >> + if (pos == nsim_dev) { >> + list_del(&nsim_dev->list); >> + break; >> + } >> + } >> + >> nsim_dev_reload_destroy(nsim_dev); >> >> nsim_bpf_dev_exit(nsim_dev); >> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >> kfree(nsim_dev->vfconfigs); >> kfree(nsim_dev->fa_cookie); >> devl_unlock(devlink); >> + mutex_unlock(&nsim_dev_list_lock); >> devlink_free(devlink); >> dev_set_drvdata(&nsim_bus_dev->dev, NULL); >> } >> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h >> index 028c825b86db..babb61d7790b 100644 >> --- a/drivers/net/netdevsim/netdevsim.h >> +++ b/drivers/net/netdevsim/netdevsim.h >> @@ -277,6 +277,7 @@ struct nsim_vf_config { >> >> struct nsim_dev { >> struct nsim_bus_dev *nsim_bus_dev; >> + struct list_head list; >> struct nsim_fib_data *fib_data; >> struct nsim_trap_data *trap_data; >> struct dentry *ddir; >> -- >> 2.39.3 >>
On 2023-12-20 08:40, Simon Horman wrote: > On Tue, Dec 19, 2023 at 05:47:43PM -0800, David Wei wrote: >> This patch adds a linked list nsim_dev_list of probed netdevsims, added >> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >> nsim_dev_list_lock protects the list. >> >> Signed-off-by: David Wei <dw@davidwei.uk> >> --- >> drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ >> drivers/net/netdevsim/netdevsim.h | 1 + >> 2 files changed, 18 insertions(+) >> >> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >> index b4d3b9cde8bd..e30a12130e07 100644 >> --- a/drivers/net/netdevsim/dev.c >> +++ b/drivers/net/netdevsim/dev.c >> @@ -35,6 +35,9 @@ >> >> #include "netdevsim.h" >> >> +static LIST_HEAD(nsim_dev_list); >> +static DEFINE_MUTEX(nsim_dev_list_lock); >> + >> static unsigned int >> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) >> { >> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> nsim_bus_dev->initial_net, &nsim_bus_dev->dev); >> if (!devlink) >> return -ENOMEM; >> + mutex_lock(&nsim_dev_list_lock); >> devl_lock(devlink); >> nsim_dev = devlink_priv(devlink); >> nsim_dev->nsim_bus_dev = nsim_bus_dev; >> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> spin_lock_init(&nsim_dev->fa_cookie_lock); >> >> dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >> + list_add(&nsim_dev->list, &nsim_dev_list); >> >> nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, >> sizeof(struct nsim_vf_config), >> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >> >> nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; >> devl_unlock(devlink); >> + mutex_unlock(&nsim_dev_list_lock); >> return 0; >> > > Hi David, > > I see Jiri has asked about the scope and type of this lock. > And updates to address those questions may obviate my observation. > But it is that mutex_unlock(&nsim_dev_list_lock); needs to > be added to the unwind ladder: > > ... > err_devlink_unlock: > devl_unlock(devlink); > mutex_unlock(&nsim_dev_list_lock); > ... > > ... > > Flagged by Smatch. Hi Simon, thanks for flagging this and I will address it in the next version.
On 2023-12-21 16:45, David Wei wrote: > On 2023-12-20 00:57, Jiri Pirko wrote: >> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote: >>> This patch adds a linked list nsim_dev_list of probed netdevsims, added >>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >>> nsim_dev_list_lock protects the list. >> >> In the commit message, you should use imperative mood, command >> the codebase what to do: >> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes > > Thanks, I didn't know about this. Will edit the commit messages. > >> >> >>> >>> Signed-off-by: David Wei <dw@davidwei.uk> >>> --- >>> drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ >>> drivers/net/netdevsim/netdevsim.h | 1 + >>> 2 files changed, 18 insertions(+) >>> >>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >>> index b4d3b9cde8bd..e30a12130e07 100644 >>> --- a/drivers/net/netdevsim/dev.c >>> +++ b/drivers/net/netdevsim/dev.c >>> @@ -35,6 +35,9 @@ >>> >>> #include "netdevsim.h" >>> >>> +static LIST_HEAD(nsim_dev_list); >>> +static DEFINE_MUTEX(nsim_dev_list_lock); >>> + >>> static unsigned int >>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) >>> { >>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> nsim_bus_dev->initial_net, &nsim_bus_dev->dev); >>> if (!devlink) >>> return -ENOMEM; >>> + mutex_lock(&nsim_dev_list_lock); >> >> I don't follow. You claim you use this mutex to protect the list. >> a) why don't you use spin-lock? > > I'm using a mutex unless I know (or someone else who knows better point > out) that a spinlock is better. It is simple, there are fewer gotchas, > and I anticipate actual contention here to be near 0. The > nsim_bus_dev_list is also protected by a mutex. > > Is a spinlock better here and if so why? > >> b) why don't don't you take the lock just for list manipulation? > > Many code paths interact here, touching drivers and netdevs. There is an > ordering of locks being taken: > > 1. nsim_bus_dev->dev.mutex > 2. devlink->lock > 3. rtnl_lock > > I was careful to avoid deadlocking by acquiring locks in the same order. > But looking at it again, I can reduce the critical section by acquiring > nsim_dev_list_lock after devlink->lock, thanks. Looking at this again, I need to prevent concurrent nsim_dev modifications _and_ nsim_dev_port modifications. This is because nsim_dev_peer_write() needs to traverse both of those lists to link up two netdevsims. I cannot use the existing devlink->lock for this, because to take it in nsim_dev_peer_write() I need to first safely get a nsim_dev. That's why in the patch I take nsim_dev_list_lock early with a seemingly large critical section. I think the following locking scheme would work: In nsim_drv_probe(): 1. Take nsim_dev_list_lock 2. Take devlink->lock 3. Construct nsim_dev 4. Construct all nsim_dev_ports a. Take rtnl_lock for each netdevsim port 5. Add to nsim_dev_list 6. Release devlink->lock 7. Release nsim_dev_list_lock Maybe 5 and 6 can be swapped, but I don't think it matters. In nsim_drv_remove(): 1. Take nsim_dev_list_lock 2. Take devlink->lock 3. Remove from nsim_dev_list 4. Destroy nsim_dev 5. Destroy all nsim_dev_ports a. During which, take rtnl_lock for each netdevsim 6. Release devlink->lock 7. Release nsim_dev_list_lock Similarly, maybe 2 and 3 can be swapped. In nsim_drv_port_add(): 1. Take devlink->lock 2. Take rtnl_lock and create netdevsim 3. Add to port_list 4. Release devlink->lock In nsim_dev_port_del(): 1. Take devlink->lock 2. Remove from port_list 3. Take rtnl_lock and destroy netdevsim 4. Release devlink->lock In nsim_dev_peer_write(): 1. Take nsim_dev_list_lock No concurrent modifications to nsim_dev_list, get peer nsim_dev 2. Take devlink->lock No concurrent modifications to port_list, get peer port and check current port in private_data still exists 3. Do the linking 4. Release devlink->lock 5. Release nsim_dev_list_lock In v4 I am taking rtnl_lock which may be a mistake. I don't know if there are other code paths that can modify a netdevsim's underlying net_device without taking devlink lock. If so, then I'd also need to take rtnl_lock after devlink->lock. > >> >> >>> devl_lock(devlink); >>> nsim_dev = devlink_priv(devlink); >>> nsim_dev->nsim_bus_dev = nsim_bus_dev; >>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> spin_lock_init(&nsim_dev->fa_cookie_lock); >>> >>> dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >>> + list_add(&nsim_dev->list, &nsim_dev_list); >>> >>> nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, >>> sizeof(struct nsim_vf_config), >>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> >>> nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> return 0; >>> >>> err_hwstats_exit: >>> @@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> { >>> struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); >>> struct devlink *devlink = priv_to_devlink(nsim_dev); >>> + struct nsim_dev *pos, *tmp; >>> >>> + mutex_lock(&nsim_dev_list_lock); >>> devl_lock(devlink); >>> + >>> + list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { >>> + if (pos == nsim_dev) { >>> + list_del(&nsim_dev->list); >>> + break; >>> + } >>> + } >>> + >>> nsim_dev_reload_destroy(nsim_dev); >>> >>> nsim_bpf_dev_exit(nsim_dev); >>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> kfree(nsim_dev->vfconfigs); >>> kfree(nsim_dev->fa_cookie); >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> devlink_free(devlink); >>> dev_set_drvdata(&nsim_bus_dev->dev, NULL); >>> } >>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h >>> index 028c825b86db..babb61d7790b 100644 >>> --- a/drivers/net/netdevsim/netdevsim.h >>> +++ b/drivers/net/netdevsim/netdevsim.h >>> @@ -277,6 +277,7 @@ struct nsim_vf_config { >>> >>> struct nsim_dev { >>> struct nsim_bus_dev *nsim_bus_dev; >>> + struct list_head list; >>> struct nsim_fib_data *fib_data; >>> struct nsim_trap_data *trap_data; >>> struct dentry *ddir; >>> -- >>> 2.39.3 >>>
Fri, Dec 22, 2023 at 01:45:58AM CET, dw@davidwei.uk wrote: >On 2023-12-20 00:57, Jiri Pirko wrote: >> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote: >>> This patch adds a linked list nsim_dev_list of probed netdevsims, added >>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >>> nsim_dev_list_lock protects the list. >> >> In the commit message, you should use imperative mood, command >> the codebase what to do: >> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes > >Thanks, I didn't know about this. Will edit the commit messages. > >> >> >>> >>> Signed-off-by: David Wei <dw@davidwei.uk> >>> --- >>> drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ >>> drivers/net/netdevsim/netdevsim.h | 1 + >>> 2 files changed, 18 insertions(+) >>> >>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >>> index b4d3b9cde8bd..e30a12130e07 100644 >>> --- a/drivers/net/netdevsim/dev.c >>> +++ b/drivers/net/netdevsim/dev.c >>> @@ -35,6 +35,9 @@ >>> >>> #include "netdevsim.h" >>> >>> +static LIST_HEAD(nsim_dev_list); >>> +static DEFINE_MUTEX(nsim_dev_list_lock); >>> + >>> static unsigned int >>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) >>> { >>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> nsim_bus_dev->initial_net, &nsim_bus_dev->dev); >>> if (!devlink) >>> return -ENOMEM; >>> + mutex_lock(&nsim_dev_list_lock); >> >> I don't follow. You claim you use this mutex to protect the list. >> a) why don't you use spin-lock? > >I'm using a mutex unless I know (or someone else who knows better point >out) that a spinlock is better. It is simple, there are fewer gotchas, >and I anticipate actual contention here to be near 0. The >nsim_bus_dev_list is also protected by a mutex. > >Is a spinlock better here and if so why? spinlock has lower overhead. If you don't need to sleep with the lock, spinlock is probably better for you. > >> b) why don't don't you take the lock just for list manipulation? > >Many code paths interact here, touching drivers and netdevs. There is an >ordering of locks being taken: > >1. nsim_bus_dev->dev.mutex >2. devlink->lock >3. rtnl_lock > >I was careful to avoid deadlocking by acquiring locks in the same order. >But looking at it again, I can reduce the critical section by acquiring >nsim_dev_list_lock after devlink->lock, thanks. > >> >> >>> devl_lock(devlink); >>> nsim_dev = devlink_priv(devlink); >>> nsim_dev->nsim_bus_dev = nsim_bus_dev; >>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> spin_lock_init(&nsim_dev->fa_cookie_lock); >>> >>> dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >>> + list_add(&nsim_dev->list, &nsim_dev_list); >>> >>> nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, >>> sizeof(struct nsim_vf_config), >>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> >>> nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> return 0; >>> >>> err_hwstats_exit: >>> @@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> { >>> struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); >>> struct devlink *devlink = priv_to_devlink(nsim_dev); >>> + struct nsim_dev *pos, *tmp; >>> >>> + mutex_lock(&nsim_dev_list_lock); >>> devl_lock(devlink); >>> + >>> + list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { >>> + if (pos == nsim_dev) { >>> + list_del(&nsim_dev->list); >>> + break; >>> + } >>> + } >>> + >>> nsim_dev_reload_destroy(nsim_dev); >>> >>> nsim_bpf_dev_exit(nsim_dev); >>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> kfree(nsim_dev->vfconfigs); >>> kfree(nsim_dev->fa_cookie); >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> devlink_free(devlink); >>> dev_set_drvdata(&nsim_bus_dev->dev, NULL); >>> } >>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h >>> index 028c825b86db..babb61d7790b 100644 >>> --- a/drivers/net/netdevsim/netdevsim.h >>> +++ b/drivers/net/netdevsim/netdevsim.h >>> @@ -277,6 +277,7 @@ struct nsim_vf_config { >>> >>> struct nsim_dev { >>> struct nsim_bus_dev *nsim_bus_dev; >>> + struct list_head list; >>> struct nsim_fib_data *fib_data; >>> struct nsim_trap_data *trap_data; >>> struct dentry *ddir; >>> -- >>> 2.39.3 >>>
Fri, Dec 22, 2023 at 01:45:58AM CET, dw@davidwei.uk wrote: >On 2023-12-20 00:57, Jiri Pirko wrote: >> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote: >>> This patch adds a linked list nsim_dev_list of probed netdevsims, added >>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex >>> nsim_dev_list_lock protects the list. >> >> In the commit message, you should use imperative mood, command >> the codebase what to do: >> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes > >Thanks, I didn't know about this. Will edit the commit messages. > >> >> >>> >>> Signed-off-by: David Wei <dw@davidwei.uk> >>> --- >>> drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ >>> drivers/net/netdevsim/netdevsim.h | 1 + >>> 2 files changed, 18 insertions(+) >>> >>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c >>> index b4d3b9cde8bd..e30a12130e07 100644 >>> --- a/drivers/net/netdevsim/dev.c >>> +++ b/drivers/net/netdevsim/dev.c >>> @@ -35,6 +35,9 @@ >>> >>> #include "netdevsim.h" >>> >>> +static LIST_HEAD(nsim_dev_list); >>> +static DEFINE_MUTEX(nsim_dev_list_lock); >>> + >>> static unsigned int >>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) >>> { >>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> nsim_bus_dev->initial_net, &nsim_bus_dev->dev); >>> if (!devlink) >>> return -ENOMEM; >>> + mutex_lock(&nsim_dev_list_lock); >> >> I don't follow. You claim you use this mutex to protect the list. >> a) why don't you use spin-lock? > >I'm using a mutex unless I know (or someone else who knows better point >out) that a spinlock is better. It is simple, there are fewer gotchas, >and I anticipate actual contention here to be near 0. The >nsim_bus_dev_list is also protected by a mutex. > >Is a spinlock better here and if so why? > >> b) why don't don't you take the lock just for list manipulation? > >Many code paths interact here, touching drivers and netdevs. There is an >ordering of locks being taken: > >1. nsim_bus_dev->dev.mutex >2. devlink->lock >3. rtnl_lock > >I was careful to avoid deadlocking by acquiring locks in the same order. >But looking at it again, I can reduce the critical section by acquiring >nsim_dev_list_lock after devlink->lock, thanks. Again, what is the purpose of the lock? I was under impression, that you just need to maintain consistency of the list. Or do you need it for anything else? > >> >> >>> devl_lock(devlink); >>> nsim_dev = devlink_priv(devlink); >>> nsim_dev->nsim_bus_dev = nsim_bus_dev; >>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> spin_lock_init(&nsim_dev->fa_cookie_lock); >>> >>> dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); >>> + list_add(&nsim_dev->list, &nsim_dev_list); >>> >>> nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, >>> sizeof(struct nsim_vf_config), >>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) >>> >>> nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> return 0; >>> >>> err_hwstats_exit: >>> @@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> { >>> struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); >>> struct devlink *devlink = priv_to_devlink(nsim_dev); >>> + struct nsim_dev *pos, *tmp; >>> >>> + mutex_lock(&nsim_dev_list_lock); >>> devl_lock(devlink); >>> + >>> + list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { >>> + if (pos == nsim_dev) { >>> + list_del(&nsim_dev->list); >>> + break; >>> + } >>> + } >>> + >>> nsim_dev_reload_destroy(nsim_dev); >>> >>> nsim_bpf_dev_exit(nsim_dev); >>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) >>> kfree(nsim_dev->vfconfigs); >>> kfree(nsim_dev->fa_cookie); >>> devl_unlock(devlink); >>> + mutex_unlock(&nsim_dev_list_lock); >>> devlink_free(devlink); >>> dev_set_drvdata(&nsim_bus_dev->dev, NULL); >>> } >>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h >>> index 028c825b86db..babb61d7790b 100644 >>> --- a/drivers/net/netdevsim/netdevsim.h >>> +++ b/drivers/net/netdevsim/netdevsim.h >>> @@ -277,6 +277,7 @@ struct nsim_vf_config { >>> >>> struct nsim_dev { >>> struct nsim_bus_dev *nsim_bus_dev; >>> + struct list_head list; >>> struct nsim_fib_data *fib_data; >>> struct nsim_trap_data *trap_data; >>> struct dentry *ddir; >>> -- >>> 2.39.3 >>>
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index b4d3b9cde8bd..e30a12130e07 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -35,6 +35,9 @@ #include "netdevsim.h" +static LIST_HEAD(nsim_dev_list); +static DEFINE_MUTEX(nsim_dev_list_lock); + static unsigned int nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index) { @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) nsim_bus_dev->initial_net, &nsim_bus_dev->dev); if (!devlink) return -ENOMEM; + mutex_lock(&nsim_dev_list_lock); devl_lock(devlink); nsim_dev = devlink_priv(devlink); nsim_dev->nsim_bus_dev = nsim_bus_dev; @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) spin_lock_init(&nsim_dev->fa_cookie_lock); dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev); + list_add(&nsim_dev->list, &nsim_dev_list); nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs, sizeof(struct nsim_vf_config), @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev) nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY; devl_unlock(devlink); + mutex_unlock(&nsim_dev_list_lock); return 0; err_hwstats_exit: @@ -1668,8 +1674,18 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) { struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev); struct devlink *devlink = priv_to_devlink(nsim_dev); + struct nsim_dev *pos, *tmp; + mutex_lock(&nsim_dev_list_lock); devl_lock(devlink); + + list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) { + if (pos == nsim_dev) { + list_del(&nsim_dev->list); + break; + } + } + nsim_dev_reload_destroy(nsim_dev); nsim_bpf_dev_exit(nsim_dev); @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev) kfree(nsim_dev->vfconfigs); kfree(nsim_dev->fa_cookie); devl_unlock(devlink); + mutex_unlock(&nsim_dev_list_lock); devlink_free(devlink); dev_set_drvdata(&nsim_bus_dev->dev, NULL); } diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index 028c825b86db..babb61d7790b 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -277,6 +277,7 @@ struct nsim_vf_config { struct nsim_dev { struct nsim_bus_dev *nsim_bus_dev; + struct list_head list; struct nsim_fib_data *fib_data; struct nsim_trap_data *trap_data; struct dentry *ddir;
This patch adds a linked list nsim_dev_list of probed netdevsims, added during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex nsim_dev_list_lock protects the list. Signed-off-by: David Wei <dw@davidwei.uk> --- drivers/net/netdevsim/dev.c | 17 +++++++++++++++++ drivers/net/netdevsim/netdevsim.h | 1 + 2 files changed, 18 insertions(+)