[RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Oleksandr Mazur]

Whenever query statistics is issued for trap with DROP action,
devlink subsystem would also fill-in statistics 'dropped' field.
In case if device driver did't register callback for hard drop
statistics querying, 'dropped' field will be omitted and not filled.
Add trap_drop_counter_get callback implementation to the netdevsim.
Add new test cases for netdevsim, to test both the callback
functionality, as well as drop statistics alteration check.

Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>
---
V3:
    1) Mark subject as RFC instead of PATCH.
V2:
    1) Change commit description / subject.
    2) Remove HARD_DROP action.
    3) Remove devlink UAPI changes.
    4) Rename hard statistics get callback to be 'trap_drop_counter_get'
    5) Make callback get called for existing trap action - DROP:
       whenever statistics for trap with DROP action is queried,
       devlink subsystem would call-in callback to get stats from HW;
    6) Add changes to the netdevsim support implemented changes
       (as well as changes to make it possible to test netdevsim with
        these changes).
    7) Add new test cases to the netdevsim's kselftests to test new
       changes provided with this patchset;

Test-results:
# selftests: drivers/net/netdevsim: devlink_trap.sh
# TEST: Initialization                                                [ OK ]
# TEST: Trap action                                                   [ OK ]
# TEST: Trap metadata                                                 [ OK ]
# TEST: Non-existing trap                                             [ OK ]
# TEST: Non-existing trap action                                      [ OK ]
# TEST: Trap statistics                                               [ OK ]
# TEST: Trap group action                                             [ OK ]
# TEST: Non-existing trap group                                       [ OK ]
# TEST: Trap group statistics                                         [ OK ]
# TEST: Trap policer                                                  [ OK ]
# TEST: Trap policer binding                                          [ OK ]
# TEST: Port delete                                                   [ OK ]
# TEST: Device delete                                                 [ OK ]
ok 1 selftests: drivers/net/netdevsim: devlink_trap.sh


 drivers/net/netdevsim/dev.c                   | 21 +++++++
 drivers/net/netdevsim/netdevsim.h             |  1 +
 include/net/devlink.h                         | 10 ++++
 net/core/devlink.c                            | 55 +++++++++++++++++--
 .../drivers/net/netdevsim/devlink_trap.sh     | 10 ++++
 .../selftests/net/forwarding/devlink_lib.sh   | 26 +++++++++
 6 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 816af1f55e2c..1fc8c7a2a1e3 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -231,6 +231,9 @@ static int nsim_dev_debugfs_init(struct nsim_dev *nsim_dev)
 	debugfs_create_bool("fail_trap_policer_counter_get", 0600,
 			    nsim_dev->ddir,
 			    &nsim_dev->fail_trap_policer_counter_get);
+	debugfs_create_bool("fail_trap_drop_counter_get", 0600,
+			    nsim_dev->ddir,
+			    &nsim_dev->fail_trap_drop_counter_get);
 	nsim_udp_tunnels_debugfs_create(nsim_dev);
 	return 0;
 }
@@ -416,6 +419,7 @@ struct nsim_trap_data {
 	struct delayed_work trap_report_dw;
 	struct nsim_trap_item *trap_items_arr;
 	u64 *trap_policers_cnt_arr;
+	u64 trap_hard_drop_cnt;
 	struct nsim_dev *nsim_dev;
 	spinlock_t trap_lock;	/* Protects trap_items_arr */
 };
@@ -892,6 +896,22 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink *devlink,
 	return 0;
 }
 
+int nsim_dev_devlink_trap_drop_counter_get(struct devlink *devlink,
+					   const struct devlink_trap *trap,
+					   u64 *p_drops)
+{
+	struct nsim_dev *nsim_dev = devlink_priv(devlink);
+	u64 *cnt;
+
+	if (nsim_dev->fail_trap_drop_counter_get)
+		return -EINVAL;
+
+	cnt = &nsim_dev->trap_data->trap_hard_drop_cnt;
+	*p_drops = (*cnt)++;
+
+	return 0;
+}
+
 static const struct devlink_ops nsim_dev_devlink_ops = {
 	.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT |
 					 DEVLINK_SUPPORT_FLASH_UPDATE_OVERWRITE_MASK,
@@ -905,6 +925,7 @@ static const struct devlink_ops nsim_dev_devlink_ops = {
 	.trap_group_set = nsim_dev_devlink_trap_group_set,
 	.trap_policer_set = nsim_dev_devlink_trap_policer_set,
 	.trap_policer_counter_get = nsim_dev_devlink_trap_policer_counter_get,
+	.trap_drop_counter_get = nsim_dev_devlink_trap_drop_counter_get,
 };
 
 #define NSIM_DEV_MAX_MACS_DEFAULT 32
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 48163c5f2ec9..b0d8ec7d09a5 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -219,6 +219,7 @@ struct nsim_dev {
 	bool fail_trap_group_set;
 	bool fail_trap_policer_set;
 	bool fail_trap_policer_counter_get;
+	bool fail_trap_drop_counter_get;
 	struct {
 		struct udp_tunnel_nic_shared utn_shared;
 		u32 __ports[2][NSIM_UDP_TUNNEL_N_PORTS];
diff --git a/include/net/devlink.h b/include/net/devlink.h
index f466819cc477..0b9ed24533c5 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1294,6 +1294,16 @@ struct devlink_ops {
 				     const struct devlink_trap_group *group,
 				     enum devlink_trap_action action,
 				     struct netlink_ext_ack *extack);
+	/**
+	 * @trap_drop_counter_get: Trap drop counter get function.
+	 *
+	 * Should be used by device drivers to report number of packets
+	 * that have been dropped, and cannot be passed to the devlink
+	 * subsystem by the underlying device.
+	 */
+	int (*trap_drop_counter_get)(struct devlink *devlink,
+				     const struct devlink_trap *trap,
+				     u64 *p_drops);
 	/**
 	 * @trap_policer_init: Trap policer initialization function.
 	 *
diff --git a/net/core/devlink.c b/net/core/devlink.c
index ee828e4b1007..2bb129cdf722 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -6791,8 +6791,9 @@ static void devlink_trap_stats_read(struct devlink_stats __percpu *trap_stats,
 	}
 }
 
-static int devlink_trap_stats_put(struct sk_buff *msg,
-				  struct devlink_stats __percpu *trap_stats)
+static int
+devlink_trap_group_stats_put(struct sk_buff *msg,
+			     struct devlink_stats __percpu *trap_stats)
 {
 	struct devlink_stats stats;
 	struct nlattr *attr;
@@ -6820,6 +6821,52 @@ static int devlink_trap_stats_put(struct sk_buff *msg,
 	return -EMSGSIZE;
 }
 
+static int devlink_trap_stats_put(struct sk_buff *msg, struct devlink *devlink,
+				  const struct devlink_trap_item *trap_item)
+{
+	struct devlink_stats stats;
+	struct nlattr *attr;
+	u64 drops = 0;
+	int err;
+
+	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
+	    devlink->ops->trap_drop_counter_get) {
+		err = devlink->ops->trap_drop_counter_get(devlink,
+							  trap_item->trap,
+							  &drops);
+		if (err)
+			return err;
+	}
+
+	devlink_trap_stats_read(trap_item->stats, &stats);
+
+	attr = nla_nest_start(msg, DEVLINK_ATTR_STATS);
+	if (!attr)
+		return -EMSGSIZE;
+
+	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_DROPPED, drops,
+			      DEVLINK_ATTR_PAD))
+		goto nla_put_failure;
+
+	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
+	    devlink->ops->trap_drop_counter_get &&
+	    nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_PACKETS,
+			      stats.rx_packets, DEVLINK_ATTR_PAD))
+			goto nla_put_failure;
+
+	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_BYTES,
+			      stats.rx_bytes, DEVLINK_ATTR_PAD))
+		goto nla_put_failure;
+
+	nla_nest_end(msg, attr);
+
+	return 0;
+
+nla_put_failure:
+	nla_nest_cancel(msg, attr);
+	return -EMSGSIZE;
+}
+
 static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink,
 				const struct devlink_trap_item *trap_item,
 				enum devlink_command cmd, u32 portid, u32 seq,
@@ -6857,7 +6904,7 @@ static int devlink_nl_trap_fill(struct sk_buff *msg, struct devlink *devlink,
 	if (err)
 		goto nla_put_failure;
 
-	err = devlink_trap_stats_put(msg, trap_item->stats);
+	err = devlink_trap_stats_put(msg, devlink, trap_item);
 	if (err)
 		goto nla_put_failure;
 
@@ -7074,7 +7121,7 @@ devlink_nl_trap_group_fill(struct sk_buff *msg, struct devlink *devlink,
 			group_item->policer_item->policer->id))
 		goto nla_put_failure;
 
-	err = devlink_trap_stats_put(msg, group_item->stats);
+	err = devlink_trap_group_stats_put(msg, group_item->stats);
 	if (err)
 		goto nla_put_failure;
 
diff --git a/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh b/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
index da49ad2761b5..ff4f3617e0c5 100755
--- a/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/devlink_trap.sh
@@ -163,6 +163,16 @@ trap_stats_test()
 			devlink_trap_action_set $trap_name "drop"
 			devlink_trap_stats_idle_test $trap_name
 			check_err $? "Stats of trap $trap_name not idle when action is drop"
+
+			echo "y"> $DEBUGFS_DIR/fail_trap_drop_counter_get
+			devlink -s trap show $DEVLINK_DEV trap $trap_name &> /dev/null
+			check_fail $? "Managed to read trap (hard dropped) statistics when should not"
+			echo "n"> $DEBUGFS_DIR/fail_trap_drop_counter_get
+			devlink -s trap show $DEVLINK_DEV trap $trap_name &> /dev/null
+			check_err $? "Did not manage to read trap (hard dropped) statistics when should"
+
+			devlink_trap_drop_stats_idle_test $trap_name
+			check_fail $? "Drop stats of trap $trap_name idle when should not"
 		else
 			devlink_trap_stats_idle_test $trap_name
 			check_fail $? "Stats of non-drop trap $trap_name idle when should not"
diff --git a/tools/testing/selftests/net/forwarding/devlink_lib.sh b/tools/testing/selftests/net/forwarding/devlink_lib.sh
index 9c12c4fd3afc..2094ba025af5 100644
--- a/tools/testing/selftests/net/forwarding/devlink_lib.sh
+++ b/tools/testing/selftests/net/forwarding/devlink_lib.sh
@@ -318,6 +318,14 @@ devlink_trap_rx_bytes_get()
 		| jq '.[][][]["stats"]["rx"]["bytes"]'
 }
 
+devlink_trap_drop_packets_get()
+{
+	local trap_name=$1; shift
+
+	devlink -js trap show $DEVLINK_DEV trap $trap_name \
+		| jq '.[][][]["stats"]["rx"]["dropped"]'
+}
+
 devlink_trap_stats_idle_test()
 {
 	local trap_name=$1; shift
@@ -339,6 +347,24 @@ devlink_trap_stats_idle_test()
 	fi
 }
 
+devlink_trap_drop_stats_idle_test()
+{
+	local trap_name=$1; shift
+	local t0_packets t0_bytes
+
+	t0_packets=$(devlink_trap_drop_packets_get $trap_name)
+
+	sleep 1
+
+	t1_packets=$(devlink_trap_drop_packets_get $trap_name)
+
+	if [[ $t0_packets -eq $t1_packets ]]; then
+		return 0
+	else
+		return 1
+	fi
+}
+
 devlink_traps_enable_all()
 {
 	local trap_name
-- 
2.17.1

Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Jakub Kicinski]

On Mon, 25 Jan 2021 14:38:56 +0200 Oleksandr Mazur wrote:
> +	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +	    devlink->ops->trap_drop_counter_get) {
> +		err = devlink->ops->trap_drop_counter_get(devlink,
> +							  trap_item->trap,
> +							  &drops);
> +		if (err)
> +			return err;
> +	}

Why only report this counter when action is set to drop?

Thinking about it again - if the action can be changed wouldn't it 
be best for the user to actually get a "HW condition hit" counter,
which would increment regardless of SW config (incl. policers)?

Otherwise if admin logs onto the box and temporarily enables a trap 
for debug this count would disappear.

Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Oleksandr Mazur]

On Mon, 25 Jan 2021 14:38:56 +0200 Oleksandr Mazur wrote:
> +     if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +         devlink->ops->trap_drop_counter_get) {
> +             err = devlink->ops->trap_drop_counter_get(devlink,
> +                                                       trap_item->trap,
> +                                                       &drops);
> +             if (err)
> +                     return err;
> +     }

> Why only report this counter when action is set to drop?

> Thinking about it again - if the action can be changed wouldn't it 
> be best for the user to actually get a "HW condition hit" counter,
> which would increment regardless of SW config (incl. policers)?

> Otherwise if admin logs onto the box and temporarily enables a trap 
> for debug this count would disappear.

Okay, so should it become like generic HW counter - trap_hw_counter_get?

Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Jakub Kicinski]

On Fri, 29 Jan 2021 11:15:43 +0000 Oleksandr Mazur wrote:
> > Thinking about it again - if the action can be changed wouldn't it 
> > be best for the user to actually get a "HW condition hit" counter,
> > which would increment regardless of SW config (incl. policers)?  
> 
> > Otherwise if admin logs onto the box and temporarily enables a trap 
> > for debug this count would disappear.  
> 
> But still this counter makes sense only for 'drop' action.

Okay, well, "dropped while trap was disabled" seems a lot less useful
of a definition than "number of times this trap would trigger" but if
that's all the HW can provide then it is what it is.

Does the HW also count packets dropped because of overload / overflow
or some other event, or purely dropped because disabled?

Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Oleksandr Mazur]

On Fri, 29 Jan 2021 11:15:43 +0000 Oleksandr Mazur wrote:
> > >Thinking about it again - if the action can be changed wouldn't it 
> > >be best for the user to actually get a "HW condition hit" counter,
> >> which would increment regardless of SW config (incl. policers)?  
> >
> > >Otherwise if admin logs onto the box and temporarily enables a trap 
> >> for debug this count would disappear.  
>> 
>> But still this counter makes sense only for 'drop' action.

>Okay, well, "dropped while trap was disabled" seems a lot less useful
>of a definition than "number of times this trap would trigger" but if
>that's all the HW can provide then it is what it is.

>Does the HW also count packets dropped because of overload / overflow
>or some other event, or purely dropped because disabled?

Hw starts counting traffic (hw drops) only when action has been explicitly set to be 'DROP';

Re: [RFC v3 net-next] net: core: devlink: add 'dropped' stats field for DROP trap action

[Ido Schimmel]

I missed this patch. Please Cc me on future versions given I commented
on previous versions.

On Mon, Jan 25, 2021 at 02:38:56PM +0200, Oleksandr Mazur wrote:
> Whenever query statistics is issued for trap with DROP action,
> devlink subsystem would also fill-in statistics 'dropped' field.
> In case if device driver did't register callback for hard drop
> statistics querying, 'dropped' field will be omitted and not filled.
> Add trap_drop_counter_get callback implementation to the netdevsim.
> Add new test cases for netdevsim, to test both the callback
> functionality, as well as drop statistics alteration check.
> 
> Signed-off-by: Oleksandr Mazur <oleksandr.mazur@plvision.eu>

[...]

> +static int devlink_trap_stats_put(struct sk_buff *msg, struct devlink *devlink,
> +				  const struct devlink_trap_item *trap_item)
> +{
> +	struct devlink_stats stats;
> +	struct nlattr *attr;
> +	u64 drops = 0;
> +	int err;
> +
> +	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +	    devlink->ops->trap_drop_counter_get) {
> +		err = devlink->ops->trap_drop_counter_get(devlink,
> +							  trap_item->trap,
> +							  &drops);
> +		if (err)
> +			return err;
> +	}
> +
> +	devlink_trap_stats_read(trap_item->stats, &stats);
> +
> +	attr = nla_nest_start(msg, DEVLINK_ATTR_STATS);
> +	if (!attr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_DROPPED, drops,
> +			      DEVLINK_ATTR_PAD))

Commit message says: "In case if device driver did't register callback
for hard drop statistics querying, 'dropped' field will be omitted and
not filled."

But looks like this attribute is always reported to user space.

> +		goto nla_put_failure;
> +
> +	if (trap_item->action == DEVLINK_TRAP_ACTION_DROP &&
> +	    devlink->ops->trap_drop_counter_get &&
> +	    nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_PACKETS,
> +			      stats.rx_packets, DEVLINK_ATTR_PAD))

This is needed for DEVLINK_ATTR_STATS_RX_DROPPED, not for
DEVLINK_ATTR_STATS_RX_PACKETS.

I don't think it makes sense for a counter to come and go based on the
action. It should always be reported (if device supports it), regardless
of current action. Note that the first check will result in this counter
being reported as zero when the action is not drop, but as non-zero
otherwise. That's not good because the basic property of a counter is
that it is monotonically increasing.

> +			goto nla_put_failure;
> +
> +	if (nla_put_u64_64bit(msg, DEVLINK_ATTR_STATS_RX_BYTES,
> +			      stats.rx_bytes, DEVLINK_ATTR_PAD))
> +		goto nla_put_failure;
> +
> +	nla_nest_end(msg, attr);
> +
> +	return 0;
> +
> +nla_put_failure:
> +	nla_nest_cancel(msg, attr);
> +	return -EMSGSIZE;
> +}