From: Leon Romanovsky <leon@kernel.org>
To: Jiri Pirko <jiri@resnulli.us>
Cc: "David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, Jiri Pirko <jiri@nvidia.com>,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Parav Pandit <parav@nvidia.com>
Subject: Re: [PATCH net-next 1/2] devlink: Break parameter notification sequence to be before/after unload/load driver
Date: Thu, 29 Jul 2021 15:11:28 +0300 [thread overview]
Message-ID: <YQKa8DqBS/UPVLaq@unreal> (raw)
In-Reply-To: <YQKXjTZ0W04L7xqG@nanopsycho>
On Thu, Jul 29, 2021 at 01:57:01PM +0200, Jiri Pirko wrote:
> Thu, Jul 29, 2021 at 01:35:55PM CEST, leon@kernel.org wrote:
> >On Thu, Jul 29, 2021 at 01:22:58PM +0200, Jiri Pirko wrote:
> >> Thu, Jul 29, 2021 at 10:15:25AM CEST, leon@kernel.org wrote:
> >> >From: Leon Romanovsky <leonro@nvidia.com>
> >
> ><...>
> >
> >> >diff --git a/net/core/devlink.c b/net/core/devlink.c
> >> >index b596a971b473..54e2a0375539 100644
> >> >--- a/net/core/devlink.c
> >> >+++ b/net/core/devlink.c
> >> >@@ -3801,8 +3801,9 @@ static void devlink_param_notify(struct devlink *devlink,
> >> > struct devlink_param_item *param_item,
> >> > enum devlink_command cmd);
> >> >
> >> >-static void devlink_reload_netns_change(struct devlink *devlink,
> >> >- struct net *dest_net)
> >> >+static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
> >>
> >> Please name it differently. This function notifies not only the params,
> >> but the devlink instance itself as well.
> >
> >I'm open for suggestion. What did you have in mind?
>
> devlink_ns_change_notify?
NP, will change
>
> >
> >>
> >>
> >> >+ struct net *curr_net,
> >> >+ enum devlink_command cmd)
> >> > {
> >> > struct devlink_param_item *param_item;
> >> >
> >> >@@ -3812,17 +3813,17 @@ static void devlink_reload_netns_change(struct devlink *devlink,
> >> > * reload process so the notifications are generated separatelly.
> >> > */
> >> >
> >> >- 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 (!dest_net || net_eq(dest_net, curr_net))
> >> >+ return;
> >> >
> >> >- __devlink_net_set(devlink, dest_net);
> >> >+ if (cmd == DEVLINK_CMD_PARAM_NEW)
> >> >+ devlink_notify(devlink, DEVLINK_CMD_NEW);
> >>
> >> This is quite odd. According to PARAMS cmd you decife devlink CMD.
> >>
> >> Just have bool arg which would help you select both
> >> DEVLINK_CMD_PARAM_NEW/DEL and DEVLINK_CMD_NEW/DEL
> >
> >The patch is quite misleading, but the final result looks neat:
> >
> > 3847 static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
> > 3848 struct net *curr_net,
> > 3849 enum devlink_command cmd)
> > 3850 {
> > 3851 struct devlink_param_item *param_item;
> > 3852
> > 3853 /* Userspace needs to be notified about devlink objects
> > 3854 * removed from original and entering new network namespace.
> > 3855 * The rest of the devlink objects are re-created during
> > 3856 * reload process so the notifications are generated separatelly.
> > 3857 */
> > 3858
> > 3859 if (!dest_net || net_eq(dest_net, curr_net))
> > 3860 return;
> > 3861
> > 3862 if (cmd == DEVLINK_CMD_PARAM_NEW)
> > 3863 devlink_notify(devlink, DEVLINK_CMD_NEW);
>
> Nothing misleading here. This is exactly what I'm pointing out...
Do you ask for this change?
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 077310c26a8b..ee7204787b29 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -3846,7 +3846,7 @@ static void devlink_param_notify(struct devlink *devlink,
static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
struct net *curr_net,
- enum devlink_command cmd)
+ enum devlink_command cmd, bool new)
{
struct devlink_param_item *param_item;
@@ -3859,13 +3859,13 @@ static void devlink_params_notify(struct devlink *devlink, struct net *dest_net,
if (!dest_net || net_eq(dest_net, curr_net))
return;
- if (cmd == DEVLINK_CMD_PARAM_NEW)
+ if (new)
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 (cmd == DEVLINK_CMD_PARAM_DEL)
+ if (!new)
devlink_notify(devlink, DEVLINK_CMD_DEL);
}
@@ -3957,7 +3957,7 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
curr_net = devlink_net(devlink);
devlink_params_notify(devlink, dest_net, curr_net,
- DEVLINK_CMD_PARAM_DEL);
+ DEVLINK_CMD_PARAM_DEL, false);
err = devlink->ops->reload_down(devlink, !!dest_net, action, limit, extack);
if (err)
return err;
@@ -3971,7 +3971,7 @@ static int devlink_reload(struct devlink *devlink, struct net *dest_net,
return err;
devlink_params_notify(devlink, dest_net, curr_net,
- DEVLINK_CMD_PARAM_NEW);
+ DEVLINK_CMD_PARAM_NEW, 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,
>
>
>
> > 3864
> > 3865 list_for_each_entry(param_item, &devlink->param_list, list)
> > 3866 devlink_param_notify(devlink, 0, param_item, cmd);
> > 3867
> > 3868 if (cmd == DEVLINK_CMD_PARAM_DEL)
> > 3869 devlink_notify(devlink, DEVLINK_CMD_DEL);
> > 3870 }
> >
> >
> >So as you can see in line 3866, we anyway will need to provide "cmd", so
> >do you suggest to add extra two bool variables to the function signature
> >to avoid "cmd == DEVLINK_CMD_PARAM_NEW" and "cmd == DEVLINK_CMD_PARAM_DEL" ifs?
> >
> >Thanks
next prev parent reply other threads:[~2021-07-29 12:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-29 8:15 [PATCH net-next 0/2] Clean devlink net namespace operations Leon Romanovsky
2021-07-29 8:15 ` [PATCH net-next 1/2] devlink: Break parameter notification sequence to be before/after unload/load driver Leon Romanovsky
2021-07-29 11:22 ` Jiri Pirko
2021-07-29 11:35 ` Leon Romanovsky
2021-07-29 11:57 ` Jiri Pirko
2021-07-29 12:11 ` Leon Romanovsky [this message]
2021-07-29 8:15 ` [PATCH net-next 2/2] devlink: Allocate devlink directly in requested net namespace Leon Romanovsky
2021-07-29 11:55 ` Jiri Pirko
2021-07-29 12:06 ` Leon Romanovsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YQKa8DqBS/UPVLaq@unreal \
--to=leon@kernel.org \
--cc=davem@davemloft.net \
--cc=jiri@nvidia.com \
--cc=jiri@resnulli.us \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=parav@nvidia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.