[PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context

linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context
@ 2020-09-02 15:32 Aya Levin
  2020-09-02 15:32 ` [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr Aya Levin
                   ` (3 more replies)
  0 siblings, 4 replies; 12+ messages in thread
From: Aya Levin @ 2020-09-02 15:32 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev
  Cc: Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel, Aya Levin

Implement support for devlink traps on per-port basis.
Dropped packets in the RX flow are related to the Ethernet port and
thus should be in port context. Traps per device should trap global
configuration which can cause drops. On top of that, enabling a trap
on a device level should trigger this trap on its siblings ports.
In addition, when devlink traps is enabled, it may cause a degradation
in performance. Hence devlink traps should be regard as a debug mode.
Considering that, it is preferred to encapsulate the debug mode as
much as possible and not to effect all the device.

Patchset:
Patch 1: Refactors devlink trap for easier code re-use in the coming
patches
Patch 2: Adds devlink traps under devlink port context
Patch 3: Adds a relation between traps in device context and traps in
ports context. In a nutshell it allows enable/disable of a trap on
all related ports which registered this trap.
Patch 4: Display a use in devlink traps in port context in mlx5
ethernet driver.

Aya Levin (4):
  devlink: Wrap trap related lists and ops in trap_mngr
  devlink: Add devlink traps under devlink_ports context
  devlink: Add hiererchy between traps in device level and port level
  net/mlx5e: Add devlink trap to catch oversize packets

 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en.h       |   2 +
 drivers/net/ethernet/mellanox/mlx5/core/en/traps.c |  32 ++
 drivers/net/ethernet/mellanox/mlx5/core/en/traps.h |  13 +
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |  41 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c    |  11 +-
 drivers/net/ethernet/mellanox/mlxsw/core.c         |   5 +
 include/net/devlink.h                              |  84 ++-
 net/core/devlink.c                                 | 616 +++++++++++++++++----
 9 files changed, 665 insertions(+), 141 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/traps.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/traps.h

-- 
2.14.1

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr
  2020-09-02 15:32 [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context Aya Levin
@ 2020-09-02 15:32 ` Aya Levin
  2020-09-08 14:12   ` Jiri Pirko
  2020-09-02 15:32 ` [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context Aya Levin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 12+ messages in thread
From: Aya Levin @ 2020-09-02 15:32 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev
  Cc: Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel, Aya Levin

Bundle the trap related lists: trap_list, trap_group_list and
trap_policer_list and trap ops like: trap_init, trap_fini,
trap_action_set... together in trap_mngr. This will be handy in the
coming patches in the set introducing traps in devlink port context.
With trap_mngr, code reuse is much simpler.

Signed-off-by: Aya Levin <ayal@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c |   4 +
 include/net/devlink.h                      |  59 ++++---
 net/core/devlink.c                         | 255 +++++++++++++++++------------
 3 files changed, 188 insertions(+), 130 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 08d101138fbe..97460f47e537 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1285,6 +1285,9 @@ static const struct devlink_ops mlxsw_devlink_ops = {
 	.sb_occ_tc_port_bind_get	= mlxsw_devlink_sb_occ_tc_port_bind_get,
 	.info_get			= mlxsw_devlink_info_get,
 	.flash_update			= mlxsw_devlink_flash_update,
+};
+
+static const struct devlink_trap_ops mlxsw_devlink_traps_ops = {
 	.trap_init			= mlxsw_devlink_trap_init,
 	.trap_fini			= mlxsw_devlink_trap_fini,
 	.trap_action_set		= mlxsw_devlink_trap_action_set,
@@ -1321,6 +1324,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 			err = -ENOMEM;
 			goto err_devlink_alloc;
 		}
+		devlink_traps_ops(devlink, &mlxsw_devlink_traps_ops);
 	}
 
 	mlxsw_core = devlink_priv(devlink);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 8f3c8a443238..d387ea5518c3 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -21,6 +21,13 @@
 #include <linux/xarray.h>
 
 struct devlink_ops;
+struct devlink_trap_ops;
+struct devlink_trap_mngr {
+	struct list_head trap_list;
+	struct list_head trap_group_list;
+	struct list_head trap_policer_list;
+	const struct devlink_trap_ops *trap_ops;
+};
 
 struct devlink {
 	struct list_head list;
@@ -33,9 +40,7 @@ struct devlink {
 	struct list_head reporter_list;
 	struct mutex reporters_lock; /* protects reporter_list */
 	struct devlink_dpipe_headers *dpipe_headers;
-	struct list_head trap_list;
-	struct list_head trap_group_list;
-	struct list_head trap_policer_list;
+	struct devlink_trap_mngr trap_mngr;
 	const struct devlink_ops *ops;
 	struct xarray snapshot_ids;
 	struct device *dev;
@@ -1054,6 +1059,31 @@ struct devlink_ops {
 	int (*flash_update)(struct devlink *devlink, const char *file_name,
 			    const char *component,
 			    struct netlink_ext_ack *extack);
+	/**
+	 * @port_function_hw_addr_get: Port function's hardware address get function.
+	 *
+	 * Should be used by device drivers to report the hardware address of a function
+	 * managed by the devlink port. Driver should return -EOPNOTSUPP if it doesn't
+	 * support port function handling for a particular port.
+	 *
+	 * Note: @extack can be NULL when port notifier queries the port function.
+	 */
+	int (*port_function_hw_addr_get)(struct devlink *devlink, struct devlink_port *port,
+					 u8 *hw_addr, int *hw_addr_len,
+					 struct netlink_ext_ack *extack);
+	/**
+	 * @port_function_hw_addr_set: Port function's hardware address set function.
+	 *
+	 * Should be used by device drivers to set the hardware address of a function
+	 * managed by the devlink port. Driver should return -EOPNOTSUPP if it doesn't
+	 * support port function handling for a particular port.
+	 */
+	int (*port_function_hw_addr_set)(struct devlink *devlink, struct devlink_port *port,
+					 const u8 *hw_addr, int hw_addr_len,
+					 struct netlink_ext_ack *extack);
+};
+
+struct devlink_trap_ops {
 	/**
 	 * @trap_init: Trap initialization function.
 	 *
@@ -1129,28 +1159,6 @@ struct devlink_ops {
 	int (*trap_policer_counter_get)(struct devlink *devlink,
 					const struct devlink_trap_policer *policer,
 					u64 *p_drops);
-	/**
-	 * @port_function_hw_addr_get: Port function's hardware address get function.
-	 *
-	 * Should be used by device drivers to report the hardware address of a function managed
-	 * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
-	 * function handling for a particular port.
-	 *
-	 * Note: @extack can be NULL when port notifier queries the port function.
-	 */
-	int (*port_function_hw_addr_get)(struct devlink *devlink, struct devlink_port *port,
-					 u8 *hw_addr, int *hw_addr_len,
-					 struct netlink_ext_ack *extack);
-	/**
-	 * @port_function_hw_addr_set: Port function's hardware address set function.
-	 *
-	 * Should be used by device drivers to set the hardware address of a function managed
-	 * by the devlink port. Driver should return -EOPNOTSUPP if it doesn't support port
-	 * function handling for a particular port.
-	 */
-	int (*port_function_hw_addr_set)(struct devlink *devlink, struct devlink_port *port,
-					 const u8 *hw_addr, int hw_addr_len,
-					 struct netlink_ext_ack *extack);
 };
 
 static inline void *devlink_priv(struct devlink *devlink)
@@ -1380,6 +1388,7 @@ void devlink_flash_update_status_notify(struct devlink *devlink,
 					unsigned long done,
 					unsigned long total);
 
+void devlink_traps_ops(struct devlink *devlink, const struct devlink_trap_ops *op);
 int devlink_traps_register(struct devlink *devlink,
 			   const struct devlink_trap *traps,
 			   size_t traps_count, void *priv);
diff --git a/net/core/devlink.c b/net/core/devlink.c
index 58c8bb07fa19..a30b5444289b 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6152,12 +6152,18 @@ struct devlink_trap_item {
 	void *priv;
 };
 
+static struct devlink_trap_mngr *
+devlink_trap_get_trap_mngr_from_info(struct devlink *devlink, struct genl_info *info)
+{
+		return &devlink->trap_mngr;
+}
+
 static struct devlink_trap_policer_item *
-devlink_trap_policer_item_lookup(struct devlink *devlink, u32 id)
+devlink_trap_policer_item_lookup(struct devlink_trap_mngr *trap_mngr, u32 id)
 {
 	struct devlink_trap_policer_item *policer_item;
 
-	list_for_each_entry(policer_item, &devlink->trap_policer_list, list) {
+	list_for_each_entry(policer_item, &trap_mngr->trap_policer_list, list) {
 		if (policer_item->policer->id == id)
 			return policer_item;
 	}
@@ -6166,11 +6172,11 @@ devlink_trap_policer_item_lookup(struct devlink *devlink, u32 id)
 }
 
 static struct devlink_trap_item *
-devlink_trap_item_lookup(struct devlink *devlink, const char *name)
+devlink_trap_item_lookup(struct devlink_trap_mngr *trap_mngr, const char *name)
 {
 	struct devlink_trap_item *trap_item;
 
-	list_for_each_entry(trap_item, &devlink->trap_list, list) {
+	list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
 		if (!strcmp(trap_item->trap->name, name))
 			return trap_item;
 	}
@@ -6179,8 +6185,7 @@ devlink_trap_item_lookup(struct devlink *devlink, const char *name)
 }
 
 static struct devlink_trap_item *
-devlink_trap_item_get_from_info(struct devlink *devlink,
-				struct genl_info *info)
+devlink_trap_item_get_from_info(struct devlink_trap_mngr *trap_mngr, struct genl_info *info)
 {
 	struct nlattr *attr;
 
@@ -6188,7 +6193,7 @@ devlink_trap_item_get_from_info(struct devlink *devlink,
 		return NULL;
 	attr = info->attrs[DEVLINK_ATTR_TRAP_NAME];
 
-	return devlink_trap_item_lookup(devlink, nla_data(attr));
+	return devlink_trap_item_lookup(trap_mngr, nla_data(attr));
 }
 
 static int
@@ -6343,14 +6348,13 @@ static int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb,
 {
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_trap_mngr *trap_mngr;
 	struct devlink_trap_item *trap_item;
 	struct sk_buff *msg;
 	int err;
 
-	if (list_empty(&devlink->trap_list))
-		return -EOPNOTSUPP;
-
-	trap_item = devlink_trap_item_get_from_info(devlink, info);
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	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;
@@ -6376,6 +6380,7 @@ static int devlink_nl_cmd_trap_get_doit(struct sk_buff *skb,
 static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 					  struct netlink_callback *cb)
 {
+	struct devlink_trap_mngr *trap_mngr;
 	struct devlink_trap_item *trap_item;
 	struct devlink *devlink;
 	int start = cb->args[0];
@@ -6386,8 +6391,9 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 	list_for_each_entry(devlink, &devlink_list, list) {
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			continue;
+		trap_mngr = &devlink->trap_mngr;
 		mutex_lock(&devlink->lock);
-		list_for_each_entry(trap_item, &devlink->trap_list, list) {
+		list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
 			if (idx < start) {
 				idx++;
 				continue;
@@ -6413,6 +6419,7 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 }
 
 static int __devlink_trap_action_set(struct devlink *devlink,
+				     struct devlink_trap_mngr *trap_mngr,
 				     struct devlink_trap_item *trap_item,
 				     enum devlink_trap_action trap_action,
 				     struct netlink_ext_ack *extack)
@@ -6425,8 +6432,8 @@ static int __devlink_trap_action_set(struct devlink *devlink,
 		return 0;
 	}
 
-	err = devlink->ops->trap_action_set(devlink, trap_item->trap,
-					    trap_action, extack);
+	err = trap_mngr->trap_ops->trap_action_set(devlink, trap_item->trap,
+						   trap_action, extack);
 	if (err)
 		return err;
 
@@ -6436,6 +6443,7 @@ static int __devlink_trap_action_set(struct devlink *devlink,
 }
 
 static int devlink_trap_action_set(struct devlink *devlink,
+				   struct devlink_trap_mngr *trap_mngr,
 				   struct devlink_trap_item *trap_item,
 				   struct genl_info *info)
 {
@@ -6451,7 +6459,7 @@ static int devlink_trap_action_set(struct devlink *devlink,
 		return -EINVAL;
 	}
 
-	return __devlink_trap_action_set(devlink, trap_item, trap_action,
+	return __devlink_trap_action_set(devlink, trap_mngr, trap_item, trap_action,
 					 info->extack);
 }
 
@@ -6460,19 +6468,21 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb,
 {
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_trap_mngr *trap_mngr;
 	struct devlink_trap_item *trap_item;
 	int err;
 
-	if (list_empty(&devlink->trap_list))
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	if (list_empty(&trap_mngr->trap_list))
 		return -EOPNOTSUPP;
 
-	trap_item = devlink_trap_item_get_from_info(devlink, info);
+	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;
 	}
 
-	err = devlink_trap_action_set(devlink, trap_item, info);
+	err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info);
 	if (err)
 		return err;
 
@@ -6480,11 +6490,11 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb,
 }
 
 static struct devlink_trap_group_item *
-devlink_trap_group_item_lookup(struct devlink *devlink, const char *name)
+devlink_trap_group_item_lookup(struct devlink_trap_mngr *trap_mngr, const char *name)
 {
 	struct devlink_trap_group_item *group_item;
 
-	list_for_each_entry(group_item, &devlink->trap_group_list, list) {
+	list_for_each_entry(group_item, &trap_mngr->trap_group_list, list) {
 		if (!strcmp(group_item->group->name, name))
 			return group_item;
 	}
@@ -6493,11 +6503,11 @@ devlink_trap_group_item_lookup(struct devlink *devlink, const char *name)
 }
 
 static struct devlink_trap_group_item *
-devlink_trap_group_item_lookup_by_id(struct devlink *devlink, u16 id)
+devlink_trap_group_item_lookup_by_id(struct devlink_trap_mngr *trap_mngr, u16 id)
 {
 	struct devlink_trap_group_item *group_item;
 
-	list_for_each_entry(group_item, &devlink->trap_group_list, list) {
+	list_for_each_entry(group_item, &trap_mngr->trap_group_list, list) {
 		if (group_item->group->id == id)
 			return group_item;
 	}
@@ -6506,7 +6516,7 @@ devlink_trap_group_item_lookup_by_id(struct devlink *devlink, u16 id)
 }
 
 static struct devlink_trap_group_item *
-devlink_trap_group_item_get_from_info(struct devlink *devlink,
+devlink_trap_group_item_get_from_info(struct devlink_trap_mngr *trap_mngr,
 				      struct genl_info *info)
 {
 	char *name;
@@ -6515,7 +6525,7 @@ devlink_trap_group_item_get_from_info(struct devlink *devlink,
 		return NULL;
 	name = nla_data(info->attrs[DEVLINK_ATTR_TRAP_GROUP_NAME]);
 
-	return devlink_trap_group_item_lookup(devlink, name);
+	return devlink_trap_group_item_lookup(trap_mngr, name);
 }
 
 static int
@@ -6566,13 +6576,15 @@ static int devlink_nl_cmd_trap_group_get_doit(struct sk_buff *skb,
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_trap_group_item *group_item;
+	struct devlink_trap_mngr *trap_mngr;
 	struct sk_buff *msg;
 	int err;
 
-	if (list_empty(&devlink->trap_group_list))
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	if (list_empty(&trap_mngr->trap_group_list))
 		return -EOPNOTSUPP;
 
-	group_item = devlink_trap_group_item_get_from_info(devlink, info);
+	group_item = devlink_trap_group_item_get_from_info(trap_mngr, info);
 	if (!group_item) {
 		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap group");
 		return -ENOENT;
@@ -6601,6 +6613,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 	enum devlink_command cmd = DEVLINK_CMD_TRAP_GROUP_NEW;
 	struct devlink_trap_group_item *group_item;
 	u32 portid = NETLINK_CB(cb->skb).portid;
+	struct devlink_trap_mngr *trap_mngr;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -6608,10 +6621,11 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 
 	mutex_lock(&devlink_mutex);
 	list_for_each_entry(devlink, &devlink_list, list) {
+		trap_mngr = &devlink->trap_mngr;
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			continue;
 		mutex_lock(&devlink->lock);
-		list_for_each_entry(group_item, &devlink->trap_group_list,
+		list_for_each_entry(group_item, &trap_mngr->trap_group_list,
 				    list) {
 			if (idx < start) {
 				idx++;
@@ -6639,6 +6653,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 
 static int
 __devlink_trap_group_action_set(struct devlink *devlink,
+				struct devlink_trap_mngr *trap_mngr,
 				struct devlink_trap_group_item *group_item,
 				enum devlink_trap_action trap_action,
 				struct netlink_ext_ack *extack)
@@ -6647,10 +6662,10 @@ __devlink_trap_group_action_set(struct devlink *devlink,
 	struct devlink_trap_item *trap_item;
 	int err;
 
-	list_for_each_entry(trap_item, &devlink->trap_list, list) {
+	list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
 		if (strcmp(trap_item->group_item->group->name, group_name))
 			continue;
-		err = __devlink_trap_action_set(devlink, trap_item,
+		err = __devlink_trap_action_set(devlink, trap_mngr, trap_item,
 						trap_action, extack);
 		if (err)
 			return err;
@@ -6661,6 +6676,7 @@ __devlink_trap_group_action_set(struct devlink *devlink,
 
 static int
 devlink_trap_group_action_set(struct devlink *devlink,
+			      struct devlink_trap_mngr *trap_mngr,
 			      struct devlink_trap_group_item *group_item,
 			      struct genl_info *info, bool *p_modified)
 {
@@ -6676,7 +6692,7 @@ devlink_trap_group_action_set(struct devlink *devlink,
 		return -EINVAL;
 	}
 
-	err = __devlink_trap_group_action_set(devlink, group_item, trap_action,
+	err = __devlink_trap_group_action_set(devlink, trap_mngr, group_item, trap_action,
 					      info->extack);
 	if (err)
 		return err;
@@ -6687,6 +6703,7 @@ devlink_trap_group_action_set(struct devlink *devlink,
 }
 
 static int devlink_trap_group_set(struct devlink *devlink,
+				  struct devlink_trap_mngr *trap_mngr,
 				  struct devlink_trap_group_item *group_item,
 				  struct genl_info *info)
 {
@@ -6699,7 +6716,7 @@ static int devlink_trap_group_set(struct devlink *devlink,
 	if (!attrs[DEVLINK_ATTR_TRAP_POLICER_ID])
 		return 0;
 
-	if (!devlink->ops->trap_group_set)
+	if (!trap_mngr->trap_ops->trap_group_set)
 		return -EOPNOTSUPP;
 
 	policer_item = group_item->policer_item;
@@ -6707,8 +6724,7 @@ static int devlink_trap_group_set(struct devlink *devlink,
 		u32 policer_id;
 
 		policer_id = nla_get_u32(attrs[DEVLINK_ATTR_TRAP_POLICER_ID]);
-		policer_item = devlink_trap_policer_item_lookup(devlink,
-								policer_id);
+		policer_item = devlink_trap_policer_item_lookup(trap_mngr, policer_id);
 		if (policer_id && !policer_item) {
 			NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap policer");
 			return -ENOENT;
@@ -6716,8 +6732,8 @@ static int devlink_trap_group_set(struct devlink *devlink,
 	}
 	policer = policer_item ? policer_item->policer : NULL;
 
-	err = devlink->ops->trap_group_set(devlink, group_item->group, policer,
-					   extack);
+	err = trap_mngr->trap_ops->trap_group_set(devlink, group_item->group, policer,
+						  extack);
 	if (err)
 		return err;
 
@@ -6732,24 +6748,26 @@ static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
 	struct devlink_trap_group_item *group_item;
+	struct devlink_trap_mngr *trap_mngr;
 	bool modified = false;
 	int err;
 
-	if (list_empty(&devlink->trap_group_list))
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	if (list_empty(&trap_mngr->trap_group_list))
 		return -EOPNOTSUPP;
 
-	group_item = devlink_trap_group_item_get_from_info(devlink, info);
+	group_item = devlink_trap_group_item_get_from_info(trap_mngr, info);
 	if (!group_item) {
 		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap group");
 		return -ENOENT;
 	}
 
-	err = devlink_trap_group_action_set(devlink, group_item, info,
+	err = devlink_trap_group_action_set(devlink, trap_mngr, group_item, info,
 					    &modified);
 	if (err)
 		return err;
 
-	err = devlink_trap_group_set(devlink, group_item, info);
+	err = devlink_trap_group_set(devlink, trap_mngr, group_item, info);
 	if (err)
 		goto err_trap_group_set;
 
@@ -6762,7 +6780,7 @@ static int devlink_nl_cmd_trap_group_set_doit(struct sk_buff *skb,
 }
 
 static struct devlink_trap_policer_item *
-devlink_trap_policer_item_get_from_info(struct devlink *devlink,
+devlink_trap_policer_item_get_from_info(struct devlink_trap_mngr *trap_mngr,
 					struct genl_info *info)
 {
 	u32 id;
@@ -6771,21 +6789,22 @@ devlink_trap_policer_item_get_from_info(struct devlink *devlink,
 		return NULL;
 	id = nla_get_u32(info->attrs[DEVLINK_ATTR_TRAP_POLICER_ID]);
 
-	return devlink_trap_policer_item_lookup(devlink, id);
+	return devlink_trap_policer_item_lookup(trap_mngr, id);
 }
 
 static int
 devlink_trap_policer_stats_put(struct sk_buff *msg, struct devlink *devlink,
-			       const struct devlink_trap_policer *policer)
+			       const struct devlink_trap_policer *policer,
+			       struct devlink_trap_mngr *trap_mngr)
 {
 	struct nlattr *attr;
 	u64 drops;
 	int err;
 
-	if (!devlink->ops->trap_policer_counter_get)
+	if (!trap_mngr->trap_ops->trap_policer_counter_get)
 		return 0;
 
-	err = devlink->ops->trap_policer_counter_get(devlink, policer, &drops);
+	err = trap_mngr->trap_ops->trap_policer_counter_get(devlink, policer, &drops);
 	if (err)
 		return err;
 
@@ -6810,6 +6829,7 @@ static int
 devlink_nl_trap_policer_fill(struct sk_buff *msg, struct devlink *devlink,
 			     const struct devlink_trap_policer_item *policer_item,
 			     enum devlink_command cmd, u32 portid, u32 seq,
+			     struct devlink_trap_mngr *trap_mngr,
 			     int flags)
 {
 	void *hdr;
@@ -6835,7 +6855,7 @@ devlink_nl_trap_policer_fill(struct sk_buff *msg, struct devlink *devlink,
 		goto nla_put_failure;
 
 	err = devlink_trap_policer_stats_put(msg, devlink,
-					     policer_item->policer);
+					     policer_item->policer, trap_mngr);
 	if (err)
 		goto nla_put_failure;
 
@@ -6854,13 +6874,15 @@ static int devlink_nl_cmd_trap_policer_get_doit(struct sk_buff *skb,
 	struct devlink_trap_policer_item *policer_item;
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_trap_mngr *trap_mngr;
 	struct sk_buff *msg;
 	int err;
 
-	if (list_empty(&devlink->trap_policer_list))
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	if (list_empty(&trap_mngr->trap_policer_list))
 		return -EOPNOTSUPP;
 
-	policer_item = devlink_trap_policer_item_get_from_info(devlink, info);
+	policer_item = devlink_trap_policer_item_get_from_info(trap_mngr, info);
 	if (!policer_item) {
 		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap policer");
 		return -ENOENT;
@@ -6872,7 +6894,7 @@ static int devlink_nl_cmd_trap_policer_get_doit(struct sk_buff *skb,
 
 	err = devlink_nl_trap_policer_fill(msg, devlink, policer_item,
 					   DEVLINK_CMD_TRAP_POLICER_NEW,
-					   info->snd_portid, info->snd_seq, 0);
+					   info->snd_portid, info->snd_seq, trap_mngr, 0);
 	if (err)
 		goto err_trap_policer_fill;
 
@@ -6889,6 +6911,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 	enum devlink_command cmd = DEVLINK_CMD_TRAP_POLICER_NEW;
 	struct devlink_trap_policer_item *policer_item;
 	u32 portid = NETLINK_CB(cb->skb).portid;
+	struct devlink_trap_mngr *trap_mngr;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -6896,10 +6919,11 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 
 	mutex_lock(&devlink_mutex);
 	list_for_each_entry(devlink, &devlink_list, list) {
+		trap_mngr = &devlink->trap_mngr;
 		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
 			continue;
 		mutex_lock(&devlink->lock);
-		list_for_each_entry(policer_item, &devlink->trap_policer_list,
+		list_for_each_entry(policer_item, &trap_mngr->trap_policer_list,
 				    list) {
 			if (idx < start) {
 				idx++;
@@ -6909,6 +6933,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 							   policer_item, cmd,
 							   portid,
 							   cb->nlh->nlmsg_seq,
+							   trap_mngr,
 							   NLM_F_MULTI);
 			if (err) {
 				mutex_unlock(&devlink->lock);
@@ -6926,7 +6951,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 }
 
 static int
-devlink_trap_policer_set(struct devlink *devlink,
+devlink_trap_policer_set(struct devlink *devlink, struct devlink_trap_mngr *trap_mngr,
 			 struct devlink_trap_policer_item *policer_item,
 			 struct genl_info *info)
 {
@@ -6964,8 +6989,8 @@ devlink_trap_policer_set(struct devlink *devlink,
 		return -EINVAL;
 	}
 
-	err = devlink->ops->trap_policer_set(devlink, policer_item->policer,
-					     rate, burst, info->extack);
+	err = trap_mngr->trap_ops->trap_policer_set(devlink, policer_item->policer,
+						    rate, burst, info->extack);
 	if (err)
 		return err;
 
@@ -6981,20 +7006,22 @@ static int devlink_nl_cmd_trap_policer_set_doit(struct sk_buff *skb,
 	struct devlink_trap_policer_item *policer_item;
 	struct netlink_ext_ack *extack = info->extack;
 	struct devlink *devlink = info->user_ptr[0];
+	struct devlink_trap_mngr *trap_mngr;
 
-	if (list_empty(&devlink->trap_policer_list))
+	trap_mngr = devlink_trap_get_trap_mngr_from_info(devlink, info);
+	if (list_empty(&trap_mngr->trap_policer_list))
 		return -EOPNOTSUPP;
 
-	if (!devlink->ops->trap_policer_set)
+	if (!trap_mngr->trap_ops->trap_policer_set)
 		return -EOPNOTSUPP;
 
-	policer_item = devlink_trap_policer_item_get_from_info(devlink, info);
+	policer_item = devlink_trap_policer_item_get_from_info(trap_mngr, info);
 	if (!policer_item) {
 		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap policer");
 		return -ENOENT;
 	}
 
-	return devlink_trap_policer_set(devlink, policer_item, info);
+	return devlink_trap_policer_set(devlink, trap_mngr, policer_item, info);
 }
 
 static const struct nla_policy devlink_nl_policy[DEVLINK_ATTR_MAX + 1] = {
@@ -7396,9 +7423,9 @@ struct devlink *devlink_alloc(const struct devlink_ops *ops, size_t priv_size)
 	INIT_LIST_HEAD(&devlink->param_list);
 	INIT_LIST_HEAD(&devlink->region_list);
 	INIT_LIST_HEAD(&devlink->reporter_list);
-	INIT_LIST_HEAD(&devlink->trap_list);
-	INIT_LIST_HEAD(&devlink->trap_group_list);
-	INIT_LIST_HEAD(&devlink->trap_policer_list);
+	INIT_LIST_HEAD(&devlink->trap_mngr.trap_list);
+	INIT_LIST_HEAD(&devlink->trap_mngr.trap_group_list);
+	INIT_LIST_HEAD(&devlink->trap_mngr.trap_policer_list);
 	mutex_init(&devlink->lock);
 	mutex_init(&devlink->reporters_lock);
 	return devlink;
@@ -7483,9 +7510,9 @@ void devlink_free(struct devlink *devlink)
 {
 	mutex_destroy(&devlink->reporters_lock);
 	mutex_destroy(&devlink->lock);
-	WARN_ON(!list_empty(&devlink->trap_policer_list));
-	WARN_ON(!list_empty(&devlink->trap_group_list));
-	WARN_ON(!list_empty(&devlink->trap_list));
+	WARN_ON(!list_empty(&devlink->trap_mngr.trap_policer_list));
+	WARN_ON(!list_empty(&devlink->trap_mngr.trap_group_list));
+	WARN_ON(!list_empty(&devlink->trap_mngr.trap_list));
 	WARN_ON(!list_empty(&devlink->reporter_list));
 	WARN_ON(!list_empty(&devlink->region_list));
 	WARN_ON(!list_empty(&devlink->param_list));
@@ -8945,13 +8972,13 @@ devlink_trap_group_notify(struct devlink *devlink,
 }
 
 static int
-devlink_trap_item_group_link(struct devlink *devlink,
+devlink_trap_item_group_link(struct devlink_trap_mngr *trap_mngr,
 			     struct devlink_trap_item *trap_item)
 {
 	u16 group_id = trap_item->trap->init_group_id;
 	struct devlink_trap_group_item *group_item;
 
-	group_item = devlink_trap_group_item_lookup_by_id(devlink, group_id);
+	group_item = devlink_trap_group_item_lookup_by_id(trap_mngr, group_id);
 	if (WARN_ON_ONCE(!group_item))
 		return -EINVAL;
 
@@ -8985,13 +9012,13 @@ static void devlink_trap_notify(struct devlink *devlink,
 }
 
 static int
-devlink_trap_register(struct devlink *devlink,
+devlink_trap_register(struct devlink *devlink, struct devlink_trap_mngr *trap_mngr,
 		      const struct devlink_trap *trap, void *priv)
 {
 	struct devlink_trap_item *trap_item;
 	int err;
 
-	if (devlink_trap_item_lookup(devlink, trap->name))
+	if (devlink_trap_item_lookup(trap_mngr, trap->name))
 		return -EEXIST;
 
 	trap_item = kzalloc(sizeof(*trap_item), GFP_KERNEL);
@@ -9008,15 +9035,15 @@ devlink_trap_register(struct devlink *devlink,
 	trap_item->action = trap->init_action;
 	trap_item->priv = priv;
 
-	err = devlink_trap_item_group_link(devlink, trap_item);
+	err = devlink_trap_item_group_link(trap_mngr, trap_item);
 	if (err)
 		goto err_group_link;
 
-	err = devlink->ops->trap_init(devlink, trap, trap_item);
+	err = trap_mngr->trap_ops->trap_init(devlink, trap, trap_item);
 	if (err)
 		goto err_trap_init;
 
-	list_add_tail(&trap_item->list, &devlink->trap_list);
+	list_add_tail(&trap_item->list, &trap_mngr->trap_list);
 	devlink_trap_notify(devlink, trap_item, DEVLINK_CMD_TRAP_NEW);
 
 	return 0;
@@ -9030,36 +9057,48 @@ devlink_trap_register(struct devlink *devlink,
 }
 
 static void devlink_trap_unregister(struct devlink *devlink,
+				    struct devlink_trap_mngr *trap_mngr,
 				    const struct devlink_trap *trap)
 {
 	struct devlink_trap_item *trap_item;
 
-	trap_item = devlink_trap_item_lookup(devlink, trap->name);
+	trap_item = devlink_trap_item_lookup(trap_mngr, trap->name);
 	if (WARN_ON_ONCE(!trap_item))
 		return;
 
 	devlink_trap_notify(devlink, trap_item, DEVLINK_CMD_TRAP_DEL);
 	list_del(&trap_item->list);
-	if (devlink->ops->trap_fini)
-		devlink->ops->trap_fini(devlink, trap, trap_item);
+	if (trap_mngr->trap_ops->trap_fini)
+		trap_mngr->trap_ops->trap_fini(devlink, trap, trap_item);
 	free_percpu(trap_item->stats);
 	kfree(trap_item);
 }
 
 static void devlink_trap_disable(struct devlink *devlink,
+				 struct devlink_trap_mngr *trap_mngr,
 				 const struct devlink_trap *trap)
 {
 	struct devlink_trap_item *trap_item;
 
-	trap_item = devlink_trap_item_lookup(devlink, trap->name);
+	trap_item = devlink_trap_item_lookup(trap_mngr, trap->name);
 	if (WARN_ON_ONCE(!trap_item))
 		return;
 
-	devlink->ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP,
-				      NULL);
+	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP, NULL);
 	trap_item->action = DEVLINK_TRAP_ACTION_DROP;
 }
 
+/**
+ * devlink_traps_ops - Register trap callbacks
+ * @devlink: devlink.
+ * @ops: trap ops
+ */
+void devlink_traps_ops(struct devlink *devlink, const struct devlink_trap_ops *ops)
+{
+	devlink->trap_mngr.trap_ops = ops;
+}
+EXPORT_SYMBOL_GPL(devlink_traps_ops);
+
 /**
  * devlink_traps_register - Register packet traps with devlink.
  * @devlink: devlink.
@@ -9073,9 +9112,10 @@ int devlink_traps_register(struct devlink *devlink,
 			   const struct devlink_trap *traps,
 			   size_t traps_count, void *priv)
 {
+	struct devlink_trap_mngr *trap_mngr = &devlink->trap_mngr;
 	int i, err;
 
-	if (!devlink->ops->trap_init || !devlink->ops->trap_action_set)
+	if (!trap_mngr->trap_ops->trap_init || !trap_mngr->trap_ops->trap_action_set)
 		return -EINVAL;
 
 	mutex_lock(&devlink->lock);
@@ -9086,7 +9126,7 @@ int devlink_traps_register(struct devlink *devlink,
 		if (err)
 			goto err_trap_verify;
 
-		err = devlink_trap_register(devlink, trap, priv);
+		err = devlink_trap_register(devlink, &devlink->trap_mngr, trap, priv);
 		if (err)
 			goto err_trap_register;
 	}
@@ -9097,7 +9137,7 @@ int devlink_traps_register(struct devlink *devlink,
 err_trap_register:
 err_trap_verify:
 	for (i--; i >= 0; i--)
-		devlink_trap_unregister(devlink, &traps[i]);
+		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
 	mutex_unlock(&devlink->lock);
 	return err;
 }
@@ -9113,6 +9153,7 @@ void devlink_traps_unregister(struct devlink *devlink,
 			      const struct devlink_trap *traps,
 			      size_t traps_count)
 {
+	struct devlink_trap_mngr *trap_mngr = &devlink->trap_mngr;
 	int i;
 
 	mutex_lock(&devlink->lock);
@@ -9120,10 +9161,10 @@ void devlink_traps_unregister(struct devlink *devlink,
 	 * traps by disabling all of them and waiting for a grace period.
 	 */
 	for (i = traps_count - 1; i >= 0; i--)
-		devlink_trap_disable(devlink, &traps[i]);
+		devlink_trap_disable(devlink, trap_mngr, &traps[i]);
 	synchronize_rcu();
 	for (i = traps_count - 1; i >= 0; i--)
-		devlink_trap_unregister(devlink, &traps[i]);
+		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_traps_unregister);
@@ -9206,7 +9247,7 @@ void *devlink_trap_ctx_priv(void *trap_ctx)
 EXPORT_SYMBOL_GPL(devlink_trap_ctx_priv);
 
 static int
-devlink_trap_group_item_policer_link(struct devlink *devlink,
+devlink_trap_group_item_policer_link(struct devlink_trap_mngr *trap_mngr,
 				     struct devlink_trap_group_item *group_item)
 {
 	u32 policer_id = group_item->group->init_policer_id;
@@ -9215,7 +9256,7 @@ devlink_trap_group_item_policer_link(struct devlink *devlink,
 	if (policer_id == 0)
 		return 0;
 
-	policer_item = devlink_trap_policer_item_lookup(devlink, policer_id);
+	policer_item = devlink_trap_policer_item_lookup(trap_mngr, policer_id);
 	if (WARN_ON_ONCE(!policer_item))
 		return -EINVAL;
 
@@ -9225,13 +9266,13 @@ devlink_trap_group_item_policer_link(struct devlink *devlink,
 }
 
 static int
-devlink_trap_group_register(struct devlink *devlink,
+devlink_trap_group_register(struct devlink *devlink, struct devlink_trap_mngr *trap_mngr,
 			    const struct devlink_trap_group *group)
 {
 	struct devlink_trap_group_item *group_item;
 	int err;
 
-	if (devlink_trap_group_item_lookup(devlink, group->name))
+	if (devlink_trap_group_item_lookup(trap_mngr, group->name))
 		return -EEXIST;
 
 	group_item = kzalloc(sizeof(*group_item), GFP_KERNEL);
@@ -9246,17 +9287,17 @@ devlink_trap_group_register(struct devlink *devlink,
 
 	group_item->group = group;
 
-	err = devlink_trap_group_item_policer_link(devlink, group_item);
+	err = devlink_trap_group_item_policer_link(trap_mngr, group_item);
 	if (err)
 		goto err_policer_link;
 
-	if (devlink->ops->trap_group_init) {
-		err = devlink->ops->trap_group_init(devlink, group);
+	if (trap_mngr->trap_ops->trap_group_init) {
+		err = trap_mngr->trap_ops->trap_group_init(devlink, group);
 		if (err)
 			goto err_group_init;
 	}
 
-	list_add_tail(&group_item->list, &devlink->trap_group_list);
+	list_add_tail(&group_item->list, &trap_mngr->trap_group_list);
 	devlink_trap_group_notify(devlink, group_item,
 				  DEVLINK_CMD_TRAP_GROUP_NEW);
 
@@ -9271,12 +9312,12 @@ devlink_trap_group_register(struct devlink *devlink,
 }
 
 static void
-devlink_trap_group_unregister(struct devlink *devlink,
+devlink_trap_group_unregister(struct devlink *devlink, struct devlink_trap_mngr *trap_mngr,
 			      const struct devlink_trap_group *group)
 {
 	struct devlink_trap_group_item *group_item;
 
-	group_item = devlink_trap_group_item_lookup(devlink, group->name);
+	group_item = devlink_trap_group_item_lookup(trap_mngr, group->name);
 	if (WARN_ON_ONCE(!group_item))
 		return;
 
@@ -9299,6 +9340,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 				 const struct devlink_trap_group *groups,
 				 size_t groups_count)
 {
+	struct devlink_trap_mngr *trap_mngr = &devlink->trap_mngr;
 	int i, err;
 
 	mutex_lock(&devlink->lock);
@@ -9309,7 +9351,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 		if (err)
 			goto err_trap_group_verify;
 
-		err = devlink_trap_group_register(devlink, group);
+		err = devlink_trap_group_register(devlink, trap_mngr, group);
 		if (err)
 			goto err_trap_group_register;
 	}
@@ -9320,7 +9362,7 @@ int devlink_trap_groups_register(struct devlink *devlink,
 err_trap_group_register:
 err_trap_group_verify:
 	for (i--; i >= 0; i--)
-		devlink_trap_group_unregister(devlink, &groups[i]);
+		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
 	mutex_unlock(&devlink->lock);
 	return err;
 }
@@ -9340,7 +9382,7 @@ void devlink_trap_groups_unregister(struct devlink *devlink,
 
 	mutex_lock(&devlink->lock);
 	for (i = groups_count - 1; i >= 0; i--)
-		devlink_trap_group_unregister(devlink, &groups[i]);
+		devlink_trap_group_unregister(devlink, &devlink->trap_mngr, &groups[i]);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_trap_groups_unregister);
@@ -9361,7 +9403,7 @@ devlink_trap_policer_notify(struct devlink *devlink,
 		return;
 
 	err = devlink_nl_trap_policer_fill(msg, devlink, policer_item, cmd, 0,
-					   0, 0);
+					   0, &devlink->trap_mngr, 0);
 	if (err) {
 		nlmsg_free(msg);
 		return;
@@ -9372,13 +9414,13 @@ devlink_trap_policer_notify(struct devlink *devlink,
 }
 
 static int
-devlink_trap_policer_register(struct devlink *devlink,
+devlink_trap_policer_register(struct devlink *devlink, struct devlink_trap_mngr *trap_mngr,
 			      const struct devlink_trap_policer *policer)
 {
 	struct devlink_trap_policer_item *policer_item;
 	int err;
 
-	if (devlink_trap_policer_item_lookup(devlink, policer->id))
+	if (devlink_trap_policer_item_lookup(trap_mngr, policer->id))
 		return -EEXIST;
 
 	policer_item = kzalloc(sizeof(*policer_item), GFP_KERNEL);
@@ -9389,13 +9431,13 @@ devlink_trap_policer_register(struct devlink *devlink,
 	policer_item->rate = policer->init_rate;
 	policer_item->burst = policer->init_burst;
 
-	if (devlink->ops->trap_policer_init) {
-		err = devlink->ops->trap_policer_init(devlink, policer);
+	if (trap_mngr->trap_ops->trap_policer_init) {
+		err = trap_mngr->trap_ops->trap_policer_init(devlink, policer);
 		if (err)
 			goto err_policer_init;
 	}
 
-	list_add_tail(&policer_item->list, &devlink->trap_policer_list);
+	list_add_tail(&policer_item->list, &trap_mngr->trap_policer_list);
 	devlink_trap_policer_notify(devlink, policer_item,
 				    DEVLINK_CMD_TRAP_POLICER_NEW);
 
@@ -9408,19 +9450,20 @@ devlink_trap_policer_register(struct devlink *devlink,
 
 static void
 devlink_trap_policer_unregister(struct devlink *devlink,
+				struct devlink_trap_mngr *trap_mngr,
 				const struct devlink_trap_policer *policer)
 {
 	struct devlink_trap_policer_item *policer_item;
 
-	policer_item = devlink_trap_policer_item_lookup(devlink, policer->id);
+	policer_item = devlink_trap_policer_item_lookup(trap_mngr, policer->id);
 	if (WARN_ON_ONCE(!policer_item))
 		return;
 
 	devlink_trap_policer_notify(devlink, policer_item,
 				    DEVLINK_CMD_TRAP_POLICER_DEL);
 	list_del(&policer_item->list);
-	if (devlink->ops->trap_policer_fini)
-		devlink->ops->trap_policer_fini(devlink, policer);
+	if (trap_mngr->trap_ops->trap_policer_fini)
+		trap_mngr->trap_ops->trap_policer_fini(devlink, policer);
 	kfree(policer_item);
 }
 
@@ -9437,6 +9480,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 			       const struct devlink_trap_policer *policers,
 			       size_t policers_count)
 {
+	struct devlink_trap_mngr *trap_mngr = &devlink->trap_mngr;
 	int i, err;
 
 	mutex_lock(&devlink->lock);
@@ -9450,7 +9494,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 			goto err_trap_policer_verify;
 		}
 
-		err = devlink_trap_policer_register(devlink, policer);
+		err = devlink_trap_policer_register(devlink, trap_mngr, policer);
 		if (err)
 			goto err_trap_policer_register;
 	}
@@ -9461,7 +9505,7 @@ devlink_trap_policers_register(struct devlink *devlink,
 err_trap_policer_register:
 err_trap_policer_verify:
 	for (i--; i >= 0; i--)
-		devlink_trap_policer_unregister(devlink, &policers[i]);
+		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
 	mutex_unlock(&devlink->lock);
 	return err;
 }
@@ -9478,11 +9522,12 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 				 const struct devlink_trap_policer *policers,
 				 size_t policers_count)
 {
+	struct devlink_trap_mngr *trap_mngr = &devlink->trap_mngr;
 	int i;
 
 	mutex_lock(&devlink->lock);
 	for (i = policers_count - 1; i >= 0; i--)
-		devlink_trap_policer_unregister(devlink, &policers[i]);
+		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
 	mutex_unlock(&devlink->lock);
 }
 EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr
  2020-09-02 15:32 ` [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr Aya Levin
@ 2020-09-08 14:12   ` Jiri Pirko
  0 siblings, 0 replies; 12+ messages in thread
From: Jiri Pirko @ 2020-09-08 14:12 UTC (permalink / raw)
  To: Aya Levin
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel

Wed, Sep 02, 2020 at 05:32:11PM CEST, ayal@mellanox.com wrote:
>Bundle the trap related lists: trap_list, trap_group_list and
>trap_policer_list and trap ops like: trap_init, trap_fini,
>trap_action_set... together in trap_mngr. This will be handy in the
>coming patches in the set introducing traps in devlink port context.
>With trap_mngr, code reuse is much simpler.
>
>Signed-off-by: Aya Levin <ayal@mellanox.com>
>---
> drivers/net/ethernet/mellanox/mlxsw/core.c |   4 +
> include/net/devlink.h                      |  59 ++++---
> net/core/devlink.c                         | 255 +++++++++++++++++------------

You need to split this. You do at least 2 separate things in one patch.
Please check it.


> 3 files changed, 188 insertions(+), 130 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>index 08d101138fbe..97460f47e537 100644
>--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>@@ -1285,6 +1285,9 @@ static const struct devlink_ops mlxsw_devlink_ops = {
> 	.sb_occ_tc_port_bind_get	= mlxsw_devlink_sb_occ_tc_port_bind_get,
> 	.info_get			= mlxsw_devlink_info_get,
> 	.flash_update			= mlxsw_devlink_flash_update,
>+};
>+
>+static const struct devlink_trap_ops mlxsw_devlink_traps_ops = {
> 	.trap_init			= mlxsw_devlink_trap_init,
> 	.trap_fini			= mlxsw_devlink_trap_fini,
> 	.trap_action_set		= mlxsw_devlink_trap_action_set,
>@@ -1321,6 +1324,7 @@ __mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
> 			err = -ENOMEM;
> 			goto err_devlink_alloc;
> 		}
>+		devlink_traps_ops(devlink, &mlxsw_devlink_traps_ops);
> 	}
> 
> 	mlxsw_core = devlink_priv(devlink);
>diff --git a/include/net/devlink.h b/include/net/devlink.h
>index 8f3c8a443238..d387ea5518c3 100644
>--- a/include/net/devlink.h
>+++ b/include/net/devlink.h
>@@ -21,6 +21,13 @@
> #include <linux/xarray.h>
> 
> struct devlink_ops;
>+struct devlink_trap_ops;
>+struct devlink_trap_mngr {

"Mngr" is odd. This is not a manager. Just a common place to store
trap-related things. Perhaps just "devlink_traps"?


>+	struct list_head trap_list;
>+	struct list_head trap_group_list;
>+	struct list_head trap_policer_list;
>+	const struct devlink_trap_ops *trap_ops;
>+};

[...]


^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context
  2020-09-02 15:32 [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context Aya Levin
  2020-09-02 15:32 ` [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr Aya Levin
@ 2020-09-02 15:32 ` Aya Levin
  2020-09-06 15:44   ` Ido Schimmel
  2020-09-02 15:32 ` [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level Aya Levin
  2020-09-02 15:32 ` [PATCH net-next RFC v1 4/4] net/mlx5e: Add devlink trap to catch oversize packets Aya Levin
  3 siblings, 1 reply; 12+ messages in thread
From: Aya Levin @ 2020-09-02 15:32 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev
  Cc: Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel, Aya Levin

There are some cases where we would like to trap dropped packets only
for a single port on a device without affecting the others. For that
purpose trap_mngr was added to devlink_port and corresponding Trap API
with devlink_port were added too.

Signed-off-by: Aya Levin <ayal@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlxsw/core.c |   1 +
 include/net/devlink.h                      |  25 +++
 net/core/devlink.c                         | 332 ++++++++++++++++++++++++++++-
 3 files changed, 353 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 97460f47e537..cb9567a6a90d 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -1178,6 +1178,7 @@ static void mlxsw_devlink_trap_fini(struct devlink *devlink,
 static int mlxsw_devlink_trap_action_set(struct devlink *devlink,
 					 const struct devlink_trap *trap,
 					 enum devlink_trap_action action,
+					 void *trap_ctx,
 					 struct netlink_ext_ack *extack)
 {
 	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
diff --git a/include/net/devlink.h b/include/net/devlink.h
index d387ea5518c3..b4897ee38209 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -110,6 +110,7 @@ struct devlink_port {
 	struct delayed_work type_warn_dw;
 	struct list_head reporter_list;
 	struct mutex reporters_lock; /* Protects reporter_list */
+	struct devlink_trap_mngr trap_mngr;
 };
 
 struct devlink_sb_pool_info {
@@ -1108,6 +1109,7 @@ struct devlink_trap_ops {
 	int (*trap_action_set)(struct devlink *devlink,
 			       const struct devlink_trap *trap,
 			       enum devlink_trap_action action,
+			       void *trap_ctx,
 			       struct netlink_ext_ack *extack);
 	/**
 	 * @trap_group_init: Trap group initialization function.
@@ -1414,6 +1416,29 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 				 const struct devlink_trap_policer *policers,
 				 size_t policers_count);
 
+void devlink_port_traps_ops(struct devlink_port *devlink_port,
+			    const struct devlink_trap_ops *ops);
+int devlink_port_traps_register(struct devlink_port *devlink_port,
+				const struct devlink_trap *traps,
+				size_t traps_count, void *priv);
+void devlink_port_traps_unregister(struct devlink_port *devlink_port,
+				   const struct devlink_trap *traps,
+				   size_t traps_count);
+void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
+			      void *trap_ctx, const struct flow_action_cookie *fa_cookie);
+int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
+				      const struct devlink_trap_group *groups,
+				      size_t groups_count);
+void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
+					 const struct devlink_trap_group *groups,
+					 size_t groups_count);
+int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
+					const struct devlink_trap_policer *policers,
+					size_t policers_count);
+void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
+					   const struct devlink_trap_policer *policers,
+					   size_t policers_count);
+
 #if IS_ENABLED(CONFIG_NET_DEVLINK)
 
 void devlink_compat_running_version(struct net_device *dev,
diff --git a/net/core/devlink.c b/net/core/devlink.c
index a30b5444289b..b13e1b40bf1c 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6155,7 +6155,13 @@ struct devlink_trap_item {
 static struct devlink_trap_mngr *
 devlink_trap_get_trap_mngr_from_info(struct devlink *devlink, struct genl_info *info)
 {
-		return &devlink->trap_mngr;
+	struct devlink_port *devlink_port;
+
+	devlink_port = devlink_port_get_from_attrs(devlink, info->attrs);
+	if (IS_ERR(devlink_port))
+		return  &devlink->trap_mngr;
+	else
+		return &devlink_port->trap_mngr;
 }
 
 static struct devlink_trap_policer_item *
@@ -6382,6 +6388,7 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 {
 	struct devlink_trap_mngr *trap_mngr;
 	struct devlink_trap_item *trap_item;
+	struct devlink_port *port;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -6411,6 +6418,30 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 	}
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		list_for_each_entry(port, &devlink->port_list, list) {
+			trap_mngr = &port->trap_mngr;
+			mutex_lock(&devlink->lock);
+			list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
+				if (idx < start) {
+					idx++;
+					continue;
+				}
+				err = devlink_nl_trap_fill(msg, devlink, trap_item,
+							   DEVLINK_CMD_TRAP_NEW,
+							   NETLINK_CB(cb->skb).portid,
+							   cb->nlh->nlmsg_seq,
+							   NLM_F_MULTI);
+				if (err)
+					goto out;
+				idx++;
+			}
+			mutex_unlock(&devlink->lock);
+		}
+	}
+
 out:
 	mutex_unlock(&devlink_mutex);
 
@@ -6433,7 +6464,7 @@ static int __devlink_trap_action_set(struct devlink *devlink,
 	}
 
 	err = trap_mngr->trap_ops->trap_action_set(devlink, trap_item->trap,
-						   trap_action, extack);
+						   trap_action, trap_item, extack);
 	if (err)
 		return err;
 
@@ -6481,6 +6512,7 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb,
 		NL_SET_ERR_MSG_MOD(extack, "Device 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)
@@ -6614,6 +6646,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 	struct devlink_trap_group_item *group_item;
 	u32 portid = NETLINK_CB(cb->skb).portid;
 	struct devlink_trap_mngr *trap_mngr;
+	struct devlink_port *port;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -6644,6 +6677,30 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 	}
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		list_for_each_entry(port, &devlink->port_list, list) {
+			trap_mngr = &port->trap_mngr;
+			mutex_lock(&devlink->lock);
+			list_for_each_entry(group_item, &trap_mngr->trap_group_list, list) {
+				if (idx < start) {
+					idx++;
+					continue;
+				}
+				err = devlink_nl_trap_group_fill(msg, devlink,
+								 group_item, cmd,
+								 portid,
+								 cb->nlh->nlmsg_seq,
+								 NLM_F_MULTI);
+				if (err)
+					goto out;
+				idx++;
+			}
+			mutex_unlock(&devlink->lock);
+		}
+	}
+
 out:
 	mutex_unlock(&devlink_mutex);
 
@@ -6912,6 +6969,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 	struct devlink_trap_policer_item *policer_item;
 	u32 portid = NETLINK_CB(cb->skb).portid;
 	struct devlink_trap_mngr *trap_mngr;
+	struct devlink_port *port;
 	struct devlink *devlink;
 	int start = cb->args[0];
 	int idx = 0;
@@ -6943,6 +7001,32 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
 		}
 		mutex_unlock(&devlink->lock);
 	}
+	list_for_each_entry(devlink, &devlink_list, list) {
+		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
+			continue;
+		list_for_each_entry(port, &devlink->port_list, list) {
+			trap_mngr = &port->trap_mngr;
+			mutex_lock(&devlink->lock);
+			list_for_each_entry(policer_item, &trap_mngr->trap_policer_list,
+					    list) {
+				if (idx < start) {
+					idx++;
+					continue;
+				}
+				err = devlink_nl_trap_policer_fill(msg, devlink,
+								   policer_item, cmd,
+								   portid,
+								   cb->nlh->nlmsg_seq,
+								   trap_mngr,
+								   NLM_F_MULTI);
+				if (err)
+					goto out;
+				idx++;
+			}
+			mutex_unlock(&devlink->lock);
+		}
+	}
+
 out:
 	mutex_unlock(&devlink_mutex);
 
@@ -7348,34 +7432,40 @@ static const struct genl_ops devlink_nl_ops[] = {
 		.cmd = DEVLINK_CMD_TRAP_GET,
 		.doit = devlink_nl_cmd_trap_get_doit,
 		.dumpit = devlink_nl_cmd_trap_get_dumpit,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 		/* can be retrieved by unprivileged users */
 	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_SET,
 		.doit = devlink_nl_cmd_trap_set_doit,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_GROUP_GET,
 		.doit = devlink_nl_cmd_trap_group_get_doit,
 		.dumpit = devlink_nl_cmd_trap_group_get_dumpit,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 		/* can be retrieved by unprivileged users */
 	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_GROUP_SET,
 		.doit = devlink_nl_cmd_trap_group_set_doit,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_POLICER_GET,
 		.doit = devlink_nl_cmd_trap_policer_get_doit,
 		.dumpit = devlink_nl_cmd_trap_policer_get_dumpit,
 		/* can be retrieved by unprivileged users */
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 	{
 		.cmd = DEVLINK_CMD_TRAP_POLICER_SET,
 		.doit = devlink_nl_cmd_trap_policer_set_doit,
 		.flags = GENL_ADMIN_PERM,
+		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
 	},
 };
 
@@ -7593,6 +7683,10 @@ int devlink_port_register(struct devlink *devlink,
 	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
 	devlink_port_type_warn_schedule(devlink_port);
 	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
+	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_list);
+	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_group_list);
+	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_policer_list);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(devlink_port_register);

@@ -9084,7 +9178,8 @@ static void devlink_trap_disable(struct devlink *devlink,
 	if (WARN_ON_ONCE(!trap_item))
 		return;
 
-	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP, NULL);
+	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP,
+					     trap_item, NULL);
 	trap_item->action = DEVLINK_TRAP_ACTION_DROP;
 }
 
@@ -9532,6 +9627,233 @@ devlink_trap_policers_unregister(struct devlink *devlink,
 }
 EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
 
+/**
+ * devlink_port_traps_ops - Register trap callbacks
+ * @devlink_port: devlink_port.
+ * @ops: trap ops
+ */
+void devlink_port_traps_ops(struct devlink_port *devlink_port,
+			    const struct devlink_trap_ops *ops)
+{
+	devlink_port->trap_mngr.trap_ops = ops;
+}
+EXPORT_SYMBOL_GPL(devlink_port_traps_ops);
+
+/**
+ * devlink_port_traps_register - Register packet traps with devlink
+ * port.
+ * @devlink_port: devlink_port.
+ * @traps: Packet traps.
+ * @traps_count: Count of provided packet traps.
+ * @priv: Driver private information.
+ *
+ * Return: Non-zero value on failure.
+ */
+int devlink_port_traps_register(struct devlink_port *devlink_port,
+				const struct devlink_trap *traps,
+				size_t traps_count, void *priv)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i, err;
+
+	if (!trap_mngr->trap_ops->trap_init || !trap_mngr->trap_ops->trap_action_set)
+		return -EINVAL;
+
+	mutex_lock(&devlink->lock);
+	for (i = 0; i < traps_count; i++) {
+		const struct devlink_trap *trap = &traps[i];
+
+		err = devlink_trap_verify(trap);
+		if (err)
+			goto err_trap_verify;
+
+		err = devlink_trap_register(devlink, trap_mngr, trap, priv);
+		if (err)
+			goto err_trap_register;
+	}
+	mutex_unlock(&devlink->lock);
+
+	return 0;
+
+err_trap_register:
+err_trap_verify:
+	for (i--; i >= 0; i--)
+		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
+	mutex_unlock(&devlink->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_traps_register);
+
+/**
+ * devlink_port_traps_unregister - Unregister packet traps from devlink_port.
+ * @devlink_port: devlink port.
+ * @traps: Packet traps.
+ * @traps_count: Count of provided packet traps.
+ */
+void devlink_port_traps_unregister(struct devlink_port *devlink_port,
+				   const struct devlink_trap *traps,
+				   size_t traps_count)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i;
+
+	mutex_lock(&devlink->lock);
+	/* Make sure we do not have any packets in-flight while unregistering
+	 * traps by disabling all of them and waiting for a grace period.
+	 */
+	for (i = traps_count - 1; i >= 0; i--)
+		devlink_trap_disable(devlink, trap_mngr, &traps[i]);
+	synchronize_rcu();
+	for (i = traps_count - 1; i >= 0; i--)
+		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_port_traps_unregister);
+
+/**
+ * devlink_port_trap_report - Report trapped packet to drop monitor.
+ * @devlink_port: devlink_port.
+ * @skb: Trapped packet.
+ * @trap_ctx: Trap context.
+ * @fa_cookie: Flow action cookie. Could be NULL.
+ */
+void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
+			      void *trap_ctx, const struct flow_action_cookie *fa_cookie)
+{
+	return devlink_trap_report(devlink_port->devlink, skb, trap_ctx, devlink_port,
+				   fa_cookie);
+}
+EXPORT_SYMBOL_GPL(devlink_port_trap_report);
+
+/**
+ * devlink_port_trap_groups_register - Register packet trap groups with devlink port.
+ * @devlink_port: devlink_port.
+ * @groups: Packet trap groups.
+ * @groups_count: Count of provided packet trap groups.
+ *
+ * Return: Non-zero value on failure.
+ */
+int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
+				      const struct devlink_trap_group *groups,
+				      size_t groups_count)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i, err;
+
+	mutex_lock(&devlink->lock);
+	for (i = 0; i < groups_count; i++) {
+		const struct devlink_trap_group *group = &groups[i];
+
+		err = devlink_trap_group_verify(group);
+		if (err)
+			goto err_trap_group_verify;
+
+		err = devlink_trap_group_register(devlink, trap_mngr, group);
+		if (err)
+			goto err_trap_group_register;
+	}
+	mutex_unlock(&devlink->lock);
+
+	return 0;
+
+err_trap_group_register:
+err_trap_group_verify:
+	for (i--; i >= 0; i--)
+		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
+	mutex_unlock(&devlink->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_trap_groups_register);
+
+/**
+ * devlink_port_trap_groups_unregister - Unregister packet trap groups from devlink port.
+ * @devlink_port: devlink_port.
+ * @groups: Packet trap groups.
+ * @groups_count: Count of provided packet trap groups.
+ */
+void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
+					 const struct devlink_trap_group *groups,
+					 size_t groups_count)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i;
+
+	mutex_lock(&devlink->lock);
+	for (i = groups_count - 1; i >= 0; i--)
+		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_port_trap_groups_unregister);
+
+/**
+ * devlink_port_trap_policers_register - Register packet trap policers with devlink port.
+ * @devlink_port: devlink_port.
+ * @policers: Packet trap policers.
+ * @policers_count: Count of provided packet trap policers.
+ *
+ * Return: Non-zero value on failure.
+ */
+int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
+					const struct devlink_trap_policer *policers,
+					size_t policers_count)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i, err;
+
+	mutex_lock(&devlink->lock);
+	for (i = 0; i < policers_count; i++) {
+		const struct devlink_trap_policer *policer = &policers[i];
+
+		if (WARN_ON(policer->id == 0 ||
+			    policer->max_rate < policer->min_rate ||
+			    policer->max_burst < policer->min_burst)) {
+			err = -EINVAL;
+			goto err_trap_policer_verify;
+		}
+
+		err = devlink_trap_policer_register(devlink, trap_mngr, policer);
+		if (err)
+			goto err_trap_policer_register;
+	}
+	mutex_unlock(&devlink->lock);
+
+	return 0;
+
+err_trap_policer_register:
+err_trap_policer_verify:
+	for (i--; i >= 0; i--)
+		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
+	mutex_unlock(&devlink->lock);
+	return err;
+}
+EXPORT_SYMBOL_GPL(devlink_port_trap_policers_register);
+
+/**
+ * devlink_port_trap_policers_unregister - Unregister packet trap policers from devlink_port
+ * @devlink_port: devlink_port.
+ * @policers: Packet trap policers.
+ * @policers_count: Count of provided packet trap policers.
+ */
+void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
+					   const struct devlink_trap_policer *policers,
+					   size_t policers_count)
+{
+	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
+	struct devlink *devlink = devlink_port->devlink;
+	int i;
+
+	mutex_lock(&devlink->lock);
+	for (i = policers_count - 1; i >= 0; i--)
+		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
+	mutex_unlock(&devlink->lock);
+}
+EXPORT_SYMBOL_GPL(devlink_port_trap_policers_unregister);
+
 static void __devlink_compat_running_version(struct devlink *devlink,
 					     char *buf, size_t len)
 {
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context
  2020-09-02 15:32 ` [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context Aya Levin
@ 2020-09-06 15:44   ` Ido Schimmel
  2020-09-07 16:52     ` Aya Levin
  2020-09-08 14:04     ` Jiri Pirko
  0 siblings, 2 replies; 12+ messages in thread
From: Ido Schimmel @ 2020-09-06 15:44 UTC (permalink / raw)
  To: Aya Levin
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel

On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:
> There are some cases where we would like to trap dropped packets only
> for a single port on a device without affecting the others. For that
> purpose trap_mngr was added to devlink_port and corresponding Trap API
> with devlink_port were added too.
> 
> Signed-off-by: Aya Levin <ayal@mellanox.com>
> ---
>  drivers/net/ethernet/mellanox/mlxsw/core.c |   1 +
>  include/net/devlink.h                      |  25 +++
>  net/core/devlink.c                         | 332 ++++++++++++++++++++++++++++-
>  3 files changed, 353 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
> index 97460f47e537..cb9567a6a90d 100644
> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
> @@ -1178,6 +1178,7 @@ static void mlxsw_devlink_trap_fini(struct devlink *devlink,
>  static int mlxsw_devlink_trap_action_set(struct devlink *devlink,
>  					 const struct devlink_trap *trap,
>  					 enum devlink_trap_action action,
> +					 void *trap_ctx,

This is an unrelated change.

>  					 struct netlink_ext_ack *extack)
>  {
>  	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
> diff --git a/include/net/devlink.h b/include/net/devlink.h
> index d387ea5518c3..b4897ee38209 100644
> --- a/include/net/devlink.h
> +++ b/include/net/devlink.h
> @@ -110,6 +110,7 @@ struct devlink_port {
>  	struct delayed_work type_warn_dw;
>  	struct list_head reporter_list;
>  	struct mutex reporters_lock; /* Protects reporter_list */
> +	struct devlink_trap_mngr trap_mngr;
>  };
>  
>  struct devlink_sb_pool_info {
> @@ -1108,6 +1109,7 @@ struct devlink_trap_ops {
>  	int (*trap_action_set)(struct devlink *devlink,
>  			       const struct devlink_trap *trap,
>  			       enum devlink_trap_action action,
> +			       void *trap_ctx,

Same.

>  			       struct netlink_ext_ack *extack);
>  	/**
>  	 * @trap_group_init: Trap group initialization function.
> @@ -1414,6 +1416,29 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>  				 const struct devlink_trap_policer *policers,
>  				 size_t policers_count);
>  
> +void devlink_port_traps_ops(struct devlink_port *devlink_port,
> +			    const struct devlink_trap_ops *ops);
> +int devlink_port_traps_register(struct devlink_port *devlink_port,
> +				const struct devlink_trap *traps,
> +				size_t traps_count, void *priv);
> +void devlink_port_traps_unregister(struct devlink_port *devlink_port,
> +				   const struct devlink_trap *traps,
> +				   size_t traps_count);
> +void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
> +			      void *trap_ctx, const struct flow_action_cookie *fa_cookie);
> +int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
> +				      const struct devlink_trap_group *groups,
> +				      size_t groups_count);
> +void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
> +					 const struct devlink_trap_group *groups,
> +					 size_t groups_count);
> +int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
> +					const struct devlink_trap_policer *policers,
> +					size_t policers_count);
> +void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
> +					   const struct devlink_trap_policer *policers,
> +					   size_t policers_count);

No driver is calling the last two functions, so lets not add them.

> +
>  #if IS_ENABLED(CONFIG_NET_DEVLINK)
>  
>  void devlink_compat_running_version(struct net_device *dev,
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index a30b5444289b..b13e1b40bf1c 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -6155,7 +6155,13 @@ struct devlink_trap_item {
>  static struct devlink_trap_mngr *
>  devlink_trap_get_trap_mngr_from_info(struct devlink *devlink, struct genl_info *info)
>  {
> -		return &devlink->trap_mngr;
> +	struct devlink_port *devlink_port;
> +
> +	devlink_port = devlink_port_get_from_attrs(devlink, info->attrs);
> +	if (IS_ERR(devlink_port))
> +		return  &devlink->trap_mngr;
> +	else
> +		return &devlink_port->trap_mngr;
>  }

I understand how this struct allows you to re-use a lot of code between
per-device and per-port traps, but it's mainly enabled by the fact that
you use the same netlink commands for both per-device and per-port
traps. Is this OK?

I see this is already done for health reporters, but it's inconsistent
with the devlink-param API:

DEVLINK_CMD_PARAM_GET
DEVLINK_CMD_PARAM_SET
DEVLINK_CMD_PARAM_NEW
DEVLINK_CMD_PARAM_DEL

DEVLINK_CMD_PORT_PARAM_GET
DEVLINK_CMD_PORT_PARAM_SET
DEVLINK_CMD_PORT_PARAM_NEW
DEVLINK_CMD_PORT_PARAM_DEL

And also with the general device/port commands:

DEVLINK_CMD_GET
DEVLINK_CMD_SET
DEVLINK_CMD_NEW
DEVLINK_CMD_DEL

DEVLINK_CMD_PORT_GET
DEVLINK_CMD_PORT_SET
DEVLINK_CMD_PORT_NEW
DEVLINK_CMD_PORT_DEL

Wouldn't it be cleaner to add new commands?

DEVLINK_CMD_PORT_TRAP_GET
DEVLINK_CMD_PORT_TRAP_SET
DEVLINK_CMD_PORT_TRAP_NEW
DEVLINK_CMD_PORT_TRAP_DEL

I think the health API is the exception in this case and therefore might
not be the best thing to mimic. IIUC, existing per-port health reporters
were exposed as per-device and later moved to be exposed as per-port
[1]:

"This patchset comes to fix a design issue as some health reporters
report on errors and run recovery on device level while the actual
functionality is on port level. As for the current implemented devlink
health reporters it is relevant only to Tx and Rx reporters of mlx5,
which has only one port, so no real effect on functionality, but this
should be fixed before more drivers will use devlink health reporters."

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96

Since we still don't have per-port traps, we can design it better from
the start.

Note that introducing new commands does not remove the benefit of code
re-use. You can still re-use 'struct devlink_trap_item' and similar
structs in a similar fashion to how the params code re-uses 'struct
devlink_param_item' between both per-device params and per-port params.

>  
>  static struct devlink_trap_policer_item *
> @@ -6382,6 +6388,7 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
>  {
>  	struct devlink_trap_mngr *trap_mngr;
>  	struct devlink_trap_item *trap_item;
> +	struct devlink_port *port;
>  	struct devlink *devlink;
>  	int start = cb->args[0];
>  	int idx = 0;
> @@ -6411,6 +6418,30 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
>  		}
>  		mutex_unlock(&devlink->lock);
>  	}
> +	list_for_each_entry(devlink, &devlink_list, list) {
> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> +			continue;
> +		list_for_each_entry(port, &devlink->port_list, list) {
> +			trap_mngr = &port->trap_mngr;
> +			mutex_lock(&devlink->lock);
> +			list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
> +				if (idx < start) {
> +					idx++;
> +					continue;
> +				}
> +				err = devlink_nl_trap_fill(msg, devlink, trap_item,
> +							   DEVLINK_CMD_TRAP_NEW,
> +							   NETLINK_CB(cb->skb).portid,
> +							   cb->nlh->nlmsg_seq,
> +							   NLM_F_MULTI);

You never patched devlink_nl_trap_fill(), so it will never fill
DEVLINK_ATTR_PORT_INDEX.

> +				if (err)
> +					goto out;
> +				idx++;
> +			}
> +			mutex_unlock(&devlink->lock);
> +		}
> +	}
> +
>  out:
>  	mutex_unlock(&devlink_mutex);
>  
> @@ -6433,7 +6464,7 @@ static int __devlink_trap_action_set(struct devlink *devlink,
>  	}
>  
>  	err = trap_mngr->trap_ops->trap_action_set(devlink, trap_item->trap,
> -						   trap_action, extack);
> +						   trap_action, trap_item, extack);

Unrelated change.

>  	if (err)
>  		return err;
>  
> @@ -6481,6 +6512,7 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb,
>  		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap");
>  		return -ENOENT;
>  	}
> +	return devlink_trap_action_set(devlink, trap_mngr, trap_item, info);

Looks like you return in the middle of the function?

>  
>  	err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info);
>  	if (err)
> @@ -6614,6 +6646,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
>  	struct devlink_trap_group_item *group_item;
>  	u32 portid = NETLINK_CB(cb->skb).portid;
>  	struct devlink_trap_mngr *trap_mngr;
> +	struct devlink_port *port;
>  	struct devlink *devlink;
>  	int start = cb->args[0];
>  	int idx = 0;
> @@ -6644,6 +6677,30 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
>  		}
>  		mutex_unlock(&devlink->lock);
>  	}
> +	list_for_each_entry(devlink, &devlink_list, list) {
> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> +			continue;
> +		list_for_each_entry(port, &devlink->port_list, list) {
> +			trap_mngr = &port->trap_mngr;
> +			mutex_lock(&devlink->lock);
> +			list_for_each_entry(group_item, &trap_mngr->trap_group_list, list) {
> +				if (idx < start) {
> +					idx++;
> +					continue;
> +				}
> +				err = devlink_nl_trap_group_fill(msg, devlink,
> +								 group_item, cmd,
> +								 portid,
> +								 cb->nlh->nlmsg_seq,
> +								 NLM_F_MULTI);

Same as before, you never fill DEVLINK_ATTR_PORT_INDEX despite this
being a per-port trap group.

> +				if (err)
> +					goto out;
> +				idx++;
> +			}
> +			mutex_unlock(&devlink->lock);
> +		}
> +	}
> +
>  out:
>  	mutex_unlock(&devlink_mutex);
>  
> @@ -6912,6 +6969,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
>  	struct devlink_trap_policer_item *policer_item;
>  	u32 portid = NETLINK_CB(cb->skb).portid;
>  	struct devlink_trap_mngr *trap_mngr;
> +	struct devlink_port *port;
>  	struct devlink *devlink;
>  	int start = cb->args[0];
>  	int idx = 0;
> @@ -6943,6 +7001,32 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
>  		}
>  		mutex_unlock(&devlink->lock);
>  	}
> +	list_for_each_entry(devlink, &devlink_list, list) {
> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
> +			continue;
> +		list_for_each_entry(port, &devlink->port_list, list) {
> +			trap_mngr = &port->trap_mngr;
> +			mutex_lock(&devlink->lock);
> +			list_for_each_entry(policer_item, &trap_mngr->trap_policer_list,
> +					    list) {
> +				if (idx < start) {
> +					idx++;
> +					continue;
> +				}
> +				err = devlink_nl_trap_policer_fill(msg, devlink,
> +								   policer_item, cmd,
> +								   portid,
> +								   cb->nlh->nlmsg_seq,
> +								   trap_mngr,
> +								   NLM_F_MULTI);

Same as before, but it's never used anyway so I don't think you should
add it if you don't have a use-case for per-port trap policers.

> +				if (err)
> +					goto out;
> +				idx++;
> +			}
> +			mutex_unlock(&devlink->lock);
> +		}
> +	}
> +
>  out:
>  	mutex_unlock(&devlink_mutex);
>  
> @@ -7348,34 +7432,40 @@ static const struct genl_ops devlink_nl_ops[] = {
>  		.cmd = DEVLINK_CMD_TRAP_GET,
>  		.doit = devlink_nl_cmd_trap_get_doit,
>  		.dumpit = devlink_nl_cmd_trap_get_dumpit,
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  		/* can be retrieved by unprivileged users */
>  	},
>  	{
>  		.cmd = DEVLINK_CMD_TRAP_SET,
>  		.doit = devlink_nl_cmd_trap_set_doit,
>  		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  	},
>  	{
>  		.cmd = DEVLINK_CMD_TRAP_GROUP_GET,
>  		.doit = devlink_nl_cmd_trap_group_get_doit,
>  		.dumpit = devlink_nl_cmd_trap_group_get_dumpit,
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  		/* can be retrieved by unprivileged users */
>  	},
>  	{
>  		.cmd = DEVLINK_CMD_TRAP_GROUP_SET,
>  		.doit = devlink_nl_cmd_trap_group_set_doit,
>  		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  	},
>  	{
>  		.cmd = DEVLINK_CMD_TRAP_POLICER_GET,
>  		.doit = devlink_nl_cmd_trap_policer_get_doit,
>  		.dumpit = devlink_nl_cmd_trap_policer_get_dumpit,
>  		/* can be retrieved by unprivileged users */
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  	},
>  	{
>  		.cmd = DEVLINK_CMD_TRAP_POLICER_SET,
>  		.doit = devlink_nl_cmd_trap_policer_set_doit,
>  		.flags = GENL_ADMIN_PERM,
> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>  	},
>  };
>  
> @@ -7593,6 +7683,10 @@ int devlink_port_register(struct devlink *devlink,
>  	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
>  	devlink_port_type_warn_schedule(devlink_port);
>  	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_list);
> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_group_list);
> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_policer_list);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(devlink_port_register);
> 
> @@ -9084,7 +9178,8 @@ static void devlink_trap_disable(struct devlink *devlink,
>  	if (WARN_ON_ONCE(!trap_item))
>  		return;
>  
> -	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP, NULL);
> +	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP,
> +					     trap_item, NULL);

Unrelated change.

>  	trap_item->action = DEVLINK_TRAP_ACTION_DROP;
>  }
>  
> @@ -9532,6 +9627,233 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>  }
>  EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
>  
> +/**
> + * devlink_port_traps_ops - Register trap callbacks
> + * @devlink_port: devlink_port.
> + * @ops: trap ops
> + */
> +void devlink_port_traps_ops(struct devlink_port *devlink_port,
> +			    const struct devlink_trap_ops *ops)
> +{
> +	devlink_port->trap_mngr.trap_ops = ops;
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_traps_ops);
> +
> +/**
> + * devlink_port_traps_register - Register packet traps with devlink
> + * port.
> + * @devlink_port: devlink_port.
> + * @traps: Packet traps.
> + * @traps_count: Count of provided packet traps.
> + * @priv: Driver private information.
> + *
> + * Return: Non-zero value on failure.
> + */
> +int devlink_port_traps_register(struct devlink_port *devlink_port,
> +				const struct devlink_trap *traps,
> +				size_t traps_count, void *priv)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i, err;
> +
> +	if (!trap_mngr->trap_ops->trap_init || !trap_mngr->trap_ops->trap_action_set)
> +		return -EINVAL;
> +
> +	mutex_lock(&devlink->lock);
> +	for (i = 0; i < traps_count; i++) {
> +		const struct devlink_trap *trap = &traps[i];
> +
> +		err = devlink_trap_verify(trap);
> +		if (err)
> +			goto err_trap_verify;
> +
> +		err = devlink_trap_register(devlink, trap_mngr, trap, priv);
> +		if (err)
> +			goto err_trap_register;
> +	}
> +	mutex_unlock(&devlink->lock);
> +
> +	return 0;
> +
> +err_trap_register:
> +err_trap_verify:
> +	for (i--; i >= 0; i--)
> +		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
> +	mutex_unlock(&devlink->lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_traps_register);
> +
> +/**
> + * devlink_port_traps_unregister - Unregister packet traps from devlink_port.
> + * @devlink_port: devlink port.
> + * @traps: Packet traps.
> + * @traps_count: Count of provided packet traps.
> + */
> +void devlink_port_traps_unregister(struct devlink_port *devlink_port,
> +				   const struct devlink_trap *traps,
> +				   size_t traps_count)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i;
> +
> +	mutex_lock(&devlink->lock);
> +	/* Make sure we do not have any packets in-flight while unregistering
> +	 * traps by disabling all of them and waiting for a grace period.
> +	 */
> +	for (i = traps_count - 1; i >= 0; i--)
> +		devlink_trap_disable(devlink, trap_mngr, &traps[i]);
> +	synchronize_rcu();
> +	for (i = traps_count - 1; i >= 0; i--)
> +		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
> +	mutex_unlock(&devlink->lock);
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_traps_unregister);
> +
> +/**
> + * devlink_port_trap_report - Report trapped packet to drop monitor.
> + * @devlink_port: devlink_port.
> + * @skb: Trapped packet.
> + * @trap_ctx: Trap context.
> + * @fa_cookie: Flow action cookie. Could be NULL.
> + */
> +void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
> +			      void *trap_ctx, const struct flow_action_cookie *fa_cookie)
> +{
> +	return devlink_trap_report(devlink_port->devlink, skb, trap_ctx, devlink_port,
> +				   fa_cookie);
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_trap_report);
> +
> +/**
> + * devlink_port_trap_groups_register - Register packet trap groups with devlink port.
> + * @devlink_port: devlink_port.
> + * @groups: Packet trap groups.
> + * @groups_count: Count of provided packet trap groups.
> + *
> + * Return: Non-zero value on failure.
> + */
> +int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
> +				      const struct devlink_trap_group *groups,
> +				      size_t groups_count)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i, err;
> +
> +	mutex_lock(&devlink->lock);
> +	for (i = 0; i < groups_count; i++) {
> +		const struct devlink_trap_group *group = &groups[i];
> +
> +		err = devlink_trap_group_verify(group);
> +		if (err)
> +			goto err_trap_group_verify;
> +
> +		err = devlink_trap_group_register(devlink, trap_mngr, group);
> +		if (err)
> +			goto err_trap_group_register;
> +	}
> +	mutex_unlock(&devlink->lock);
> +
> +	return 0;
> +
> +err_trap_group_register:
> +err_trap_group_verify:
> +	for (i--; i >= 0; i--)
> +		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
> +	mutex_unlock(&devlink->lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_trap_groups_register);
> +
> +/**
> + * devlink_port_trap_groups_unregister - Unregister packet trap groups from devlink port.
> + * @devlink_port: devlink_port.
> + * @groups: Packet trap groups.
> + * @groups_count: Count of provided packet trap groups.
> + */
> +void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
> +					 const struct devlink_trap_group *groups,
> +					 size_t groups_count)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i;
> +
> +	mutex_lock(&devlink->lock);
> +	for (i = groups_count - 1; i >= 0; i--)
> +		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
> +	mutex_unlock(&devlink->lock);
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_trap_groups_unregister);
> +
> +/**
> + * devlink_port_trap_policers_register - Register packet trap policers with devlink port.
> + * @devlink_port: devlink_port.
> + * @policers: Packet trap policers.
> + * @policers_count: Count of provided packet trap policers.
> + *
> + * Return: Non-zero value on failure.
> + */
> +int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
> +					const struct devlink_trap_policer *policers,
> +					size_t policers_count)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i, err;
> +
> +	mutex_lock(&devlink->lock);
> +	for (i = 0; i < policers_count; i++) {
> +		const struct devlink_trap_policer *policer = &policers[i];
> +
> +		if (WARN_ON(policer->id == 0 ||
> +			    policer->max_rate < policer->min_rate ||
> +			    policer->max_burst < policer->min_burst)) {
> +			err = -EINVAL;
> +			goto err_trap_policer_verify;
> +		}
> +
> +		err = devlink_trap_policer_register(devlink, trap_mngr, policer);
> +		if (err)
> +			goto err_trap_policer_register;
> +	}
> +	mutex_unlock(&devlink->lock);
> +
> +	return 0;
> +
> +err_trap_policer_register:
> +err_trap_policer_verify:
> +	for (i--; i >= 0; i--)
> +		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
> +	mutex_unlock(&devlink->lock);
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_trap_policers_register);
> +
> +/**
> + * devlink_port_trap_policers_unregister - Unregister packet trap policers from devlink_port
> + * @devlink_port: devlink_port.
> + * @policers: Packet trap policers.
> + * @policers_count: Count of provided packet trap policers.
> + */
> +void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
> +					   const struct devlink_trap_policer *policers,
> +					   size_t policers_count)
> +{
> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
> +	struct devlink *devlink = devlink_port->devlink;
> +	int i;
> +
> +	mutex_lock(&devlink->lock);
> +	for (i = policers_count - 1; i >= 0; i--)
> +		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
> +	mutex_unlock(&devlink->lock);
> +}
> +EXPORT_SYMBOL_GPL(devlink_port_trap_policers_unregister);
> +
>  static void __devlink_compat_running_version(struct devlink *devlink,
>  					     char *buf, size_t len)
>  {
> -- 
> 2.14.1
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context
  2020-09-06 15:44   ` Ido Schimmel
@ 2020-09-07 16:52     ` Aya Levin
  2020-09-08 14:04     ` Jiri Pirko
  1 sibling, 0 replies; 12+ messages in thread
From: Aya Levin @ 2020-09-07 16:52 UTC (permalink / raw)
  To: Ido Schimmel, Aya Levin
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel



On 9/6/2020 6:44 PM, Ido Schimmel wrote:
> On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:
>> There are some cases where we would like to trap dropped packets only
>> for a single port on a device without affecting the others. For that
>> purpose trap_mngr was added to devlink_port and corresponding Trap API
>> with devlink_port were added too.
>>
>> Signed-off-by: Aya Levin <ayal@mellanox.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlxsw/core.c |   1 +
>>   include/net/devlink.h                      |  25 +++
>>   net/core/devlink.c                         | 332 ++++++++++++++++++++++++++++-
>>   3 files changed, 353 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
>> index 97460f47e537..cb9567a6a90d 100644
>> --- a/drivers/net/ethernet/mellanox/mlxsw/core.c
>> +++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
>> @@ -1178,6 +1178,7 @@ static void mlxsw_devlink_trap_fini(struct devlink *devlink,
>>   static int mlxsw_devlink_trap_action_set(struct devlink *devlink,
>>   					 const struct devlink_trap *trap,
>>   					 enum devlink_trap_action action,
>> +					 void *trap_ctx,
> 
> This is an unrelated change.
> 
>>   					 struct netlink_ext_ack *extack)
>>   {
>>   	struct mlxsw_core *mlxsw_core = devlink_priv(devlink);
>> diff --git a/include/net/devlink.h b/include/net/devlink.h
>> index d387ea5518c3..b4897ee38209 100644
>> --- a/include/net/devlink.h
>> +++ b/include/net/devlink.h
>> @@ -110,6 +110,7 @@ struct devlink_port {
>>   	struct delayed_work type_warn_dw;
>>   	struct list_head reporter_list;
>>   	struct mutex reporters_lock; /* Protects reporter_list */
>> +	struct devlink_trap_mngr trap_mngr;
>>   };
>>   
>>   struct devlink_sb_pool_info {
>> @@ -1108,6 +1109,7 @@ struct devlink_trap_ops {
>>   	int (*trap_action_set)(struct devlink *devlink,
>>   			       const struct devlink_trap *trap,
>>   			       enum devlink_trap_action action,
>> +			       void *trap_ctx,
> 
> Same.
This change is related in the sense that it allows flexability to the 
callback which needs the devlink_port and not devlink as an input.
I agree this is not pretty.
> 
>>   			       struct netlink_ext_ack *extack);
>>   	/**
>>   	 * @trap_group_init: Trap group initialization function.
>> @@ -1414,6 +1416,29 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>>   				 const struct devlink_trap_policer *policers,
>>   				 size_t policers_count);
>>   
>> +void devlink_port_traps_ops(struct devlink_port *devlink_port,
>> +			    const struct devlink_trap_ops *ops);
>> +int devlink_port_traps_register(struct devlink_port *devlink_port,
>> +				const struct devlink_trap *traps,
>> +				size_t traps_count, void *priv);
>> +void devlink_port_traps_unregister(struct devlink_port *devlink_port,
>> +				   const struct devlink_trap *traps,
>> +				   size_t traps_count);
>> +void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
>> +			      void *trap_ctx, const struct flow_action_cookie *fa_cookie);
>> +int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
>> +				      const struct devlink_trap_group *groups,
>> +				      size_t groups_count);
>> +void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
>> +					 const struct devlink_trap_group *groups,
>> +					 size_t groups_count);
>> +int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
>> +					const struct devlink_trap_policer *policers,
>> +					size_t policers_count);
>> +void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
>> +					   const struct devlink_trap_policer *policers,
>> +					   size_t policers_count);
> 
> No driver is calling the last two functions, so lets not add them.
> 
>> +
>>   #if IS_ENABLED(CONFIG_NET_DEVLINK)
>>   
>>   void devlink_compat_running_version(struct net_device *dev,
>> diff --git a/net/core/devlink.c b/net/core/devlink.c
>> index a30b5444289b..b13e1b40bf1c 100644
>> --- a/net/core/devlink.c
>> +++ b/net/core/devlink.c
>> @@ -6155,7 +6155,13 @@ struct devlink_trap_item {
>>   static struct devlink_trap_mngr *
>>   devlink_trap_get_trap_mngr_from_info(struct devlink *devlink, struct genl_info *info)
>>   {
>> -		return &devlink->trap_mngr;
>> +	struct devlink_port *devlink_port;
>> +
>> +	devlink_port = devlink_port_get_from_attrs(devlink, info->attrs);
>> +	if (IS_ERR(devlink_port))
>> +		return  &devlink->trap_mngr;
>> +	else
>> +		return &devlink_port->trap_mngr;
>>   }
> 
> I understand how this struct allows you to re-use a lot of code between
> per-device and per-port traps, but it's mainly enabled by the fact that
> you use the same netlink commands for both per-device and per-port
> traps. Is this OK?
> 
> I see this is already done for health reporters, but it's inconsistent
> with the devlink-param API:
> 
> DEVLINK_CMD_PARAM_GET
> DEVLINK_CMD_PARAM_SET
> DEVLINK_CMD_PARAM_NEW
> DEVLINK_CMD_PARAM_DEL
> 
> DEVLINK_CMD_PORT_PARAM_GET
> DEVLINK_CMD_PORT_PARAM_SET
> DEVLINK_CMD_PORT_PARAM_NEW
> DEVLINK_CMD_PORT_PARAM_DEL
> 
> And also with the general device/port commands:
> 
> DEVLINK_CMD_GET
> DEVLINK_CMD_SET
> DEVLINK_CMD_NEW
> DEVLINK_CMD_DEL
> 
> DEVLINK_CMD_PORT_GET
> DEVLINK_CMD_PORT_SET
> DEVLINK_CMD_PORT_NEW
> DEVLINK_CMD_PORT_DEL
> 
> Wouldn't it be cleaner to add new commands?
I am open for adding new commands although it will reduce code re-use. 
On the other hand it will symplify the implementation.
> 
> DEVLINK_CMD_PORT_TRAP_GET
> DEVLINK_CMD_PORT_TRAP_SET
> DEVLINK_CMD_PORT_TRAP_NEW
> DEVLINK_CMD_PORT_TRAP_DEL

DEVLINK_CMD_PORT_TRAP_GROUP_GET
DEVLINK_CMD_PORT_TRAP_GROUP_SET
DEVLINK_CMD_PORT_TRAP_GROUP_NEW
DEVLINK_CMD_PORT_TRAP_GROUP_DEL
and the same for policer eventually.
This will inflate the code - but in a cleaner way :-)
> 
> I think the health API is the exception in this case and therefore might
> not be the best thing to mimic. IIUC, existing per-port health reporters
> were exposed as per-device and later moved to be exposed as per-port
> [1]:
> 
> "This patchset comes to fix a design issue as some health reporters
> report on errors and run recovery on device level while the actual
> functionality is on port level. As for the current implemented devlink
> health reporters it is relevant only to Tx and Rx reporters of mlx5,
> which has only one port, so no real effect on functionality, but this
> should be fixed before more drivers will use devlink health reporters."
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96
> 
> Since we still don't have per-port traps, we can design it better from
> the start.
I tried to fit into the current trap design that is a little rigid.
> 
> Note that introducing new commands does not remove the benefit of code
> re-use. You can still re-use 'struct devlink_trap_item' and similar
> structs in a similar fashion to how the params code re-uses 'struct
> devlink_param_item' between both per-device params and per-port params.

Thanks a lot for your input!
I'll wait for more comments before V2
> 
>>   
>>   static struct devlink_trap_policer_item *
>> @@ -6382,6 +6388,7 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
>>   {
>>   	struct devlink_trap_mngr *trap_mngr;
>>   	struct devlink_trap_item *trap_item;
>> +	struct devlink_port *port;
>>   	struct devlink *devlink;
>>   	int start = cb->args[0];
>>   	int idx = 0;
>> @@ -6411,6 +6418,30 @@ static int devlink_nl_cmd_trap_get_dumpit(struct sk_buff *msg,
>>   		}
>>   		mutex_unlock(&devlink->lock);
>>   	}
>> +	list_for_each_entry(devlink, &devlink_list, list) {
>> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> +			continue;
>> +		list_for_each_entry(port, &devlink->port_list, list) {
>> +			trap_mngr = &port->trap_mngr;
>> +			mutex_lock(&devlink->lock);
>> +			list_for_each_entry(trap_item, &trap_mngr->trap_list, list) {
>> +				if (idx < start) {
>> +					idx++;
>> +					continue;
>> +				}
>> +				err = devlink_nl_trap_fill(msg, devlink, trap_item,
>> +							   DEVLINK_CMD_TRAP_NEW,
>> +							   NETLINK_CB(cb->skb).portid,
>> +							   cb->nlh->nlmsg_seq,
>> +							   NLM_F_MULTI);
> 
> You never patched devlink_nl_trap_fill(), so it will never fill
> DEVLINK_ATTR_PORT_INDEX.
nice catch :-)
> 
>> +				if (err)
>> +					goto out;
>> +				idx++;
>> +			}
>> +			mutex_unlock(&devlink->lock);
>> +		}
>> +	}
>> +
>>   out:
>>   	mutex_unlock(&devlink_mutex);
>>   
>> @@ -6433,7 +6464,7 @@ static int __devlink_trap_action_set(struct devlink *devlink,
>>   	}
>>   
>>   	err = trap_mngr->trap_ops->trap_action_set(devlink, trap_item->trap,
>> -						   trap_action, extack);
>> +						   trap_action, trap_item, extack);
> 
> Unrelated change.
> 
>>   	if (err)
>>   		return err;
>>   
>> @@ -6481,6 +6512,7 @@ static int devlink_nl_cmd_trap_set_doit(struct sk_buff *skb,
>>   		NL_SET_ERR_MSG_MOD(extack, "Device did not register this trap");
>>   		return -ENOENT;
>>   	}
>> +	return devlink_trap_action_set(devlink, trap_mngr, trap_item, info);
> 
> Looks like you return in the middle of the function?
> 
>>   
>>   	err = devlink_trap_action_set(devlink, trap_mngr, trap_item, info);
>>   	if (err)
>> @@ -6614,6 +6646,7 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
>>   	struct devlink_trap_group_item *group_item;
>>   	u32 portid = NETLINK_CB(cb->skb).portid;
>>   	struct devlink_trap_mngr *trap_mngr;
>> +	struct devlink_port *port;
>>   	struct devlink *devlink;
>>   	int start = cb->args[0];
>>   	int idx = 0;
>> @@ -6644,6 +6677,30 @@ static int devlink_nl_cmd_trap_group_get_dumpit(struct sk_buff *msg,
>>   		}
>>   		mutex_unlock(&devlink->lock);
>>   	}
>> +	list_for_each_entry(devlink, &devlink_list, list) {
>> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> +			continue;
>> +		list_for_each_entry(port, &devlink->port_list, list) {
>> +			trap_mngr = &port->trap_mngr;
>> +			mutex_lock(&devlink->lock);
>> +			list_for_each_entry(group_item, &trap_mngr->trap_group_list, list) {
>> +				if (idx < start) {
>> +					idx++;
>> +					continue;
>> +				}
>> +				err = devlink_nl_trap_group_fill(msg, devlink,
>> +								 group_item, cmd,
>> +								 portid,
>> +								 cb->nlh->nlmsg_seq,
>> +								 NLM_F_MULTI);
> 
> Same as before, you never fill DEVLINK_ATTR_PORT_INDEX despite this
> being a per-port trap group.
> 
>> +				if (err)
>> +					goto out;
>> +				idx++;
>> +			}
>> +			mutex_unlock(&devlink->lock);
>> +		}
>> +	}
>> +
>>   out:
>>   	mutex_unlock(&devlink_mutex);
>>   
>> @@ -6912,6 +6969,7 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
>>   	struct devlink_trap_policer_item *policer_item;
>>   	u32 portid = NETLINK_CB(cb->skb).portid;
>>   	struct devlink_trap_mngr *trap_mngr;
>> +	struct devlink_port *port;
>>   	struct devlink *devlink;
>>   	int start = cb->args[0];
>>   	int idx = 0;
>> @@ -6943,6 +7001,32 @@ static int devlink_nl_cmd_trap_policer_get_dumpit(struct sk_buff *msg,
>>   		}
>>   		mutex_unlock(&devlink->lock);
>>   	}
>> +	list_for_each_entry(devlink, &devlink_list, list) {
>> +		if (!net_eq(devlink_net(devlink), sock_net(msg->sk)))
>> +			continue;
>> +		list_for_each_entry(port, &devlink->port_list, list) {
>> +			trap_mngr = &port->trap_mngr;
>> +			mutex_lock(&devlink->lock);
>> +			list_for_each_entry(policer_item, &trap_mngr->trap_policer_list,
>> +					    list) {
>> +				if (idx < start) {
>> +					idx++;
>> +					continue;
>> +				}
>> +				err = devlink_nl_trap_policer_fill(msg, devlink,
>> +								   policer_item, cmd,
>> +								   portid,
>> +								   cb->nlh->nlmsg_seq,
>> +								   trap_mngr,
>> +								   NLM_F_MULTI);
> 
> Same as before, but it's never used anyway so I don't think you should
> add it if you don't have a use-case for per-port trap policers.
> 
>> +				if (err)
>> +					goto out;
>> +				idx++;
>> +			}
>> +			mutex_unlock(&devlink->lock);
>> +		}
>> +	}
>> +
>>   out:
>>   	mutex_unlock(&devlink_mutex);
>>   
>> @@ -7348,34 +7432,40 @@ static const struct genl_ops devlink_nl_ops[] = {
>>   		.cmd = DEVLINK_CMD_TRAP_GET,
>>   		.doit = devlink_nl_cmd_trap_get_doit,
>>   		.dumpit = devlink_nl_cmd_trap_get_dumpit,
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   		/* can be retrieved by unprivileged users */
>>   	},
>>   	{
>>   		.cmd = DEVLINK_CMD_TRAP_SET,
>>   		.doit = devlink_nl_cmd_trap_set_doit,
>>   		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   	},
>>   	{
>>   		.cmd = DEVLINK_CMD_TRAP_GROUP_GET,
>>   		.doit = devlink_nl_cmd_trap_group_get_doit,
>>   		.dumpit = devlink_nl_cmd_trap_group_get_dumpit,
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   		/* can be retrieved by unprivileged users */
>>   	},
>>   	{
>>   		.cmd = DEVLINK_CMD_TRAP_GROUP_SET,
>>   		.doit = devlink_nl_cmd_trap_group_set_doit,
>>   		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   	},
>>   	{
>>   		.cmd = DEVLINK_CMD_TRAP_POLICER_GET,
>>   		.doit = devlink_nl_cmd_trap_policer_get_doit,
>>   		.dumpit = devlink_nl_cmd_trap_policer_get_dumpit,
>>   		/* can be retrieved by unprivileged users */
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   	},
>>   	{
>>   		.cmd = DEVLINK_CMD_TRAP_POLICER_SET,
>>   		.doit = devlink_nl_cmd_trap_policer_set_doit,
>>   		.flags = GENL_ADMIN_PERM,
>> +		.internal_flags = DEVLINK_NL_FLAG_NEED_DEVLINK_OR_PORT,
>>   	},
>>   };
>>   
>> @@ -7593,6 +7683,10 @@ int devlink_port_register(struct devlink *devlink,
>>   	INIT_DELAYED_WORK(&devlink_port->type_warn_dw, &devlink_port_type_warn);
>>   	devlink_port_type_warn_schedule(devlink_port);
>>   	devlink_port_notify(devlink_port, DEVLINK_CMD_PORT_NEW);
>> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_list);
>> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_group_list);
>> +	INIT_LIST_HEAD(&devlink_port->trap_mngr.trap_policer_list);
>> +
>>   	return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(devlink_port_register);
>>
>> @@ -9084,7 +9178,8 @@ static void devlink_trap_disable(struct devlink *devlink,
>>   	if (WARN_ON_ONCE(!trap_item))
>>   		return;
>>   
>> -	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP, NULL);
>> +	trap_mngr->trap_ops->trap_action_set(devlink, trap, DEVLINK_TRAP_ACTION_DROP,
>> +					     trap_item, NULL);
> 
> Unrelated change.
> 
>>   	trap_item->action = DEVLINK_TRAP_ACTION_DROP;
>>   }
>>   
>> @@ -9532,6 +9627,233 @@ devlink_trap_policers_unregister(struct devlink *devlink,
>>   }
>>   EXPORT_SYMBOL_GPL(devlink_trap_policers_unregister);
>>   
>> +/**
>> + * devlink_port_traps_ops - Register trap callbacks
>> + * @devlink_port: devlink_port.
>> + * @ops: trap ops
>> + */
>> +void devlink_port_traps_ops(struct devlink_port *devlink_port,
>> +			    const struct devlink_trap_ops *ops)
>> +{
>> +	devlink_port->trap_mngr.trap_ops = ops;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_traps_ops);
>> +
>> +/**
>> + * devlink_port_traps_register - Register packet traps with devlink
>> + * port.
>> + * @devlink_port: devlink_port.
>> + * @traps: Packet traps.
>> + * @traps_count: Count of provided packet traps.
>> + * @priv: Driver private information.
>> + *
>> + * Return: Non-zero value on failure.
>> + */
>> +int devlink_port_traps_register(struct devlink_port *devlink_port,
>> +				const struct devlink_trap *traps,
>> +				size_t traps_count, void *priv)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i, err;
>> +
>> +	if (!trap_mngr->trap_ops->trap_init || !trap_mngr->trap_ops->trap_action_set)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	for (i = 0; i < traps_count; i++) {
>> +		const struct devlink_trap *trap = &traps[i];
>> +
>> +		err = devlink_trap_verify(trap);
>> +		if (err)
>> +			goto err_trap_verify;
>> +
>> +		err = devlink_trap_register(devlink, trap_mngr, trap, priv);
>> +		if (err)
>> +			goto err_trap_register;
>> +	}
>> +	mutex_unlock(&devlink->lock);
>> +
>> +	return 0;
>> +
>> +err_trap_register:
>> +err_trap_verify:
>> +	for (i--; i >= 0; i--)
>> +		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
>> +	mutex_unlock(&devlink->lock);
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_traps_register);
>> +
>> +/**
>> + * devlink_port_traps_unregister - Unregister packet traps from devlink_port.
>> + * @devlink_port: devlink port.
>> + * @traps: Packet traps.
>> + * @traps_count: Count of provided packet traps.
>> + */
>> +void devlink_port_traps_unregister(struct devlink_port *devlink_port,
>> +				   const struct devlink_trap *traps,
>> +				   size_t traps_count)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	/* Make sure we do not have any packets in-flight while unregistering
>> +	 * traps by disabling all of them and waiting for a grace period.
>> +	 */
>> +	for (i = traps_count - 1; i >= 0; i--)
>> +		devlink_trap_disable(devlink, trap_mngr, &traps[i]);
>> +	synchronize_rcu();
>> +	for (i = traps_count - 1; i >= 0; i--)
>> +		devlink_trap_unregister(devlink, trap_mngr, &traps[i]);
>> +	mutex_unlock(&devlink->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_traps_unregister);
>> +
>> +/**
>> + * devlink_port_trap_report - Report trapped packet to drop monitor.
>> + * @devlink_port: devlink_port.
>> + * @skb: Trapped packet.
>> + * @trap_ctx: Trap context.
>> + * @fa_cookie: Flow action cookie. Could be NULL.
>> + */
>> +void devlink_port_trap_report(struct devlink_port *devlink_port, struct sk_buff *skb,
>> +			      void *trap_ctx, const struct flow_action_cookie *fa_cookie)
>> +{
>> +	return devlink_trap_report(devlink_port->devlink, skb, trap_ctx, devlink_port,
>> +				   fa_cookie);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_trap_report);
>> +
>> +/**
>> + * devlink_port_trap_groups_register - Register packet trap groups with devlink port.
>> + * @devlink_port: devlink_port.
>> + * @groups: Packet trap groups.
>> + * @groups_count: Count of provided packet trap groups.
>> + *
>> + * Return: Non-zero value on failure.
>> + */
>> +int devlink_port_trap_groups_register(struct devlink_port *devlink_port,
>> +				      const struct devlink_trap_group *groups,
>> +				      size_t groups_count)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i, err;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	for (i = 0; i < groups_count; i++) {
>> +		const struct devlink_trap_group *group = &groups[i];
>> +
>> +		err = devlink_trap_group_verify(group);
>> +		if (err)
>> +			goto err_trap_group_verify;
>> +
>> +		err = devlink_trap_group_register(devlink, trap_mngr, group);
>> +		if (err)
>> +			goto err_trap_group_register;
>> +	}
>> +	mutex_unlock(&devlink->lock);
>> +
>> +	return 0;
>> +
>> +err_trap_group_register:
>> +err_trap_group_verify:
>> +	for (i--; i >= 0; i--)
>> +		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
>> +	mutex_unlock(&devlink->lock);
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_trap_groups_register);
>> +
>> +/**
>> + * devlink_port_trap_groups_unregister - Unregister packet trap groups from devlink port.
>> + * @devlink_port: devlink_port.
>> + * @groups: Packet trap groups.
>> + * @groups_count: Count of provided packet trap groups.
>> + */
>> +void devlink_port_trap_groups_unregister(struct devlink_port *devlink_port,
>> +					 const struct devlink_trap_group *groups,
>> +					 size_t groups_count)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	for (i = groups_count - 1; i >= 0; i--)
>> +		devlink_trap_group_unregister(devlink, trap_mngr, &groups[i]);
>> +	mutex_unlock(&devlink->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_trap_groups_unregister);
>> +
>> +/**
>> + * devlink_port_trap_policers_register - Register packet trap policers with devlink port.
>> + * @devlink_port: devlink_port.
>> + * @policers: Packet trap policers.
>> + * @policers_count: Count of provided packet trap policers.
>> + *
>> + * Return: Non-zero value on failure.
>> + */
>> +int devlink_port_trap_policers_register(struct devlink_port *devlink_port,
>> +					const struct devlink_trap_policer *policers,
>> +					size_t policers_count)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i, err;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	for (i = 0; i < policers_count; i++) {
>> +		const struct devlink_trap_policer *policer = &policers[i];
>> +
>> +		if (WARN_ON(policer->id == 0 ||
>> +			    policer->max_rate < policer->min_rate ||
>> +			    policer->max_burst < policer->min_burst)) {
>> +			err = -EINVAL;
>> +			goto err_trap_policer_verify;
>> +		}
>> +
>> +		err = devlink_trap_policer_register(devlink, trap_mngr, policer);
>> +		if (err)
>> +			goto err_trap_policer_register;
>> +	}
>> +	mutex_unlock(&devlink->lock);
>> +
>> +	return 0;
>> +
>> +err_trap_policer_register:
>> +err_trap_policer_verify:
>> +	for (i--; i >= 0; i--)
>> +		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
>> +	mutex_unlock(&devlink->lock);
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_trap_policers_register);
>> +
>> +/**
>> + * devlink_port_trap_policers_unregister - Unregister packet trap policers from devlink_port
>> + * @devlink_port: devlink_port.
>> + * @policers: Packet trap policers.
>> + * @policers_count: Count of provided packet trap policers.
>> + */
>> +void devlink_port_trap_policers_unregister(struct devlink_port *devlink_port,
>> +					   const struct devlink_trap_policer *policers,
>> +					   size_t policers_count)
>> +{
>> +	struct devlink_trap_mngr *trap_mngr = &devlink_port->devlink->trap_mngr;
>> +	struct devlink *devlink = devlink_port->devlink;
>> +	int i;
>> +
>> +	mutex_lock(&devlink->lock);
>> +	for (i = policers_count - 1; i >= 0; i--)
>> +		devlink_trap_policer_unregister(devlink, trap_mngr, &policers[i]);
>> +	mutex_unlock(&devlink->lock);
>> +}
>> +EXPORT_SYMBOL_GPL(devlink_port_trap_policers_unregister);
>> +
>>   static void __devlink_compat_running_version(struct devlink *devlink,
>>   					     char *buf, size_t len)
>>   {
>> -- 
>> 2.14.1
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context
  2020-09-06 15:44   ` Ido Schimmel
  2020-09-07 16:52     ` Aya Levin
@ 2020-09-08 14:04     ` Jiri Pirko
  2020-09-10  6:16       ` Aya Levin
  1 sibling, 1 reply; 12+ messages in thread
From: Jiri Pirko @ 2020-09-08 14:04 UTC (permalink / raw)
  To: Ido Schimmel
  Cc: Aya Levin, David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel

Sun, Sep 06, 2020 at 05:44:28PM CEST, idosch@idosch.org wrote:
>On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:

[...]

>
>I understand how this struct allows you to re-use a lot of code between
>per-device and per-port traps, but it's mainly enabled by the fact that
>you use the same netlink commands for both per-device and per-port
>traps. Is this OK?
>
>I see this is already done for health reporters, but it's inconsistent
>with the devlink-param API:
>
>DEVLINK_CMD_PARAM_GET
>DEVLINK_CMD_PARAM_SET
>DEVLINK_CMD_PARAM_NEW
>DEVLINK_CMD_PARAM_DEL
>
>DEVLINK_CMD_PORT_PARAM_GET
>DEVLINK_CMD_PORT_PARAM_SET
>DEVLINK_CMD_PORT_PARAM_NEW
>DEVLINK_CMD_PORT_PARAM_DEL
>
>And also with the general device/port commands:
>
>DEVLINK_CMD_GET
>DEVLINK_CMD_SET
>DEVLINK_CMD_NEW
>DEVLINK_CMD_DEL
>
>DEVLINK_CMD_PORT_GET
>DEVLINK_CMD_PORT_SET
>DEVLINK_CMD_PORT_NEW
>DEVLINK_CMD_PORT_DEL
>
>Wouldn't it be cleaner to add new commands?
>
>DEVLINK_CMD_PORT_TRAP_GET
>DEVLINK_CMD_PORT_TRAP_SET
>DEVLINK_CMD_PORT_TRAP_NEW
>DEVLINK_CMD_PORT_TRAP_DEL
>
>I think the health API is the exception in this case and therefore might
>not be the best thing to mimic. IIUC, existing per-port health reporters
>were exposed as per-device and later moved to be exposed as per-port
>[1]:
>
>"This patchset comes to fix a design issue as some health reporters
>report on errors and run recovery on device level while the actual
>functionality is on port level. As for the current implemented devlink
>health reporters it is relevant only to Tx and Rx reporters of mlx5,
>which has only one port, so no real effect on functionality, but this
>should be fixed before more drivers will use devlink health reporters."

Yeah, this slipped trough my fingers unfortunatelly :/ But with
introduction of per-port health reporters, we could introduce new
commands, that would be no problem. Pity :/


>
>[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96
>
>Since we still don't have per-port traps, we can design it better from
>the start.

I agree. Let's have a separate commands for per-port.


[...]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context
  2020-09-08 14:04     ` Jiri Pirko
@ 2020-09-10  6:16       ` Aya Levin
  0 siblings, 0 replies; 12+ messages in thread
From: Aya Levin @ 2020-09-10  6:16 UTC (permalink / raw)
  To: Jiri Pirko, Ido Schimmel
  Cc: Aya Levin, David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel



On 9/8/2020 5:04 PM, Jiri Pirko wrote:
> Sun, Sep 06, 2020 at 05:44:28PM CEST, idosch@idosch.org wrote:
>> On Wed, Sep 02, 2020 at 06:32:12PM +0300, Aya Levin wrote:
> 
> [...]
> 
>>
>> I understand how this struct allows you to re-use a lot of code between
>> per-device and per-port traps, but it's mainly enabled by the fact that
>> you use the same netlink commands for both per-device and per-port
>> traps. Is this OK?
>>
>> I see this is already done for health reporters, but it's inconsistent
>> with the devlink-param API:
>>
>> DEVLINK_CMD_PARAM_GET
>> DEVLINK_CMD_PARAM_SET
>> DEVLINK_CMD_PARAM_NEW
>> DEVLINK_CMD_PARAM_DEL
>>
>> DEVLINK_CMD_PORT_PARAM_GET
>> DEVLINK_CMD_PORT_PARAM_SET
>> DEVLINK_CMD_PORT_PARAM_NEW
>> DEVLINK_CMD_PORT_PARAM_DEL
>>
>> And also with the general device/port commands:
>>
>> DEVLINK_CMD_GET
>> DEVLINK_CMD_SET
>> DEVLINK_CMD_NEW
>> DEVLINK_CMD_DEL
>>
>> DEVLINK_CMD_PORT_GET
>> DEVLINK_CMD_PORT_SET
>> DEVLINK_CMD_PORT_NEW
>> DEVLINK_CMD_PORT_DEL
>>
>> Wouldn't it be cleaner to add new commands?
>>
>> DEVLINK_CMD_PORT_TRAP_GET
>> DEVLINK_CMD_PORT_TRAP_SET
>> DEVLINK_CMD_PORT_TRAP_NEW
>> DEVLINK_CMD_PORT_TRAP_DEL
>>
>> I think the health API is the exception in this case and therefore might
>> not be the best thing to mimic. IIUC, existing per-port health reporters
>> were exposed as per-device and later moved to be exposed as per-port
>> [1]:
>>
>> "This patchset comes to fix a design issue as some health reporters
>> report on errors and run recovery on device level while the actual
>> functionality is on port level. As for the current implemented devlink
>> health reporters it is relevant only to Tx and Rx reporters of mlx5,
>> which has only one port, so no real effect on functionality, but this
>> should be fixed before more drivers will use devlink health reporters."
> 
> Yeah, this slipped trough my fingers unfortunatelly :/ But with
> introduction of per-port health reporters, we could introduce new
> commands, that would be no problem. Pity :/
> 
> 
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ac4cd4781eacd1fd185c85522e869bd5d3254b96
>>
>> Since we still don't have per-port traps, we can design it better from
>> the start.
> 
> I agree. Let's have a separate commands for per-port.
Thanks for your input
I'm preparing V2
> 
> 
> [...]
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level
  2020-09-02 15:32 [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context Aya Levin
  2020-09-02 15:32 ` [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr Aya Levin
  2020-09-02 15:32 ` [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context Aya Levin
@ 2020-09-02 15:32 ` Aya Levin
  2020-09-06 15:58   ` Ido Schimmel
  2020-09-02 15:32 ` [PATCH net-next RFC v1 4/4] net/mlx5e: Add devlink trap to catch oversize packets Aya Levin
  3 siblings, 1 reply; 12+ messages in thread
From: Aya Levin @ 2020-09-02 15:32 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev
  Cc: Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel, Aya Levin

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(-)

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

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level
  2020-09-02 15:32 ` [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level Aya Levin
@ 2020-09-06 15:58   ` Ido Schimmel
  2020-09-07 16:26     ` Aya Levin
  0 siblings, 1 reply; 12+ messages in thread
From: Ido Schimmel @ 2020-09-06 15:58 UTC (permalink / raw)
  To: Aya Levin
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level
  2020-09-06 15:58   ` Ido Schimmel
@ 2020-09-07 16:26     ` Aya Levin
  0 siblings, 0 replies; 12+ messages in thread
From: Aya Levin @ 2020-09-07 16:26 UTC (permalink / raw)
  To: Ido Schimmel, Aya Levin
  Cc: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev,
	Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel



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
>>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH net-next RFC v1 4/4] net/mlx5e: Add devlink trap to catch oversize packets
  2020-09-02 15:32 [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context Aya Levin
                   ` (2 preceding siblings ...)
  2020-09-02 15:32 ` [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level Aya Levin
@ 2020-09-02 15:32 ` Aya Levin
  3 siblings, 0 replies; 12+ messages in thread
From: Aya Levin @ 2020-09-02 15:32 UTC (permalink / raw)
  To: David S. Miller, Jakub Kicinski, Jiri Pirko, netdev
  Cc: Moshe Shemesh, Eran Ben Elisha, Ido Schimmel, linux-kernel, Aya Levin

Register MTU error trap to allow visibility of oversize packets. Display
a naive use of devlink trap in devlink port context.

Signed-off-by: Aya Levin <ayal@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |  2 +-
 drivers/net/ethernet/mellanox/mlx5/core/en.h       |  2 ++
 drivers/net/ethernet/mellanox/mlx5/core/en/traps.c | 32 +++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en/traps.h | 13 +++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  | 41 ++++++++++++++++++++++
 drivers/net/ethernet/mellanox/mlx5/core/en_rx.c    | 11 ++++--
 6 files changed, 98 insertions(+), 3 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/traps.c
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en/traps.h

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 94268a697e1c..78e2e9107986 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -25,7 +25,7 @@ mlx5_core-$(CONFIG_MLX5_CORE_EN) += en_main.o en_common.o en_fs.o en_ethtool.o \
 		en_tx.o en_rx.o en_dim.o en_txrx.o en/xdp.o en_stats.o \
 		en_selftest.o en/port.o en/monitor_stats.o en/health.o \
 		en/reporter_tx.o en/reporter_rx.o en/params.o en/xsk/umem.o \
-		en/xsk/setup.o en/xsk/rx.o en/xsk/tx.o en/devlink.o
+		en/xsk/setup.o en/xsk/rx.o en/xsk/tx.o en/devlink.o en/traps.o
 
 #
 # Netdev extra
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 0cc2080fd847..7e5581ed9311 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -814,6 +814,8 @@ struct mlx5e_priv {
 	struct mlx5e_hv_vhca_stats_agent stats_agent;
 #endif
 	struct mlx5e_scratchpad    scratchpad;
+	void *trap_mtu;
+	bool trap_oversize;
 };
 
 struct mlx5e_rx_handlers {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/traps.c b/drivers/net/ethernet/mellanox/mlx5/core/en/traps.c
new file mode 100644
index 000000000000..e365e8dbd6ff
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/traps.c
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2020 Mellanox Technologies.
+#include "en.h"
+#include "traps.h"
+#include <linux/kernel.h>
+#include <uapi/linux/devlink.h>
+
+#define MLX5E_TRAP(_id, _group_id)                                      \
+	DEVLINK_TRAP_GENERIC(DROP, DROP, _id,                           \
+			     DEVLINK_TRAP_GROUP_GENERIC_ID_##_group_id, \
+			     DEVLINK_TRAP_METADATA_TYPE_F_IN_PORT)
+static struct devlink_trap mlx5e_traps_arr[] = {
+	MLX5E_TRAP(MTU_ERROR, L2_DROPS),
+};
+
+int mlx5e_devlink_traps_create(struct mlx5e_priv *priv)
+{
+	struct devlink_port *dl_port = &priv->dl_port;
+
+	return devlink_port_traps_register(dl_port, mlx5e_traps_arr,
+					   ARRAY_SIZE(mlx5e_traps_arr),
+					   priv);
+}
+
+void mlx5e_devlink_traps_destroy(struct mlx5e_priv *priv)
+{
+	struct devlink_port *dl_port = &priv->dl_port;
+
+	devlink_port_traps_unregister(dl_port, mlx5e_traps_arr,
+				      ARRAY_SIZE(mlx5e_traps_arr));
+}
+
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/traps.h b/drivers/net/ethernet/mellanox/mlx5/core/en/traps.h
new file mode 100644
index 000000000000..14a32b6968ee
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/traps.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2020 Mellanox Technologies.*/
+
+#ifndef __MLX5E_EN_TRAPS_H
+#define __MLX5E_EN_TRAPS_H
+
+#include "en.h"
+
+int mlx5e_devlink_traps_create(struct mlx5e_priv *priv);
+void mlx5e_devlink_traps_destroy(struct mlx5e_priv *priv);
+
+#endif
+
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index aebcf73f8546..056f34326b3d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -64,6 +64,7 @@
 #include "en/hv_vhca_stats.h"
 #include "en/devlink.h"
 #include "lib/mlx5.h"
+#include "en/traps.h"
 
 bool mlx5e_check_fragmented_striding_rq_cap(struct mlx5_core_dev *mdev)
 {
@@ -4976,6 +4977,43 @@ void mlx5e_destroy_q_counters(struct mlx5e_priv *priv)
 	}
 }
 
+static int mlx5e_devlink_trap_init(struct devlink *devlink,
+				   const struct devlink_trap *trap,
+				   void *trap_ctx)
+{
+	struct mlx5e_priv *priv = (struct mlx5e_priv *)devlink_trap_ctx_priv(trap_ctx);
+
+	priv->trap_mtu = trap_ctx;
+	return 0;
+}
+
+static void mlx5e_devlink_trap_fini(struct devlink *devlink,
+				    const struct devlink_trap *trap,
+				    void *trap_ctx)
+{
+	struct mlx5e_priv *priv = (struct mlx5e_priv *)devlink_trap_ctx_priv(trap_ctx);
+
+	priv->trap_mtu = NULL;
+}
+
+static int mlx5e_devlink_trap_action_set(struct devlink *devlink,
+					 const struct devlink_trap *trap,
+					 enum devlink_trap_action action,
+					 void *trap_ctx,
+					 struct netlink_ext_ack *extack)
+{
+	struct mlx5e_priv *priv = (struct mlx5e_priv *)devlink_trap_ctx_priv(trap_ctx);
+
+	priv->trap_oversize = !!action;
+	return 0;
+}
+
+static const struct devlink_trap_ops mlx5e_devlink_traps_ops = {
+	.trap_init		= mlx5e_devlink_trap_init,
+	.trap_fini		= mlx5e_devlink_trap_fini,
+	.trap_action_set	= mlx5e_devlink_trap_action_set,
+};
+
 static int mlx5e_nic_init(struct mlx5_core_dev *mdev,
 			  struct net_device *netdev,
 			  const struct mlx5e_profile *profile,
@@ -5005,12 +5043,15 @@ static int mlx5e_nic_init(struct mlx5_core_dev *mdev,
 	if (err)
 		mlx5_core_err(mdev, "mlx5e_devlink_port_register failed, %d\n", err);
 	mlx5e_health_create_reporters(priv);
+	devlink_port_traps_ops(&priv->dl_port, &mlx5e_devlink_traps_ops);
+	mlx5e_devlink_traps_create(priv);
 
 	return 0;
 }
 
 static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 {
+	mlx5e_devlink_traps_destroy(priv);
 	mlx5e_health_destroy_reporters(priv);
 	mlx5e_devlink_port_unregister(priv);
 	mlx5e_tls_cleanup(priv);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
index 1e42e27eae26..4fe20a6e6d6d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_rx.c
@@ -1437,6 +1437,7 @@ mlx5e_skb_from_cqe_mpwrq_linear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *wi,
 				u16 cqe_bcnt, u32 head_offset, u32 page_idx)
 {
 	struct mlx5e_dma_info *di = &wi->umr.dma_info[page_idx];
+	struct mlx5e_priv *priv = rq->channel->priv;
 	u16 rx_headroom = rq->buff.headroom;
 	u32 cqe_bcnt32 = cqe_bcnt;
 	struct xdp_buff xdp;
@@ -1444,11 +1445,14 @@ mlx5e_skb_from_cqe_mpwrq_linear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *wi,
 	void *va, *data;
 	u32 frag_size;
 	bool consumed;
+	bool trap;
 
 	/* Check packet size. Note LRO doesn't use linear SKB */
 	if (unlikely(cqe_bcnt > rq->hw_mtu)) {
 		rq->stats->oversize_pkts_sw_drop++;
-		return NULL;
+		if (!priv->trap_oversize)
+			return NULL;
+		trap = true;
 	}
 
 	va             = page_address(di->page) + head_offset;
@@ -1475,7 +1479,10 @@ mlx5e_skb_from_cqe_mpwrq_linear(struct mlx5e_rq *rq, struct mlx5e_mpw_info *wi,
 	skb = mlx5e_build_linear_skb(rq, va, frag_size, rx_headroom, cqe_bcnt32);
 	if (unlikely(!skb))
 		return NULL;
-
+	if (trap) {
+		devlink_port_trap_report(&priv->dl_port, skb, priv->trap_mtu, NULL);
+		return NULL;
+	}
 	/* queue up for recycling/reuse */
 	page_ref_inc(di->page);
 
-- 
2.14.1


^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-09-10  6:17 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-02 15:32 [PATCH net-next RFC v1 0/4] Add devlink traps in devlink port context Aya Levin
2020-09-02 15:32 ` [PATCH net-next RFC v1 1/4] devlink: Wrap trap related lists and ops in trap_mngr Aya Levin
2020-09-08 14:12   ` Jiri Pirko
2020-09-02 15:32 ` [PATCH net-next RFC v1 2/4] devlink: Add devlink traps under devlink_ports context Aya Levin
2020-09-06 15:44   ` Ido Schimmel
2020-09-07 16:52     ` Aya Levin
2020-09-08 14:04     ` Jiri Pirko
2020-09-10  6:16       ` Aya Levin
2020-09-02 15:32 ` [PATCH net-next RFC v1 3/4] devlink: Add hierarchy between traps in device level and port level Aya Levin
2020-09-06 15:58   ` Ido Schimmel
2020-09-07 16:26     ` Aya Levin
2020-09-02 15:32 ` [PATCH net-next RFC v1 4/4] net/mlx5e: Add devlink trap to catch oversize packets Aya Levin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).