| Message ID | 20230203135349.547933-1-horatiu.vultur@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: lan966x: Add support for TC flower filter statistics | expand |
On Fri, Feb 03, 2023 at 02:53:49PM +0100, Horatiu Vultur wrote: > Add flower filter packet statistics. This will just read the TCAM > counter of the rule, which mention how many packages were hit by this > rule. I am curious to know how HW stats only updating the packet count interacts with SW stats also incrementing other values, such as the byte count. > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > --- > .../microchip/lan966x/lan966x_tc_flower.c | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > index 88c655d6318fa..aac3d7c87f1d5 100644 > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > @@ -234,6 +234,26 @@ static int lan966x_tc_flower_del(struct lan966x_port *port, > return err; > } > > +static int lan966x_tc_flower_stats(struct lan966x_port *port, > + struct flow_cls_offload *f, > + struct vcap_admin *admin) > +{ > + struct vcap_counter count; > + int err; > + > + memset(&count, 0, sizeof(count)); nit: As was pointed out to me recently it's simpler to declare count as follows and skip the memset entirely. No need to respin for this! struct vcap_counter count = {}; > + > + err = vcap_get_rule_count_by_cookie(port->lan966x->vcap_ctrl, > + &count, f->cookie); > + if (err) > + return err; > + > + flow_stats_update(&f->stats, 0x0, count.value, 0, 0, > + FLOW_ACTION_HW_STATS_IMMEDIATE); > + > + return err; > +} > + > int lan966x_tc_flower(struct lan966x_port *port, > struct flow_cls_offload *f, > bool ingress) > @@ -252,6 +272,8 @@ int lan966x_tc_flower(struct lan966x_port *port, > return lan966x_tc_flower_add(port, f, admin, ingress); > case FLOW_CLS_DESTROY: > return lan966x_tc_flower_del(port, f, admin); > + case FLOW_CLS_STATS: > + return lan966x_tc_flower_stats(port, f, admin); > default: > return -EOPNOTSUPP; > } > -- Also, not strictly related, but could you consider, as a favour to reviewers, fixing the driver so that the following doesn't fail: $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o DESCEND objtool CALL scripts/checksyscalls.sh CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o In file included from drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c:3: drivers/net/ethernet/microchip/lan966x/lan966x_main.h:18:10: fatal error: vcap_api.h: No such file or directory 18 | #include <vcap_api.h> | ^~~~~~~~~~~~ compilation terminated.
The 02/04/2023 18:12, Simon Horman wrote: > > On Fri, Feb 03, 2023 at 02:53:49PM +0100, Horatiu Vultur wrote: > > Add flower filter packet statistics. This will just read the TCAM > > counter of the rule, which mention how many packages were hit by this > > rule. > > I am curious to know how HW stats only updating the packet count > interacts with SW stats also incrementing other values, such as the byte > count. First, our HW can count only the packages and not also the bytes, unfortunately. Also we use the flag 'skip_sw' when we add the rules in this case the statistics look OK. If the user doesn't use the skip_sw then the statistics will look something like this (using command: tc -s filter show dev eth0 ingress): Action statistics: Sent 92 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) Sent software 92 bytes 2 pkt Sent hardware 0 bytes 2 pkt backlog 0b 0p requeues 0 used_hw_stats immediate As you see there are different counters for SW and Hw statistics. > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> > > --- > > .../microchip/lan966x/lan966x_tc_flower.c | 22 +++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > > index 88c655d6318fa..aac3d7c87f1d5 100644 > > --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > > +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c > > @@ -234,6 +234,26 @@ static int lan966x_tc_flower_del(struct lan966x_port *port, > > return err; > > } > > > > +static int lan966x_tc_flower_stats(struct lan966x_port *port, > > + struct flow_cls_offload *f, > > + struct vcap_admin *admin) > > +{ > > + struct vcap_counter count; > > + int err; > > + > > + memset(&count, 0, sizeof(count)); > > nit: As was pointed out to me recently it's simpler to declare > count as follows and skip the memset entirely. > No need to respin for this! > > struct vcap_counter count = {}; Good to know this. > > > + > > + err = vcap_get_rule_count_by_cookie(port->lan966x->vcap_ctrl, > > + &count, f->cookie); > > + if (err) > > + return err; > > + > > + flow_stats_update(&f->stats, 0x0, count.value, 0, 0, > > + FLOW_ACTION_HW_STATS_IMMEDIATE); > > + > > + return err; > > +} > > + > > int lan966x_tc_flower(struct lan966x_port *port, > > struct flow_cls_offload *f, > > bool ingress) > > @@ -252,6 +272,8 @@ int lan966x_tc_flower(struct lan966x_port *port, > > return lan966x_tc_flower_add(port, f, admin, ingress); > > case FLOW_CLS_DESTROY: > > return lan966x_tc_flower_del(port, f, admin); > > + case FLOW_CLS_STATS: > > + return lan966x_tc_flower_stats(port, f, admin); > > default: > > return -EOPNOTSUPP; > > } > > -- > > Also, not strictly related, but could you consider, as a favour to > reviewers, fixing the driver so that the following doesn't fail: > > $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > DESCEND objtool > CALL scripts/checksyscalls.sh > CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > In file included from drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c:3: > drivers/net/ethernet/microchip/lan966x/lan966x_main.h:18:10: fatal error: vcap_api.h: No such file or directory > 18 | #include <vcap_api.h> > | ^~~~~~~~~~~~ > compilation terminated. I will try to have a look at this.
On Mon, Feb 06, 2023 at 10:52:27AM +0100, Horatiu Vultur wrote: > The 02/04/2023 18:12, Simon Horman wrote: > > > > On Fri, Feb 03, 2023 at 02:53:49PM +0100, Horatiu Vultur wrote: > > > Add flower filter packet statistics. This will just read the TCAM > > > counter of the rule, which mention how many packages were hit by this > > > rule. > > > > I am curious to know how HW stats only updating the packet count > > interacts with SW stats also incrementing other values, such as the byte > > count. > > First, our HW can count only the packages and not also the bytes, > unfortunately. Also we use the flag 'skip_sw' when we add the rules in > this case the statistics look OK. > If the user doesn't use the skip_sw then the statistics will look > something like this (using command: tc -s filter show dev eth0 ingress): > > Action statistics: > Sent 92 bytes 4 pkt (dropped 0, overlimits 0 requeues 0) > Sent software 92 bytes 2 pkt > Sent hardware 0 bytes 2 pkt > backlog 0b 0p requeues 0 > used_hw_stats immediate > > As you see there are different counters for SW and Hw statistics. Thanks, that answers my question. I appreciate that hw has limitations, and this does seem to be an appropriate solution in this case. > > > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> Reviewed-by: Simon Horman <simon.horman@corigine.com> ... > > Also, not strictly related, but could you consider, as a favour to > > reviewers, fixing the driver so that the following doesn't fail: > > > > $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > DESCEND objtool > > CALL scripts/checksyscalls.sh > > CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > In file included from drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c:3: > > drivers/net/ethernet/microchip/lan966x/lan966x_main.h:18:10: fatal error: vcap_api.h: No such file or directory > > 18 | #include <vcap_api.h> > > | ^~~~~~~~~~~~ > > compilation terminated. > > I will try to have a look at this. Thanks, much appreciated.
The 02/06/2023 10:59, Simon Horman wrote: > > > > Also, not strictly related, but could you consider, as a favour to > > > reviewers, fixing the driver so that the following doesn't fail: > > > > > > $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > > DESCEND objtool > > > CALL scripts/checksyscalls.sh > > > CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > > In file included from drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c:3: > > > drivers/net/ethernet/microchip/lan966x/lan966x_main.h:18:10: fatal error: vcap_api.h: No such file or directory > > > 18 | #include <vcap_api.h> > > > | ^~~~~~~~~~~~ > > > compilation terminated. > > > > I will try to have a look at this. > > Thanks, much appreciated. Sorry for coming back to this, but it seems that I can't reproduce the issue: $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o CALL scripts/checksyscalls.sh DESCEND objtool CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o I have tried different configurations without any luck. Any suggestion on how to reproduce the issue?
On Mon, Feb 06, 2023 at 11:32:52AM +0100, Horatiu Vultur wrote: > The 02/06/2023 10:59, Simon Horman wrote: > > > > > > Also, not strictly related, but could you consider, as a favour to > > > > reviewers, fixing the driver so that the following doesn't fail: > > > > > > > > $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > > > DESCEND objtool > > > > CALL scripts/checksyscalls.sh > > > > CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > > > In file included from drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c:3: > > > > drivers/net/ethernet/microchip/lan966x/lan966x_main.h:18:10: fatal error: vcap_api.h: No such file or directory > > > > 18 | #include <vcap_api.h> > > > > | ^~~~~~~~~~~~ > > > > compilation terminated. > > > > > > I will try to have a look at this. > > > > Thanks, much appreciated. > > Sorry for coming back to this, but it seems that I can't reproduce the > issue: > > $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > CALL scripts/checksyscalls.sh > DESCEND objtool > CC drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o > > I have tried different configurations without any luck. Any suggestion > on how to reproduce the issue? Sorry, perhaps it is something on my side. I tried running through the steps below, which is what I assume I had done last time, but I don't see the problem any more. I'll let you know if it shows up again. But in the meantime sorry for the false reporting. $ git checkout net-next/main $ make allmodconfig $ make prepare $ make drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.o
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c index 88c655d6318fa..aac3d7c87f1d5 100644 --- a/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c +++ b/drivers/net/ethernet/microchip/lan966x/lan966x_tc_flower.c @@ -234,6 +234,26 @@ static int lan966x_tc_flower_del(struct lan966x_port *port, return err; } +static int lan966x_tc_flower_stats(struct lan966x_port *port, + struct flow_cls_offload *f, + struct vcap_admin *admin) +{ + struct vcap_counter count; + int err; + + memset(&count, 0, sizeof(count)); + + err = vcap_get_rule_count_by_cookie(port->lan966x->vcap_ctrl, + &count, f->cookie); + if (err) + return err; + + flow_stats_update(&f->stats, 0x0, count.value, 0, 0, + FLOW_ACTION_HW_STATS_IMMEDIATE); + + return err; +} + int lan966x_tc_flower(struct lan966x_port *port, struct flow_cls_offload *f, bool ingress) @@ -252,6 +272,8 @@ int lan966x_tc_flower(struct lan966x_port *port, return lan966x_tc_flower_add(port, f, admin, ingress); case FLOW_CLS_DESTROY: return lan966x_tc_flower_del(port, f, admin); + case FLOW_CLS_STATS: + return lan966x_tc_flower_stats(port, f, admin); default: return -EOPNOTSUPP; }
Add flower filter packet statistics. This will just read the TCAM counter of the rule, which mention how many packages were hit by this rule. Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com> --- .../microchip/lan966x/lan966x_tc_flower.c | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+)