Re: [RFC v5 net-next] net: core: devlink: add 'dropped' stats field for traps

2021-02-06 Thread Ido Schimmel
On Thu, Feb 04, 2021 at 01:41:22PM +0200, Oleksandr Mazur wrote:
> Whenever query statistics is issued for trap, devlink subsystem
> would also fill-in statistics 'dropped' field. This field indicates
> the number of packets HW dropped and failed to report to the device driver,
> and thus - to the devlink subsystem itself.
> In case if device driver didn'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 

Code looks fine to me, but for non-RFC submission (with actual hw
implementation), you probably want to split it into three patches:
devlink, netdevsim, selftest.


[RFC v5 net-next] net: core: devlink: add 'dropped' stats field for traps

2021-02-04 Thread Oleksandr Mazur
Whenever query statistics is issued for trap, devlink subsystem
would also fill-in statistics 'dropped' field. This field indicates
the number of packets HW dropped and failed to report to the device driver,
and thus - to the devlink subsystem itself.
In case if device driver didn'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 
---
V5:
1) Add missed static specifier for function in netdevsim/dev.c
V4:
1) Change commit description / subject.
2) Change 'dropped' statistics fill condition from
   'trap_drop_counter_get is registered' && 'trap action == drop'
   to
   'trap_drop_counter_get is registered'.
3) Fix statistics fill condition used for wrong stats attribute:
   DEVLINK_ATTR_STATS_RX_PACKETS -> DEVLINK_ATTR_STATS_RX_DROPPED
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   | 22 
 drivers/net/netdevsim/netdevsim.h |  1 +
 include/net/devlink.h | 10 
 net/core/devlink.c| 53 +--
 .../drivers/net/netdevsim/devlink_trap.sh | 10 
 .../selftests/net/forwarding/devlink_lib.sh   | 26 +
 6 files changed, 118 insertions(+), 4 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index 816af1f55e2c..d83a53bd0930 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,
_dev->fail_trap_policer_counter_get);
+   debugfs_create_bool("fail_trap_counter_get", 0600,
+   nsim_dev->ddir,
+   _dev->fail_trap_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_pkt_cnt;
struct nsim_dev *nsim_dev;
spinlock_t trap_lock;   /* Protects trap_items_arr */
 };
@@ -892,6 +896,23 @@ nsim_dev_devlink_trap_policer_counter_get(struct devlink 
*devlink,
return 0;
 }
 
+static int
+nsim_dev_devlink_trap_hw_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_counter_get)
+   return -EINVAL;
+
+   cnt = _dev->trap_data->trap_pkt_cnt;
+   *p_drops = (*cnt)++;
+
+   return 0;
+}
+
 static const struct devlink_ops nsim_dev_devlink_ops = {
.supported_flash_update_params = DEVLINK_SUPPORT_FLASH_UPDATE_COMPONENT 
|