| Message ID | 1599060734-26617-4-git-send-email-ayal@mellanox.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Add devlink traps in devlink port context | expand |
On Wed, Sep 02, 2020 at 06:32:13PM +0300, Aya Levin wrote: > Managing large scale port's traps may be complicated. This patch > introduces a shortcut: when setting a trap on a device and this trap is > not registered on this device, the action will take place on all related > ports that did register this trap. I'm not really a fan of this and I'm not sure there is precedent for something similar. Also, it's an optimization, so I wouldn't put it as part of the first submission before you gather some operational experience with the initial interface. In addition, I find it very unintuitive for users. When I do 'devlink trap show' I will not see anything. I will only see the traps when I issue 'devlink port trap show', yet 'devlink trap set ...' is expected to work. Lets assume that this is a valid change, it would be better implemented with my suggestion from the previous patch: When devlink sees that a trap is registered on all the ports it can auto-register a new per-device trap and user space gets the appropriate notification. > > Signed-off-by: Aya Levin <ayal@mellanox.com> > --- > net/core/devlink.c | 43 +++++++++++++++++++++++++++++++++---------- > 1 file changed, 33 insertions(+), 10 deletions(-) > > diff --git a/net/core/devlink.c b/net/core/devlink.c > index b13e1b40bf1c..dea5482b2517 100644 > --- a/net/core/devlink.c > +++ b/net/core/devlink.c > @@ -6501,23 +6501,46 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb, > struct devlink *devlink = info->user_ptr[0]; > struct devlink_trap_mngr *trap_mngr; > struct devlink_trap_item *trap_item; > + struct devlink_port *devlink_port; > int err; > > - trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info); > - if (list_empty(&trap_mngr->trap_list)) > - return -EOPNOTSUPP; > + devlink_port = devlink_port_get_from_attrs(devlink, info->attrs); > + if (IS_ERR(devlink_port)) { > + trap_mngr = &devlink->trap_mngr; > + if (list_empty(&trap_mngr->trap_list)) > + goto loop_over_ports; > > - trap_item = devlink_trap_item_get_from_info(trap_mngr, info); > - if (!trap_item) { > - NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap"); > - return -ENOENT; > + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); > + if (!trap_item) > + goto loop_over_ports; > + } else { > + trap_mngr = &devlink_port->trap_mngr; > + if (list_empty(&trap_mngr->trap_list)) > + return -EOPNOTSUPP; > + > + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); > + if (!trap_item) { > + NL_SET_ERR_MSG_MOD(extack, "Port did not register this trap"); > + return -ENOENT; > + } > } > return devlink_trap_action_set(devlink, trap_mngr, trap_item, info); > > - err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); > - if (err) > - return err; > +loop_over_ports: > + if (list_empty(&devlink->port_list)) > + return -EOPNOTSUPP; > + list_for_each_entry(devlink_port, &devlink->port_list, list) { > + trap_mngr = &devlink_port->trap_mngr; > + if (list_empty(&trap_mngr->trap_list)) > + continue; > > + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); > + if (!trap_item) > + continue; > + err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); > + if (err) > + return err; > + } > return 0; > } > > -- > 2.14.1 >
On 9/6/2020 6:58 PM, Ido Schimmel wrote: > On Wed, Sep 02, 2020 at 06:32:13PM +0300, Aya Levin wrote: >> Managing large scale port's traps may be complicated. This patch >> introduces a shortcut: when setting a trap on a device and this trap is >> not registered on this device, the action will take place on all related >> ports that did register this trap. > > I'm not really a fan of this and I'm not sure there is precedent for > something similar. Also, it's an optimization, so I wouldn't put it as > part of the first submission before you gather some operational > experience with the initial interface. I thought it was a nice idea but I agree that this is an optimization and can be dropped from preliminary submission > > In addition, I find it very unintuitive for users. When I do 'devlink > trap show' I will not see anything. I will only see the traps when I > issue 'devlink port trap show', yet 'devlink trap set ...' is expected > to work. > > Lets assume that this is a valid change, it would be better implemented > with my suggestion from the previous patch: When devlink sees that a > trap is registered on all the ports it can auto-register a new > per-device trap and user space gets the appropriate notification. > >> >> Signed-off-by: Aya Levin <ayal@mellanox.com> >> --- >> net/core/devlink.c | 43 +++++++++++++++++++++++++++++++++---------- >> 1 file changed, 33 insertions(+), 10 deletions(-) >> >> diff --git a/net/core/devlink.c b/net/core/devlink.c >> index b13e1b40bf1c..dea5482b2517 100644 >> --- a/net/core/devlink.c >> +++ b/net/core/devlink.c >> @@ -6501,23 +6501,46 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb, >> struct devlink *devlink = info->user_ptr[0]; >> struct devlink_trap_mngr *trap_mngr; >> struct devlink_trap_item *trap_item; >> + struct devlink_port *devlink_port; >> int err; >> >> - trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info); >> - if (list_empty(&trap_mngr->trap_list)) >> - return -EOPNOTSUPP; >> + devlink_port = devlink_port_get_from_attrs(devlink, info->attrs); >> + if (IS_ERR(devlink_port)) { >> + trap_mngr = &devlink->trap_mngr; >> + if (list_empty(&trap_mngr->trap_list)) >> + goto loop_over_ports; >> >> - trap_item = devlink_trap_item_get_from_info(trap_mngr, info); >> - if (!trap_item) { >> - NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap"); >> - return -ENOENT; >> + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); >> + if (!trap_item) >> + goto loop_over_ports; >> + } else { >> + trap_mngr = &devlink_port->trap_mngr; >> + if (list_empty(&trap_mngr->trap_list)) >> + return -EOPNOTSUPP; >> + >> + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); >> + if (!trap_item) { >> + NL_SET_ERR_MSG_MOD(extack, "Port did not register this trap"); >> + return -ENOENT; >> + } >> } >> return devlink_trap_action_set(devlink, trap_mngr, trap_item, info); >> >> - err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); >> - if (err) >> - return err; >> +loop_over_ports: >> + if (list_empty(&devlink->port_list)) >> + return -EOPNOTSUPP; >> + list_for_each_entry(devlink_port, &devlink->port_list, list) { >> + trap_mngr = &devlink_port->trap_mngr; >> + if (list_empty(&trap_mngr->trap_list)) >> + continue; >> >> + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); >> + if (!trap_item) >> + continue; >> + err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); >> + if (err) >> + return err; >> + } >> return 0; >> } >> >> -- >> 2.14.1 >>
diff --git a/net/core/devlink.c b/net/core/devlink.c index b13e1b40bf1c..dea5482b2517 100644 --- a/net/core/devlink.c +++ b/net/core/devlink.c @@ -6501,23 +6501,46 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb, struct devlink *devlink = info->user_ptr[0]; struct devlink_trap_mngr *trap_mngr; struct devlink_trap_item *trap_item; + struct devlink_port *devlink_port; int err; - trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info); - if (list_empty(&trap_mngr->trap_list)) - return -EOPNOTSUPP; + devlink_port = devlink_port_get_from_attrs(devlink, info->attrs); + if (IS_ERR(devlink_port)) { + trap_mngr = &devlink->trap_mngr; + if (list_empty(&trap_mngr->trap_list)) + goto loop_over_ports; - trap_item = devlink_trap_item_get_from_info(trap_mngr, info); - if (!trap_item) { - NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap"); - return -ENOENT; + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); + if (!trap_item) + goto loop_over_ports; + } else { + trap_mngr = &devlink_port->trap_mngr; + if (list_empty(&trap_mngr->trap_list)) + return -EOPNOTSUPP; + + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); + if (!trap_item) { + NL_SET_ERR_MSG_MOD(extack, "Port did not register this trap"); + return -ENOENT; + } } return devlink_trap_action_set(devlink, trap_mngr, trap_item, info); - err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); - if (err) - return err; +loop_over_ports: + if (list_empty(&devlink->port_list)) + return -EOPNOTSUPP; + list_for_each_entry(devlink_port, &devlink->port_list, list) { + trap_mngr = &devlink_port->trap_mngr; + if (list_empty(&trap_mngr->trap_list)) + continue; + trap_item = devlink_trap_item_get_from_info(trap_mngr, info); + if (!trap_item) + continue; + err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info); + if (err) + return err; + } return 0; }
Managing large scale port's traps may be complicated. This patch introduces a shortcut: when setting a trap on a device and this trap is not registered on this device, the action will take place on all related ports that did register this trap. Signed-off-by: Aya Levin <ayal@mellanox.com> --- net/core/devlink.c | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-)