Re: [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level
From: Ido Schimmel <hidden>
Date: 2020-09-06 15:59:36
Also in:
lkml
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.
quoted hunk ↗ jump to hunk
Signed-off-by: Aya Levin <redacted> --- 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