| Message ID | 20221217011953.152487-2-kuba@kernel.org (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | devlink: remove the wait-for-references on unregister | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote: >We use a clever find_first() / find_after() scheme currently, >which works nicely as xarray will write the "current" index >into the variable we pass. > >We can't do the same thing during the "dump walk" because >there we must not increment the index until we're sure >that the instance has been fully dumped. To be honest, this "we something" desctiption style makes things quite hard to understand. Could you please rephrase it to actually talk about the entities in code? > >Since we have a precedent and a requirement for manually futzing >with the increment of the index, let's switch >devlinks_xa_for_each_registered_get() to do the same thing. >It removes some indirections. > >Signed-off-by: Jakub Kicinski <kuba@kernel.org> >--- > net/devlink/core.c | 31 +++++++++---------------------- > net/devlink/devl_internal.h | 17 ++++------------- > 2 files changed, 13 insertions(+), 35 deletions(-) > >diff --git a/net/devlink/core.c b/net/devlink/core.c >index 371d6821315d..88c88b8053e2 100644 >--- a/net/devlink/core.c >+++ b/net/devlink/core.c >@@ -91,16 +91,13 @@ void devlink_put(struct devlink *devlink) > call_rcu(&devlink->rcu, __devlink_put_rcu); > } > >-struct devlink * >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, >- void * (*xa_find_fn)(struct xarray *, unsigned long *, >- unsigned long, xa_mark_t)) >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp) > { >- struct devlink *devlink; >+ struct devlink *devlink = NULL; > > rcu_read_lock(); > retry: >- devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); >+ devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); > if (!devlink) > goto unlock; > >@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, > * This prevents live-lock of devlink_unregister() wait for completion. > */ > if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING)) >- goto retry; >+ goto next; > >- /* For a possible retry, the xa_find_after() should be always used */ >- xa_find_fn = xa_find_after; Hmm. Any idea why xa_find_after()? implementation is different to xa_find()? > if (!devlink_try_get(devlink)) >- goto retry; >+ goto next; > if (!net_eq(devlink_net(devlink), net)) { > devlink_put(devlink); >- goto retry; >+ goto next; > } > unlock: > rcu_read_unlock(); > return devlink; >-} >- >-struct devlink * >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp) >-{ >- return devlinks_xa_find_get(net, indexp, xa_find); >-} > >-struct devlink * >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp) >-{ >- return devlinks_xa_find_get(net, indexp, xa_find_after); >+next: >+ (*indexp)++; >+ goto retry; > } > > /** >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h >index 1d7ab11f2f7e..ef0369449592 100644 >--- a/net/devlink/devl_internal.h >+++ b/net/devlink/devl_internal.h >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family; > * in loop body in order to release the reference. > */ > #define devlinks_xa_for_each_registered_get(net, index, devlink) \ >- for (index = 0, \ >- devlink = devlinks_xa_find_get_first(net, &index); \ >- devlink; devlink = devlinks_xa_find_get_next(net, &index)) >- >-struct devlink * >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, >- void * (*xa_find_fn)(struct xarray *, unsigned long *, >- unsigned long, xa_mark_t)); >-struct devlink * >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp); >-struct devlink * >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp); >+ for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++) You don't need ()' in the 2nd for arg. >+ >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp); > > /* Netlink */ > #define DEVLINK_NL_FLAG_NEED_PORT BIT(0) >@@ -133,7 +124,7 @@ struct devlink_gen_cmd { > */ > #define devlink_dump_for_each_instance_get(msg, dump, devlink) \ > for (; (devlink = devlinks_xa_find_get(sock_net(msg->sk), \ >- &dump->instance, xa_find)); \ >+ &dump->instance)); \ > dump->instance++, dump->idx = 0) > > extern const struct genl_small_ops devlink_nl_ops[56]; >-- >2.38.1 >
On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote: > Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote: > >We use a clever find_first() / find_after() scheme currently, > >which works nicely as xarray will write the "current" index > >into the variable we pass. > > > >We can't do the same thing during the "dump walk" because > >there we must not increment the index until we're sure > >that the instance has been fully dumped. > > To be honest, this "we something" desctiption style makes things quite > hard to understand. Could you please rephrase it to actually talk > about the entities in code? Could you be more specific? I'm probably misunderstanding but it sounds to me like you're asking me to describe what I'm doing rather than the background and motivation. > >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h > >index 1d7ab11f2f7e..ef0369449592 100644 > >--- a/net/devlink/devl_internal.h > >+++ b/net/devlink/devl_internal.h > >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family; > > * in loop body in order to release the reference. > > */ > > #define devlinks_xa_for_each_registered_get(net, index, devlink) \ > >- for (index = 0, \ > >- devlink = devlinks_xa_find_get_first(net, &index); \ > >- devlink; devlink = devlinks_xa_find_get_next(net, &index)) > >- > >-struct devlink * > >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, > >- void * (*xa_find_fn)(struct xarray *, unsigned long *, > >- unsigned long, xa_mark_t)); > >-struct devlink * > >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp); > >-struct devlink * > >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp); > >+ for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++) > > You don't need ()' in the 2nd for arg. Pretty sure I was getting a compiler warning for assignment in a condition....
On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote: > >-struct devlink * > >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, > >- void * (*xa_find_fn)(struct xarray *, unsigned long *, > >- unsigned long, xa_mark_t)) > >+struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp) > > { > >- struct devlink *devlink; > >+ struct devlink *devlink = NULL; > > > > rcu_read_lock(); > > retry: > >- devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); > >+ devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); > > if (!devlink) > > goto unlock; > > > >@@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, > > * This prevents live-lock of devlink_unregister() wait for completion. > > */ > > if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING)) > >- goto retry; > >+ goto next; > > > >- /* For a possible retry, the xa_find_after() should be always used */ > >- xa_find_fn = xa_find_after; > > Hmm. Any idea why xa_find_after()? implementation is different to > xa_find()? I'm guessing it's because for _after the code needs to take special care of skipping multi-index entries. If an entry spans 0-3 xa_find(0) can return the same entry as xa_find(1). But xa_find_after(1) must return an entry under index 4 or higher. Since our use of the Xarray is very trivial with no range indexes, it should not matter. Let me CC Matthew, just in case. The question boils down to whether: for (index = 0; (entry = xa_find(net, &index)); index++) is a legal way of iterating over an Xarray without range indexes. > > if (!devlink_try_get(devlink)) > >- goto retry; > >+ goto next; > > if (!net_eq(devlink_net(devlink), net)) { > > devlink_put(devlink); > >- goto retry; > >+ goto next; > > } > > unlock: > > rcu_read_unlock(); > > return devlink; > >-} > >- > >-struct devlink * > >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp) > >-{ > >- return devlinks_xa_find_get(net, indexp, xa_find); > >-} > > > >-struct devlink * > >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp) > >-{ > >- return devlinks_xa_find_get(net, indexp, xa_find_after); > >+next: > >+ (*indexp)++; > >+ goto retry; > > } > > > > /** > >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h > >index 1d7ab11f2f7e..ef0369449592 100644 > >--- a/net/devlink/devl_internal.h > >+++ b/net/devlink/devl_internal.h > >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family; > > * in loop body in order to release the reference. > > */ > > #define devlinks_xa_for_each_registered_get(net, index, devlink) \ > >- for (index = 0, \ > >- devlink = devlinks_xa_find_get_first(net, &index); \ > >- devlink; devlink = devlinks_xa_find_get_next(net, &index)) > >- > >-struct devlink * > >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, > >- void * (*xa_find_fn)(struct xarray *, unsigned long *, > >- unsigned long, xa_mark_t)); > >-struct devlink * > >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp); > >-struct devlink * > >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp); > >+ for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++)
Mon, Jan 02, 2023 at 11:48:13PM CET, kuba@kernel.org wrote: >On Mon, 2 Jan 2023 14:24:39 +0100 Jiri Pirko wrote: >> Sat, Dec 17, 2022 at 02:19:44AM CET, kuba@kernel.org wrote: >> >We use a clever find_first() / find_after() scheme currently, >> >which works nicely as xarray will write the "current" index >> >into the variable we pass. >> > >> >We can't do the same thing during the "dump walk" because >> >there we must not increment the index until we're sure >> >that the instance has been fully dumped. >> >> To be honest, this "we something" desctiption style makes things quite >> hard to understand. Could you please rephrase it to actually talk >> about the entities in code? > >Could you be more specific? I'm probably misunderstanding but it sounds >to me like you're asking me to describe what I'm doing rather than >the background and motivation. "We" is us, you me and other people. It is weird to talk about the code and what there as "we do", "we use" etc. It's quite confusing as the reader it not able to distinguish if you are talking about code or people (developers in this case). I'm just askin if it is possible to make the descriptions easier to understand. > >> >diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h >> >index 1d7ab11f2f7e..ef0369449592 100644 >> >--- a/net/devlink/devl_internal.h >> >+++ b/net/devlink/devl_internal.h >> >@@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family; >> > * in loop body in order to release the reference. >> > */ >> > #define devlinks_xa_for_each_registered_get(net, index, devlink) \ >> >- for (index = 0, \ >> >- devlink = devlinks_xa_find_get_first(net, &index); \ >> >- devlink; devlink = devlinks_xa_find_get_next(net, &index)) >> >- >> >-struct devlink * >> >-devlinks_xa_find_get(struct net *net, unsigned long *indexp, >> >- void * (*xa_find_fn)(struct xarray *, unsigned long *, >> >- unsigned long, xa_mark_t)); >> >-struct devlink * >> >-devlinks_xa_find_get_first(struct net *net, unsigned long *indexp); >> >-struct devlink * >> >-devlinks_xa_find_get_next(struct net *net, unsigned long *indexp); >> >+ for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++) >> >> You don't need ()' in the 2nd for arg. > >Pretty sure I was getting a compiler warning for assignment in >a condition.... I see.
On Tue, 3 Jan 2023 08:35:16 +0100 Jiri Pirko wrote: > >> To be honest, this "we something" desctiption style makes things quite > >> hard to understand. Could you please rephrase it to actually talk > >> about the entities in code? > > > >Could you be more specific? I'm probably misunderstanding but it sounds > >to me like you're asking me to describe what I'm doing rather than > >the background and motivation. > > "We" is us, you me and other people. It is weird to talk about the code > and what there as "we do", "we use" etc. It's quite confusing as the > reader it not able to distinguish if you are talking about code or > people (developers in this case). I'm just askin if it is possible > to make the descriptions easier to understand. Actually thinking about it again, this patch is not strictly necessary. I'll rewrite the commit message to just say it's a simplification based on the fact we don't have multi-index entries.
diff --git a/net/devlink/core.c b/net/devlink/core.c index 371d6821315d..88c88b8053e2 100644 --- a/net/devlink/core.c +++ b/net/devlink/core.c @@ -91,16 +91,13 @@ void devlink_put(struct devlink *devlink) call_rcu(&devlink->rcu, __devlink_put_rcu); } -struct devlink * -devlinks_xa_find_get(struct net *net, unsigned long *indexp, - void * (*xa_find_fn)(struct xarray *, unsigned long *, - unsigned long, xa_mark_t)) +struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp) { - struct devlink *devlink; + struct devlink *devlink = NULL; rcu_read_lock(); retry: - devlink = xa_find_fn(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); + devlink = xa_find(&devlinks, indexp, ULONG_MAX, DEVLINK_REGISTERED); if (!devlink) goto unlock; @@ -109,31 +106,21 @@ devlinks_xa_find_get(struct net *net, unsigned long *indexp, * This prevents live-lock of devlink_unregister() wait for completion. */ if (xa_get_mark(&devlinks, *indexp, DEVLINK_UNREGISTERING)) - goto retry; + goto next; - /* For a possible retry, the xa_find_after() should be always used */ - xa_find_fn = xa_find_after; if (!devlink_try_get(devlink)) - goto retry; + goto next; if (!net_eq(devlink_net(devlink), net)) { devlink_put(devlink); - goto retry; + goto next; } unlock: rcu_read_unlock(); return devlink; -} - -struct devlink * -devlinks_xa_find_get_first(struct net *net, unsigned long *indexp) -{ - return devlinks_xa_find_get(net, indexp, xa_find); -} -struct devlink * -devlinks_xa_find_get_next(struct net *net, unsigned long *indexp) -{ - return devlinks_xa_find_get(net, indexp, xa_find_after); +next: + (*indexp)++; + goto retry; } /** diff --git a/net/devlink/devl_internal.h b/net/devlink/devl_internal.h index 1d7ab11f2f7e..ef0369449592 100644 --- a/net/devlink/devl_internal.h +++ b/net/devlink/devl_internal.h @@ -82,18 +82,9 @@ extern struct genl_family devlink_nl_family; * in loop body in order to release the reference. */ #define devlinks_xa_for_each_registered_get(net, index, devlink) \ - for (index = 0, \ - devlink = devlinks_xa_find_get_first(net, &index); \ - devlink; devlink = devlinks_xa_find_get_next(net, &index)) - -struct devlink * -devlinks_xa_find_get(struct net *net, unsigned long *indexp, - void * (*xa_find_fn)(struct xarray *, unsigned long *, - unsigned long, xa_mark_t)); -struct devlink * -devlinks_xa_find_get_first(struct net *net, unsigned long *indexp); -struct devlink * -devlinks_xa_find_get_next(struct net *net, unsigned long *indexp); + for (index = 0; (devlink = devlinks_xa_find_get(net, &index)); index++) + +struct devlink *devlinks_xa_find_get(struct net *net, unsigned long *indexp); /* Netlink */ #define DEVLINK_NL_FLAG_NEED_PORT BIT(0) @@ -133,7 +124,7 @@ struct devlink_gen_cmd { */ #define devlink_dump_for_each_instance_get(msg, dump, devlink) \ for (; (devlink = devlinks_xa_find_get(sock_net(msg->sk), \ - &dump->instance, xa_find)); \ + &dump->instance)); \ dump->instance++, dump->idx = 0) extern const struct genl_small_ops devlink_nl_ops[56];
We use a clever find_first() / find_after() scheme currently, which works nicely as xarray will write the "current" index into the variable we pass. We can't do the same thing during the "dump walk" because there we must not increment the index until we're sure that the instance has been fully dumped. Since we have a precedent and a requirement for manually futzing with the increment of the index, let's switch devlinks_xa_for_each_registered_get() to do the same thing. It removes some indirections. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- net/devlink/core.c | 31 +++++++++---------------------- net/devlink/devl_internal.h | 17 ++++------------- 2 files changed, 13 insertions(+), 35 deletions(-)