Message ID | 20230127155042.1846608-2-jiri@resnulli.us (mailing list archive) |
---|---|
State | Accepted |
Commit | 7d7e9169a3ecdc14a4921b2130066ce26952c9e1 |
Delegated to: | Netdev Maintainers |
Headers | show |
Series | devlink: fix reload notifications and remove features | expand |
Context | Check | Description |
---|---|---|
netdev/tree_selection | success | Clearly marked for net-next |
netdev/fixes_present | success | Fixes tag not required for -next series |
netdev/subject_prefix | success | Link |
netdev/cover_letter | success | Series has a cover letter |
netdev/patch_count | success | Link |
netdev/header_inline | success | No static functions without inline keyword in header files |
netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
netdev/build_clang | success | Errors and warnings before: 1 this patch: 1 |
netdev/module_param | success | Was 0 now: 0 |
netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
netdev/check_selftest | success | No net selftest shell script |
netdev/verify_fixes | success | No Fixes tag |
netdev/build_allmodconfig_warn | success | Errors and warnings before: 2 this patch: 2 |
netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 70 lines checked |
netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
netdev/source_inline | success | Was 0 now: 0 |
On 1/27/2023 7:50 AM, Jiri Pirko wrote: > From: Jiri Pirko <jiri@nvidia.com> > > This effectively reverts commit 05a7f4a8dff1 ("devlink: Break parameter > notification sequence to be before/after unload/load driver"). > > Cited commit resolved a problem in mlx5 params implementation, > when param notification code accessed memory previously freed > during reload. > > Now, when the params can be registered and unregistered when devlink > instance is registered, mlx5 code unregisters the problematic param > during devlink reload. The fix is therefore no longer needed. > > Current behavior is a it problematic, as it sends DEL notifications even > in potential case when reload_down() call fails which might confuse > userspace notifications listener. > > So move the reload notifications back where they were originally in > between reload_down() and reload_up() calls. > > Signed-off-by: Jiri Pirko <jiri@nvidia.com> > --- Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > net/devlink/leftover.c | 37 ++++++++++++++++--------------------- > 1 file changed, 16 insertions(+), 21 deletions(-) > > diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c > index bd4c5d2dd612..24e20861a28b 100644 > --- a/net/devlink/leftover.c > +++ b/net/devlink/leftover.c > @@ -4235,12 +4235,11 @@ static void devlink_param_notify(struct devlink *devlink, > struct devlink_param_item *param_item, > enum devlink_command cmd); > > -static void devlink_ns_change_notify(struct devlink *devlink, > - struct net *dest_net, struct net *curr_net, > - bool new) > +static void devlink_reload_netns_change(struct devlink *devlink, > + struct net *curr_net, > + struct net *dest_net) > { > struct devlink_param_item *param_item; > - enum devlink_command cmd; > > /* Userspace needs to be notified about devlink objects > * removed from original and entering new network namespace. > @@ -4248,18 +4247,19 @@ static void devlink_ns_change_notify(struct devlink *devlink, > * reload process so the notifications are generated separatelly. > */ > > - if (!dest_net || net_eq(dest_net, curr_net)) > - return; > + list_for_each_entry(param_item, &devlink->param_list, list) > + devlink_param_notify(devlink, 0, param_item, > + DEVLINK_CMD_PARAM_DEL); > + devlink_notify(devlink, DEVLINK_CMD_DEL); > > - if (new) > - devlink_notify(devlink, DEVLINK_CMD_NEW); > + move_netdevice_notifier_net(curr_net, dest_net, > + &devlink->netdevice_nb); > + write_pnet(&devlink->_net, dest_net); > > - cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL; > + devlink_notify(devlink, DEVLINK_CMD_NEW); > list_for_each_entry(param_item, &devlink->param_list, list) > - devlink_param_notify(devlink, 0, param_item, cmd); > - > - if (!new) > - devlink_notify(devlink, DEVLINK_CMD_DEL); > + devlink_param_notify(devlink, 0, param_item, > + DEVLINK_CMD_PARAM_NEW); > } > > static void devlink_reload_failed_set(struct devlink *devlink, > @@ -4341,24 +4341,19 @@ int devlink_reload(struct devlink *devlink, struct net *dest_net, > memcpy(remote_reload_stats, devlink->stats.remote_reload_stats, > sizeof(remote_reload_stats)); > > - curr_net = devlink_net(devlink); > - devlink_ns_change_notify(devlink, dest_net, curr_net, false); > err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); > if (err) > return err; > > - if (dest_net && !net_eq(dest_net, curr_net)) { > - move_netdevice_notifier_net(curr_net, dest_net, > - &devlink->netdevice_nb); > - write_pnet(&devlink->_net, dest_net); > - } > + curr_net = devlink_net(devlink); > + if (dest_net && !net_eq(dest_net, curr_net)) > + devlink_reload_netns_change(devlink, curr_net, dest_net); > > err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack); > devlink_reload_failed_set(devlink, !!err); > if (err) > return err; > > - devlink_ns_change_notify(devlink, dest_net, curr_net, true); > WARN_ON(!(*actions_performed & BIT(action))); > /* Catch driver on updating the remote action within devlink reload */ > WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c index bd4c5d2dd612..24e20861a28b 100644 --- a/net/devlink/leftover.c +++ b/net/devlink/leftover.c @@ -4235,12 +4235,11 @@ static void devlink_param_notify(struct devlink *devlink, struct devlink_param_item *param_item, enum devlink_command cmd); -static void devlink_ns_change_notify(struct devlink *devlink, - struct net *dest_net, struct net *curr_net, - bool new) +static void devlink_reload_netns_change(struct devlink *devlink, + struct net *curr_net, + struct net *dest_net) { struct devlink_param_item *param_item; - enum devlink_command cmd; /* Userspace needs to be notified about devlink objects * removed from original and entering new network namespace. @@ -4248,18 +4247,19 @@ static void devlink_ns_change_notify(struct devlink *devlink, * reload process so the notifications are generated separatelly. */ - if (!dest_net || net_eq(dest_net, curr_net)) - return; + list_for_each_entry(param_item, &devlink->param_list, list) + devlink_param_notify(devlink, 0, param_item, + DEVLINK_CMD_PARAM_DEL); + devlink_notify(devlink, DEVLINK_CMD_DEL); - if (new) - devlink_notify(devlink, DEVLINK_CMD_NEW); + move_netdevice_notifier_net(curr_net, dest_net, + &devlink->netdevice_nb); + write_pnet(&devlink->_net, dest_net); - cmd = new ? DEVLINK_CMD_PARAM_NEW : DEVLINK_CMD_PARAM_DEL; + devlink_notify(devlink, DEVLINK_CMD_NEW); list_for_each_entry(param_item, &devlink->param_list, list) - devlink_param_notify(devlink, 0, param_item, cmd); - - if (!new) - devlink_notify(devlink, DEVLINK_CMD_DEL); + devlink_param_notify(devlink, 0, param_item, + DEVLINK_CMD_PARAM_NEW); } static void devlink_reload_failed_set(struct devlink *devlink, @@ -4341,24 +4341,19 @@ int devlink_reload(struct devlink *devlink, struct net *dest_net, memcpy(remote_reload_stats, devlink->stats.remote_reload_stats, sizeof(remote_reload_stats)); - curr_net = devlink_net(devlink); - devlink_ns_change_notify(devlink, dest_net, curr_net, false); err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack); if (err) return err; - if (dest_net && !net_eq(dest_net, curr_net)) { - move_netdevice_notifier_net(curr_net, dest_net, - &devlink->netdevice_nb); - write_pnet(&devlink->_net, dest_net); - } + curr_net = devlink_net(devlink); + if (dest_net && !net_eq(dest_net, curr_net)) + devlink_reload_netns_change(devlink, curr_net, dest_net); err = devlink->ops->reload_up(devlink, action, limit, actions_performed, extack); devlink_reload_failed_set(devlink, !!err); if (err) return err; - devlink_ns_change_notify(devlink, dest_net, curr_net, true); WARN_ON(!(*actions_performed & BIT(action))); /* Catch driver on updating the remote action within devlink reload */ WARN_ON(memcmp(remote_reload_stats, devlink->stats.remote_reload_stats,