From: Jiri Pirko <redacted>
Currently, when user adds a TC action and the action gets offloaded,
the user expects the HW stats to be counted and included in stats dump.
However, since drivers may implement different types of counting, there
is no way to specify which one the user is interested in.
For example for mlx5, only delayed counters are available as the driver
periodically polls for updated stats.
In case of mlxsw, the counters are queried on dump time. However, the
HW resources for this type of counters is quite limited (couple of
thousands). This limits the amount of supported offloaded filters
significantly. Without counter assigned, the HW is capable to carry
millions of those.
On top of that, mlxsw HW is able to support delayed counters as well in
greater numbers. That is going to be added in a follow-up patch.
This patchset allows user to specify one of the following types of HW
stats for added action:
any - current default, user does not care about the type, just expects
any type of stats.
immediate - queried during dump time
delayed - polled from HW periodically or sent by HW in async manner
disabled - no stats needed
Examples:
$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats disabled
$ tc -s filter show dev enp0s16np28 ingress
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 192.168.1.1
skip_sw
in_hw in_hw_count 2
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1 installed 13 sec used 4 sec
Action statistics:
Sent 1164 bytes 588185936 pkt (dropped 588185936, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 1164 bytes 588185936 pkt
backlog 0b 0p requeues 0
hw_stats disabled
$ tc filter add dev enp0s16np28 ingress proto ip handle 1 pref 1 flower skip_sw dst_ip 192.168.1.1 action drop hw_stats immediate
$ tc -s filter show dev enp0s16np28 ingress
filter protocol ip pref 1 flower chain 0
filter protocol ip pref 1 flower chain 0 handle 0x1
eth_type ipv4
dst_ip 192.168.1.1
skip_sw
in_hw in_hw_count 2
action order 1: gact action drop
random type none pass val 0
index 1 ref 1 bind 1 installed 11 sec used 4 sec
Action statistics:
Sent 102 bytes 1 pkt (dropped 1, overlimits 0 requeues 0)
Sent software 0 bytes 0 pkt
Sent hardware 102 bytes 1 pkt
backlog 0b 0p requeues 0
hw_stats immediate
Jiri Pirko (12):
flow_offload: Introduce offload of HW stats type
ocelot_flower: use flow_offload_has_one_action() helper
flow_offload: check for basic action hw stats type
mlx5: en_tc: Do not allow mixing HW stats types for actions
mlxsw: spectrum_flower: Do not allow mixing HW stats types for actions
mlx5: restrict supported HW stats type to "any"
mlxsw: restrict supported HW stats type to "any"
flow_offload: introduce "immediate" HW stats type and allow it in
mlxsw
flow_offload: introduce "delayed" HW stats type and allow it in mlx5
mlxsw: spectrum_acl: Ask device for rule stats only if counter was
created
flow_offload: introduce "disabled" HW stats type and allow it in mlxsw
sched: act: allow user to specify type of HW stats for a filter
drivers/net/ethernet/broadcom/bnxt/bnxt_tc.c | 9 +++-
.../ethernet/chelsio/cxgb4/cxgb4_tc_flower.c | 8 +++-
.../ethernet/chelsio/cxgb4/cxgb4_tc_flower.h | 3 +-
.../chelsio/cxgb4/cxgb4_tc_matchall.c | 3 +-
.../net/ethernet/marvell/mvpp2/mvpp2_cls.c | 6 +++
.../net/ethernet/mellanox/mlx5/core/en_tc.c | 23 ++++++++++
.../net/ethernet/mellanox/mlxsw/spectrum.h | 3 +-
.../ethernet/mellanox/mlxsw/spectrum_acl.c | 25 ++++++----
.../ethernet/mellanox/mlxsw/spectrum_flower.c | 21 +++++++--
drivers/net/ethernet/mscc/ocelot_flower.c | 6 ++-
.../ethernet/netronome/nfp/flower/action.c | 4 ++
.../net/ethernet/qlogic/qede/qede_filter.c | 10 ++--
.../net/ethernet/stmicro/stmmac/stmmac_tc.c | 9 +++-
include/net/act_api.h | 1 +
include/net/flow_offload.h | 46 +++++++++++++++++++
include/uapi/linux/pkt_cls.h | 26 +++++++++++
net/dsa/slave.c | 4 ++
net/sched/act_api.c | 21 +++++++++
net/sched/cls_api.c | 26 +++++++++++
19 files changed, 229 insertions(+), 25 deletions(-)
--
2.21.1
From: Jiri Pirko <redacted>
Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to actions
- add mixed bool
---
include/net/flow_offload.h | 6 ++++++
1 file changed, 6 insertions(+)
From: Jiri Pirko <redacted>
As there is one set of counters for the whole action chain, forbid to
mix the HW stats types.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- new patch
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 ++
1 file changed, 2 insertions(+)
@@ -26,6 +26,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,if(!flow_action_has_entries(flow_action))return0;+if(!flow_action_mixed_hw_stats_types_check(flow_action,extack))+return-EOPNOTSUPP;/* Count action is inserted first */err=mlxsw_sp_acl_rulei_act_count(mlxsw_sp,rulei,extack);
From: Jiri Pirko <redacted>
As there is one set of counters for the whole action chain, forbid to
mix the HW stats types.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- new patch
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 6 ++++++
1 file changed, 6 insertions(+)
@@ -279,7 +279,8 @@ bnxt_tc_parse_pedit(struct bnxt *bp, struct bnxt_tc_actions *actions,staticintbnxt_tc_parse_actions(structbnxt*bp,structbnxt_tc_actions*actions,-structflow_action*flow_action)+structflow_action*flow_action,+structnetlink_ext_ack*extack){/* Used to store the L2 rewrite mask for dmac (6 bytes) followed by*smac(6bytes)ifrewriteofbothisspecified,otherwiseeither
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp,return-EINVAL;}+if(!flow_action_basic_hw_stats_types_check(flow_action,extack))+return-EOPNOTSUPP;+flow_action_for_each(i,act,flow_action){switch(act->id){caseFLOW_ACTION_DROP:
@@ -491,7 +495,8 @@ static int bnxt_tc_parse_flow(struct bnxt *bp,flow->tun_mask.tp_src=match.mask->src;}-returnbnxt_tc_parse_actions(bp,&flow->actions,&rule->action);+returnbnxt_tc_parse_actions(bp,&flow->actions,&rule->action,+tc_flow_cmd->common.extack);}staticintbnxt_hwrm_cfa_flow_free(structbnxt*bp,
@@ -1746,7 +1746,8 @@ int qede_get_arfs_filter_count(struct qede_dev *edev)}staticintqede_parse_actions(structqede_dev*edev,-structflow_action*flow_action)+structflow_action*flow_action,+structnetlink_ext_ack*extack){conststructflow_action_entry*act;inti;
@@ -1756,6 +1757,9 @@ static int qede_parse_actions(struct qede_dev *edev,return-EINVAL;}+if(!flow_action_basic_hw_stats_types_check(flow_action,extack))+return-EOPNOTSUPP;+flow_action_for_each(i,act,flow_action){switch(act->id){caseFLOW_ACTION_DROP:
@@ -1970,7 +1974,7 @@ int qede_add_tc_flower_fltr(struct qede_dev *edev, __be16 proto,}/* parse tc actions and get the vf_id */-if(qede_parse_actions(edev,&f->rule->action))+if(qede_parse_actions(edev,&f->rule->action,f->common.extack))gotounlock;if(qede_flow_find_fltr(edev,&t)){
@@ -2038,7 +2042,7 @@ static int qede_flow_spec_validate(struct qede_dev *edev,return-EINVAL;}-if(qede_parse_actions(edev,flow_action))+if(qede_parse_actions(edev,flow_action,NULL))return-EINVAL;return0;
From: Jiri Pirko <redacted>
Currently don't allow actions with any other type to be inserted.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- move the code to the first action processing
---
.../ethernet/mellanox/mlxsw/spectrum_flower.c | 16 ++++++++++++----
1 file changed, 12 insertions(+), 4 deletions(-)
@@ -29,10 +29,18 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp,if(!flow_action_mixed_hw_stats_types_check(flow_action,extack))return-EOPNOTSUPP;-/* Count action is inserted first */-err=mlxsw_sp_acl_rulei_act_count(mlxsw_sp,rulei,extack);-if(err)-returnerr;+act=flow_action_first_entry_get(flow_action);+switch(act->hw_stats_type){+caseFLOW_ACTION_HW_STATS_TYPE_ANY:+/* Count action is inserted first */+err=mlxsw_sp_acl_rulei_act_count(mlxsw_sp,rulei,extack);+if(err)+returnerr;+break;+default:+NL_SET_ERR_MSG_MOD(extack,"Unsupported action HW stats type");+return-EOPNOTSUPP;+}flow_action_for_each(i,act,flow_action){switch(act->id){
From: Jiri Pirko <redacted>
Currently don't allow action with any other type than "any"
to be inserted.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved the check to action
---
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
From: Jiri Pirko <redacted>
Introduce new type for disabled HW stats and allow the value in
mlxsw offload.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to action
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 2 ++
include/net/flow_offload.h | 1 +
2 files changed, 3 insertions(+)
From: Jiri Pirko <redacted>
Currently, user who is adding an action expects HW to report stats,
however it does not have exact expectations about the stats types.
That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.
Allow user to specify the type of HW stats for an action and require it.
Pass the information down to flow_offload layer.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved the stats attr from cls_flower (filter) to any action
- rebased on top of cookie offload changes
- adjusted the patch description a bit
---
include/net/act_api.h | 1 +
include/uapi/linux/pkt_cls.h | 26 ++++++++++++++++++++++++++
net/sched/act_api.c | 21 +++++++++++++++++++++
net/sched/cls_api.c | 26 ++++++++++++++++++++++++++
4 files changed, 74 insertions(+)
@@ -118,6 +119,31 @@ enum tca_id {#define TCA_ID_MAX __TCA_ID_MAX+/* tca HW stats type */+enumtca_act_hw_stats_type{+TCA_ACT_HW_STATS_TYPE_ANY,/* User does not care, it's default+*whenuserdoesnotpasstheattr.+*Instructsthedriverthatuserdoesnot+*careiftheHWstatsare"immediate"+*or"delayed".+*/+TCA_ACT_HW_STATS_TYPE_IMMEDIATE,/* Means that in dump, user gets+*thecurrentHWstatsstatefrom+*thedevicequeriedatthedumptime.+*/+TCA_ACT_HW_STATS_TYPE_DELAYED,/* Means that in dump, user gets+*HWstatsthatmightbeoutofdate+*forsometime,maybecoupleof+*seconds.Thisisthecasewhendriver+*pollsstatsupdatesperiodically+*orwhenitgetsasyncstatsupdate+*fromthedevice.+*/+TCA_ACT_HW_STATS_TYPE_DISABLED,/* User is not interested in getting+*anyHWstatistics.+*/+};+structtc_police{__u32index;intaction;
@@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref)}rcu_read_unlock();+if(nla_put_u8(skb,TCA_ACT_HW_STATS_TYPE,a->hw_stats_type))+gotonla_put_failure;+if(a->tcfa_flags){structnla_bitfield32flags={a->tcfa_flags,a->tcfa_flags,};
@@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb)returnc;}+staticinlineenumtca_act_hw_stats_type+tcf_action_hw_stats_type_get(structnlattr*hw_stats_type_attr)+{+/* If the user did not pass the attr, that means he does+*notcareaboutthetype.Return"any"inthatcase.+*/+returnhw_stats_type_attr?nla_get_u8(hw_stats_type_attr):+TCA_ACT_HW_STATS_TYPE_ANY;+}+staticconstu32tca_act_flags_allowed=TCA_ACT_FLAGS_NO_PERCPU_STATS;staticconststructnla_policytcf_action_policy[TCA_ACT_MAX+1]={[TCA_ACT_KIND]={.type=NLA_STRING},[TCA_ACT_INDEX]={.type=NLA_U32},[TCA_ACT_COOKIE]={.type=NLA_BINARY,.len=TC_COOKIE_MAX_SIZE},+[TCA_ACT_HW_STATS_TYPE]={.type=NLA_U8},[TCA_ACT_OPTIONS]={.type=NLA_NESTED},[TCA_ACT_FLAGS]={.type=NLA_BITFIELD32,.validation_data=&tca_act_flags_allowed},
@@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp,if(!name&&tb[TCA_ACT_COOKIE])tcf_set_action_cookie(&a->act_cookie,cookie);+if(!name)+a->hw_stats_type=hw_stats_type;+/* module count goes up only when brand new policy is created*ifitexistsandisonlyboundtoina_o->init()then*ACT_P_CREATEDisnotreturned(azerois).
From: Jiri Pirko <redacted>
Set a flag in case rule counter was created. Only query the device for
stats of a rule, which has the valid counter assigned.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- new patch
---
.../net/ethernet/mellanox/mlxsw/spectrum.h | 3 ++-
.../ethernet/mellanox/mlxsw/spectrum_acl.c | 25 +++++++++++++------
2 files changed, 19 insertions(+), 9 deletions(-)
From: Jiri Pirko <redacted>
Introduce new type for immediate HW stats and allow the value in
mlxsw offload.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to action
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_flower.c | 3 ++-
include/net/flow_offload.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-02-28 19:41:00
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted hunk
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
At some point we should come up with a way to express the limitations
at callback registration time so we don't need to add checks like this
to all the drivers. On the TODO list it goes :)
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
case FLOW_ACTION_DROP:
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-02-28 19:59:28
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
From: Jiri Pirko <redacted>
Currently, user who is adding an action expects HW to report stats,
however it does not have exact expectations about the stats types.
That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.
Allow user to specify the type of HW stats for an action and require it.
Pass the information down to flow_offload layer.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved the stats attr from cls_flower (filter) to any action
- rebased on top of cookie offload changes
- adjusted the patch description a bit
Thanks, this looks good... I mean I wish we could just share actions
instead but this set is less objectionable than v1 :)
@@ -118,6 +119,31 @@ enum tca_id {#define TCA_ID_MAX __TCA_ID_MAX+/* tca HW stats type */+enumtca_act_hw_stats_type{+TCA_ACT_HW_STATS_TYPE_ANY,/* User does not care, it's default+*whenuserdoesnotpasstheattr.+*Instructsthedriverthatuserdoesnot+*careiftheHWstatsare"immediate"+*or"delayed".+*/+TCA_ACT_HW_STATS_TYPE_IMMEDIATE,/* Means that in dump, user gets+*thecurrentHWstatsstatefrom+*thedevicequeriedatthedumptime.+*/+TCA_ACT_HW_STATS_TYPE_DELAYED,/* Means that in dump, user gets+*HWstatsthatmightbeoutofdate+*forsometime,maybecoupleof+*seconds.Thisisthecasewhendriver+*pollsstatsupdatesperiodically+*orwhenitgetsasyncstatsupdate+*fromthedevice.+*/+TCA_ACT_HW_STATS_TYPE_DISABLED,/* User is not interested in getting+*anyHWstatistics.+*/+};
On the ABI I wonder if we can redefine it a little bit..
Can we make the stat types into a bitfield?
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
That assumes that we may one day add another stat type which would
not be just based on the reporting time.
If we only foresee time-based reporting would it make sense to turn
the attribute into max acceptable delay in ms?
0 -> only immediate / blocking stats
(0, MAX) -> given reporting delay in ms is acceptable
MAX -> don't care about stats at all
@@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (!name && tb[TCA_ACT_COOKIE]) tcf_set_action_cookie(&a->act_cookie, cookie);+ if (!name)+ a->hw_stats_type = hw_stats_type;+ /* module count goes up only when brand new policy is created * if it exists and is only bound to in a_o->init() then * ACT_P_CREATED is not returned (a zero is).
From: Vladimir Oltean <olteanv@gmail.com> Date: 2020-02-29 00:50:21
On Fri, 28 Feb 2020 at 19:25, Jiri Pirko [off-list ref] wrote:
From: Jiri Pirko <redacted>
Instead of directly checking number of action entries, use
flow_offload_has_one_action() helper.
Signed-off-by: Jiri Pirko <redacted>
---
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..
At some point we should come up with a way to express the limitations
at callback registration time so we don't need to add checks like this
to all the drivers. On the TODO list it goes :)
I was thinking about that. Not really easy with the currect infra.
quoted
flow_action_for_each(i, act, flow_action) {
switch (act->id) {
case FLOW_ACTION_DROP:
Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
quoted
From: Jiri Pirko <redacted>
Currently, user who is adding an action expects HW to report stats,
however it does not have exact expectations about the stats types.
That is aligned with TCA_ACT_HW_STATS_TYPE_ANY.
Allow user to specify the type of HW stats for an action and require it.
Pass the information down to flow_offload layer.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved the stats attr from cls_flower (filter) to any action
- rebased on top of cookie offload changes
- adjusted the patch description a bit
Thanks, this looks good... I mean I wish we could just share actions
instead but this set is less objectionable than v1 :)
You can still share actions, this patchset does not stop you from doing
that.
@@ -118,6 +119,31 @@ enum tca_id {#define TCA_ID_MAX __TCA_ID_MAX+/* tca HW stats type */+enumtca_act_hw_stats_type{+TCA_ACT_HW_STATS_TYPE_ANY,/* User does not care, it's default+*whenuserdoesnotpasstheattr.+*Instructsthedriverthatuserdoesnot+*careiftheHWstatsare"immediate"+*or"delayed".+*/+TCA_ACT_HW_STATS_TYPE_IMMEDIATE,/* Means that in dump, user gets+*thecurrentHWstatsstatefrom+*thedevicequeriedatthedumptime.+*/+TCA_ACT_HW_STATS_TYPE_DELAYED,/* Means that in dump, user gets+*HWstatsthatmightbeoutofdate+*forsometime,maybecoupleof+*seconds.Thisisthecasewhendriver+*pollsstatsupdatesperiodically+*orwhenitgetsasyncstatsupdate+*fromthedevice.+*/+TCA_ACT_HW_STATS_TYPE_DISABLED,/* User is not interested in getting+*anyHWstatistics.+*/+};
On the ABI I wonder if we can redefine it a little bit..
Can we make the stat types into a bitfield?
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.
That assumes that we may one day add another stat type which would
not be just based on the reporting time.
If we only foresee time-based reporting would it make sense to turn
the attribute into max acceptable delay in ms?
0 -> only immediate / blocking stats
(0, MAX) -> given reporting delay in ms is acceptable
MAX -> don't care about stats at all
Interesting, is this "delayed" granularity something that has a usecase?
It might turn into a guessing game between user and driver during action
insertion :/
@@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (!name && tb[TCA_ACT_COOKIE]) tcf_set_action_cookie(&a->act_cookie, cookie);+ if (!name)+ a->hw_stats_type = hw_stats_type;+ /* module count goes up only when brand new policy is created * if it exists and is only bound to in a_o->init() then * ACT_P_CREATED is not returned (a zero is).
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-02-29 19:18:52
On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..
I'd think most drivers implement some form of DELAYED today, 'cause for
the number of flows things like OvS need that's the only practical one.
I was thinking to let drivers pass DELAYED here.
I agree that your patch would most likely pass ANY in almost all cases
as you shouldn't be expected to know all the drivers, but at least the
maintainers can easily just tweak the parameter.
Does that make sense? Maybe I'm missing something.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-02-29 19:29:55
On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
quoted hunk
From: Jiri Pirko <redacted>
Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to actions
- add mixed bool
---
include/net/flow_offload.h | 6 ++++++
1 file changed, 6 insertions(+)
Why do you want to place this built-in into the struct flow_action as
a boolean?
You can express the same thing through a new FLOW_ACTION_COUNTER.
I know tc has implicit counters in actions, in that case tc can just
generate the counter right after the action.
Please, explain me why it would be a problem from the driver side to
provide a separated counter action.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-02-29 19:32:26
On Fri, Feb 28, 2020 at 06:25:01PM +0100, Jiri Pirko wrote:
[...]
quoted hunk
@@ -31,7 +31,8 @@ static int mlxsw_sp_flower_parse_actions(struct mlxsw_sp *mlxsw_sp, act = flow_action_first_entry_get(flow_action); switch (act->hw_stats_type) {- case FLOW_ACTION_HW_STATS_TYPE_ANY:+ case FLOW_ACTION_HW_STATS_TYPE_ANY: /* fall-through */+ case FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE:
This TYPE_ANY mean that driver picks the counter type for you?
Otherwise, users will not have a way to know how to interpret what
kind of counter this is.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-02-29 20:14:57
On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote:
Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
quoted
From: Jiri Pirko <redacted>
+/* tca HW stats type */
+enum tca_act_hw_stats_type {
+ TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
+ * when user does not pass the attr.
+ * Instructs the driver that user does not
+ * care if the HW stats are "immediate"
+ * or "delayed".
+ */
+ TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
+ * the current HW stats state from
+ * the device queried at the dump time.
+ */
+ TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
+ * HW stats that might be out of date
+ * for some time, maybe couple of
+ * seconds. This is the case when driver
+ * polls stats updates periodically
+ * or when it gets async stats update
+ * from the device.
+ */
+ TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
+ * any HW statistics.
+ */
+};
On the ABI I wonder if we can redefine it a little bit..
Can we make the stat types into a bitfield?
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.
quoted
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.
Yeah, I wonder how that squares with strict netlink parsing.
quoted
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
Separate attribute would work.
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.
Why an array? The counter itself is added up from all the drivers.
If the value is a bitfield all drivers can just OR-in their type.
quoted
That assumes that we may one day add another stat type which would
not be just based on the reporting time.
If we only foresee time-based reporting would it make sense to turn
the attribute into max acceptable delay in ms?
0 -> only immediate / blocking stats
(0, MAX) -> given reporting delay in ms is acceptable
MAX -> don't care about stats at all
Interesting, is this "delayed" granularity something that has a usecase?
It might turn into a guessing game between user and driver during action
insertion :/
Yeah, I don't like the guessing part too, worst case refresh time may
be system dependent.
With just "DELAYED" I'm worried users will think the delay may be too
long for OvS. Or simply poll the statistics more often than the HW
reports them, which would be pointless.
For the latter case I guess the best case refresh time is needed,
while the former needs worst case. Hopefully the two are not too far
apart.
Maybe some day drivers may also tweak the refresh rate based on user
requests to save PCIe bandwidth and CPU..
Anyway.. maybe its not worth it today.
Sat, Feb 29, 2020 at 08:29:47PM CET, pablo@netfilter.org wrote:
On Fri, Feb 28, 2020 at 06:24:54PM +0100, Jiri Pirko wrote:
quoted
From: Jiri Pirko <redacted>
Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to actions
- add mixed bool
---
include/net/flow_offload.h | 6 ++++++
1 file changed, 6 insertions(+)
Why do you want to place this built-in into the struct flow_action as
a boolean?
Because it is convenient for the driver to know if multiple hw_stats_type
values are used for multiple actions.
You can express the same thing through a new FLOW_ACTION_COUNTER.
I don't see how.
I know tc has implicit counters in actions, in that case tc can just
generate the counter right after the action.
I don't follow. Each action has a separate stats.
Please, explain me why it would be a problem from the driver side to
provide a separated counter action.
I don't see any point in doing that. The action itself implies that has
stats, you don't need a separate action for that for the flow_offload
abstraction layer. What you would end up with is:
counter_action1, actual_action1, counter_action2, actual_action2,...
What is the point of that?
Sat, Feb 29, 2020 at 09:14:52PM CET, kuba@kernel.org wrote:
On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote:
quoted
Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote:
quoted
From: Jiri Pirko <redacted>
+/* tca HW stats type */
+enum tca_act_hw_stats_type {
+ TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default
+ * when user does not pass the attr.
+ * Instructs the driver that user does not
+ * care if the HW stats are "immediate"
+ * or "delayed".
+ */
+ TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets
+ * the current HW stats state from
+ * the device queried at the dump time.
+ */
+ TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets
+ * HW stats that might be out of date
+ * for some time, maybe couple of
+ * seconds. This is the case when driver
+ * polls stats updates periodically
+ * or when it gets async stats update
+ * from the device.
+ */
+ TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting
+ * any HW statistics.
+ */
+};
On the ABI I wonder if we can redefine it a little bit..
Can we make the stat types into a bitfield?
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.
quoted
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.
Yeah, I wonder how that squares with strict netlink parsing.
quoted
quoted
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
Separate attribute would work.
quoted
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.
Why an array? The counter itself is added up from all the drivers.
If the value is a bitfield all drivers can just OR-in their type.
Yeah, for uapi. Internally the array would be still needed. Also the
driver would need to somehow "write-back" the value to the offload
caller and someone (caller/tc) would have to use the array to track
these bitfields for individual callbacks (probably idr of some sort).
I don't know, is this excercise worth it?
Seems to me like we are overengineering this one a bit.
Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.
quoted
quoted
That assumes that we may one day add another stat type which would
not be just based on the reporting time.
If we only foresee time-based reporting would it make sense to turn
the attribute into max acceptable delay in ms?
0 -> only immediate / blocking stats
(0, MAX) -> given reporting delay in ms is acceptable
MAX -> don't care about stats at all
Interesting, is this "delayed" granularity something that has a usecase?
It might turn into a guessing game between user and driver during action
insertion :/
Yeah, I don't like the guessing part too, worst case refresh time may
be system dependent.
With just "DELAYED" I'm worried users will think the delay may be too
long for OvS. Or simply poll the statistics more often than the HW
reports them, which would be pointless.
For the latter case I guess the best case refresh time is needed,
while the former needs worst case. Hopefully the two are not too far
apart.
Maybe some day drivers may also tweak the refresh rate based on user
requests to save PCIe bandwidth and CPU..
Anyway.. maybe its not worth it today.
Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
quoted
Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..
I'd think most drivers implement some form of DELAYED today, 'cause for
the number of flows things like OvS need that's the only practical one.
I was thinking to let drivers pass DELAYED here.
I agree that your patch would most likely pass ANY in almost all cases
as you shouldn't be expected to know all the drivers, but at least the
maintainers can easily just tweak the parameter.
Does that make sense? Maybe I'm missing something.
Well, I guess. mlx5 only supports "delayed". It would work for it.
How about having flow_action_basic_hw_stats_types_check() as is and
add flow_action_basic_hw_stats_types_check_ext() that would accept extra
arg with enum?
Why do you want to place this built-in into the struct flow_action as
a boolean?
Because it is convenient for the driver to know if multiple hw_stats_type
values are used for multiple actions.
quoted
You can express the same thing through a new FLOW_ACTION_COUNTER.
[...]
quoted
Please, explain me why it would be a problem from the driver side to
provide a separated counter action.
I don't see any point in doing that. The action itself implies that has
stats, you don't need a separate action for that for the flow_offload
abstraction layer. What you would end up with is:
counter_action1, actual_action1, counter_action2, actual_action2,...
What is the point of that?
Yes, it's a bit more work for tc to generate counter action + actual
action.
However, netfilter has two ways to use counters:
1) per-rule counter, in this case the counter is updated after rule
matching, right before calling the action. This is the legacy mode.
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
ethtool does not have counters yet. Now there is a netlink interface
for it, there might be counters there at some point.
I'm suggesting a model that would work for the existing front-ends
using the flow_action API.
Why do you want to place this built-in into the struct flow_action as
a boolean?
Because it is convenient for the driver to know if multiple hw_stats_type
values are used for multiple actions.
quoted
You can express the same thing through a new FLOW_ACTION_COUNTER.
[...]
quoted
quoted
Please, explain me why it would be a problem from the driver side to
provide a separated counter action.
I don't see any point in doing that. The action itself implies that has
stats, you don't need a separate action for that for the flow_offload
abstraction layer. What you would end up with is:
counter_action1, actual_action1, counter_action2, actual_action2,...
What is the point of that?
Yes, it's a bit more work for tc to generate counter action + actual
action.
However, netfilter has two ways to use counters:
1) per-rule counter, in this case the counter is updated after rule
matching, right before calling the action. This is the legacy mode.
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
ethtool does not have counters yet. Now there is a netlink interface
for it, there might be counters there at some point.
I'm suggesting a model that would work for the existing front-ends
using the flow_action API.
I see. I'm interested in 1) now. If you ever want to implement 2), I see
no problem in doing it.
From: Edward Cree <hidden> Date: 2020-03-02 16:29:48
On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe. Plus, Jiri's
new API will allow specifying a counter on any action (rather than only,
implicitly, those which have .stats_update()) should that prove to be
necessary.
I really think the 'explicit counter action' is a solution in search of a
problem, let's not add random orthogonality violations. (Equally if the
counter action had been there first, I'd be against adding counters to
the other actions.)
-ed
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-03-02 19:24:42
On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
quoted
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe. Plus, Jiri's
new API will allow specifying a counter on any action (rather than only,
implicitly, those which have .stats_update()) should that prove to be
necessary.
I really think the 'explicit counter action' is a solution in search of a
problem, let's not add random orthogonality violations. (Equally if the
counter action had been there first, I'd be against adding counters to
the other actions.)
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-02 19:33:24
On Sun, 1 Mar 2020 10:00:09 +0100 Jiri Pirko wrote:
Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
quoted
On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
quoted
Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..
I'd think most drivers implement some form of DELAYED today, 'cause for
the number of flows things like OvS need that's the only practical one.
I was thinking to let drivers pass DELAYED here.
I agree that your patch would most likely pass ANY in almost all cases
as you shouldn't be expected to know all the drivers, but at least the
maintainers can easily just tweak the parameter.
Does that make sense? Maybe I'm missing something.
Well, I guess. mlx5 only supports "delayed". It would work for it.
How about having flow_action_basic_hw_stats_types_check() as is and
add flow_action_basic_hw_stats_types_check_ext() that would accept extra
arg with enum?
SGTM, perhaps with a more concise name?
Just flow_basic_hw_stats_check()?
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-02 19:39:38
On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.
quoted
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.
Yeah, I wonder how that squares with strict netlink parsing.
quoted
quoted
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
Separate attribute would work.
quoted
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.
Why an array? The counter itself is added up from all the drivers.
If the value is a bitfield all drivers can just OR-in their type.
Yeah, for uapi. Internally the array would be still needed. Also the
driver would need to somehow "write-back" the value to the offload
caller and someone (caller/tc) would have to use the array to track
these bitfields for individual callbacks (probably idr of some sort).
I don't know, is this excercise worth it?
I was thinking of just doing this on HW stats dump. Drivers which don't
report stats by definition don't need to set any bit :)
Seems to me like we are overengineering this one a bit.
That's possible, the reporting could be added later... I mostly wanted
to have the discussion.
Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.
In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-02 20:18:57
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
quoted
On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
quoted
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe. Plus, Jiri's
new API will allow specifying a counter on any action (rather than only,
implicitly, those which have .stats_update()) should that prove to be
necessary.
I really think the 'explicit counter action' is a solution in search of a
problem, let's not add random orthogonality violations. (Equally if the
counter action had been there first, I'd be against adding counters to
the other actions.)
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
Undeniably part of the reason is that given how complex flow offloads
got there may be some resistance to large re-factoring. IMHO well
thought out refactoring of stats is needed.. but I'm not convinced
this is the direction.
Could you give us clearer understanding of what the use cases for the
counter action is?
AFAIK right now actions do the accounting on input. That seems like the
only logical option. Either action takes the packet out of the action
pipeline, in which case even the counter action after will not see it,
or it doesn't and the input counter of the next action can be used.
Given counters must be next to real actions and not other counter
to have value, having them as a separate action seems to make no
difference at all (if users are silly, we can use the pipe/no-op).
IOW modeling the stats as attribute of other actions or a separate
action is entirely equivalent, and there's nothing to be gained from
moving from the existing scheme to explicit actions... other than it'd
make it look more like nft actions... :)
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-03-02 21:47:07
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
quoted
On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
quoted
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe. Plus, Jiri's
new API will allow specifying a counter on any action (rather than only,
implicitly, those which have .stats_update()) should that prove to be
necessary.
I really think the 'explicit counter action' is a solution in search of a
problem, let's not add random orthogonality violations. (Equally if the
counter action had been there first, I'd be against adding counters to
the other actions.)
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
Undeniably part of the reason is that given how complex flow offloads
got there may be some resistance to large re-factoring. IMHO well
thought out refactoring of stats is needed.. but I'm not convinced
this is the direction.
Could you give us clearer understanding of what the use cases for the
counter action is?
AFAIK right now actions do the accounting on input. That seems like the
only logical option. Either action takes the packet out of the action
pipeline, in which case even the counter action after will not see it,
or it doesn't and the input counter of the next action can be used.
Given counters must be next to real actions and not other counter
to have value, having them as a separate action seems to make no
difference at all (if users are silly, we can use the pipe/no-op).
This model that is proposed here is correct in the tc world, where
counters are tied to actions (as you describe above). However, the
flow_offload API already supports for ethtool and netfilter these
days.
In Netfilter, counters are detached from actions. Obviously, a counter
must be placed before the action _if_ the action gets the packet out
of the pipeline, e.g.
ip saddr 1.1.1.1 counter drop
In this case, the counter is placed before the 'drop' action. Users
that need no counters have to remove 'counter' from the rule syntax to
opt-out.
IOW modeling the stats as attribute of other actions or a separate
action is entirely equivalent, and there's nothing to be gained from
moving from the existing scheme to explicit actions... other than it'd
make it look more like nft actions... :)
I just wonder if a model that allows tc and netfilter to use this new
statistics infrastructure would make everyone happy. My understanding
is that it is not far away from what this patchset provides.
The retorical question here probably is if you still want to allow the
Netfilter front-end to benefit from this new flow_action API
extension.
The real question is: if you think this tc counter+action scheme can
be used by netfilter, then please explain how.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-02 22:49:33
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 04:29:32PM +0000, Edward Cree wrote:
quoted
On 02/03/2020 13:20, Pablo Neira Ayuso wrote:
quoted
2) explicit counter action, in this case the user specifies explicitly
that it needs a counter in a given position of the rule. This
counter might come before or after the actual action.
But the existing API can already do this, with a gact pipe. Plus, Jiri's
new API will allow specifying a counter on any action (rather than only,
implicitly, those which have .stats_update()) should that prove to be
necessary.
I really think the 'explicit counter action' is a solution in search of a
problem, let's not add random orthogonality violations. (Equally if the
counter action had been there first, I'd be against adding counters to
the other actions.)
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
Undeniably part of the reason is that given how complex flow offloads
got there may be some resistance to large re-factoring. IMHO well
thought out refactoring of stats is needed.. but I'm not convinced
this is the direction.
Could you give us clearer understanding of what the use cases for the
counter action is?
AFAIK right now actions do the accounting on input. That seems like the
only logical option. Either action takes the packet out of the action
pipeline, in which case even the counter action after will not see it,
or it doesn't and the input counter of the next action can be used.
Given counters must be next to real actions and not other counter
to have value, having them as a separate action seems to make no
difference at all (if users are silly, we can use the pipe/no-op).
This model that is proposed here is correct in the tc world, where
counters are tied to actions (as you describe above). However, the
flow_offload API already supports for ethtool and netfilter these
days.
In Netfilter, counters are detached from actions. Obviously, a counter
must be placed before the action _if_ the action gets the packet out
of the pipeline, e.g.
ip saddr 1.1.1.1 counter drop
In this case, the counter is placed before the 'drop' action. Users
that need no counters have to remove 'counter' from the rule syntax to
opt-out.
In Jiri's set if counter exists DROP should get the ANY flag, if
counter is not there - DISABLED.
quoted
IOW modeling the stats as attribute of other actions or a separate
action is entirely equivalent, and there's nothing to be gained from
moving from the existing scheme to explicit actions... other than it'd
make it look more like nft actions... :)
I just wonder if a model that allows tc and netfilter to use this new
statistics infrastructure would make everyone happy. My understanding
is that it is not far away from what this patchset provides.
The retorical question here probably is if you still want to allow the
Netfilter front-end to benefit from this new flow_action API
extension.
The real question is: if you think this tc counter+action scheme can
be used by netfilter, then please explain how.
In Jiri's latest patch set the counter type is per action, so just
"merge right" the counter info into the next action and the models
are converted.
If user is silly and has multiple counter actions in a row - the
pipe/no-op action comes into play (that isn't part of this set,
as Jiri said).
Can you give us examples of what wouldn't work? Can you for instance
share the counter across rules?
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
AFAICT with Jiri's change we only need one check in the drivers to
convert from old scheme to new, with explicit action we need two
(additional one being ignoring the counter action). Not a big deal,
but 1 is less than 2 🤷♂️
Mon, Mar 02, 2020 at 08:33:19PM CET, kuba@kernel.org wrote:
On Sun, 1 Mar 2020 10:00:09 +0100 Jiri Pirko wrote:
quoted
Sat, Feb 29, 2020 at 08:18:48PM CET, kuba@kernel.org wrote:
quoted
On Sat, 29 Feb 2020 08:40:04 +0100 Jiri Pirko wrote:
quoted
Fri, Feb 28, 2020 at 08:40:56PM CET, kuba@kernel.org wrote:
quoted
On Fri, 28 Feb 2020 18:24:56 +0100 Jiri Pirko wrote:
quoted
@@ -299,6 +300,9 @@ static int bnxt_tc_parse_actions(struct bnxt *bp, return -EINVAL; }+ if (!flow_action_basic_hw_stats_types_check(flow_action, extack))+ return -EOPNOTSUPP;
Could we have this helper take one stat type? To let drivers pass the
stat type they support?
That would be always "any" as "any" is supported by all drivers.
And that is exactly what the helper checks..
I'd think most drivers implement some form of DELAYED today, 'cause for
the number of flows things like OvS need that's the only practical one.
I was thinking to let drivers pass DELAYED here.
I agree that your patch would most likely pass ANY in almost all cases
as you shouldn't be expected to know all the drivers, but at least the
maintainers can easily just tweak the parameter.
Does that make sense? Maybe I'm missing something.
Well, I guess. mlx5 only supports "delayed". It would work for it.
How about having flow_action_basic_hw_stats_types_check() as is and
add flow_action_basic_hw_stats_types_check_ext() that would accept extra
arg with enum?
SGTM, perhaps with a more concise name?
Just flow_basic_hw_stats_check()?
Mon, Mar 02, 2020 at 08:39:33PM CET, kuba@kernel.org wrote:
On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
quoted
On request:
- no attr -> any stats allowed but some stats must be provided *
- 0 -> no stats requested / disabled
- 0x1 -> must be stat type0
- 0x6 -> stat type1 or stat type2 are both fine
I was thinking about this of course. On the write side, this is ok
however, this is very tricky on read side. See below.
quoted
* no attr kinda doesn't work 'cause u32 offload has no stats and this
is action-level now, not flower-level :S What about u32 and matchall?
The fact that cls does not implement stats offloading is a lack of
feature of the particular cls.
Yeah, I wonder how that squares with strict netlink parsing.
quoted
quoted
We can add a separate attribute with "active" stat types:
- no attr -> old kernel
- 0 -> no stats are provided / stats disabled
- 0x1 -> only stat type0 is used by drivers
- 0x6 -> at least one driver is using type1 and one type2
There are 2 problems:
1) There is a mismatch between write and read. User might pass different
value than it eventually gets from kernel. I guess this might be fine.
Separate attribute would work.
quoted
2) Much bigger problem is, that since the same action may be offloaded
by multiple drivers, the read would have to provide an array of
bitfields, each array item would represent one offloaded driver. That is
why I decided for simple value instead of bitfield which is the same on
write and read.
Why an array? The counter itself is added up from all the drivers.
If the value is a bitfield all drivers can just OR-in their type.
Yeah, for uapi. Internally the array would be still needed. Also the
driver would need to somehow "write-back" the value to the offload
caller and someone (caller/tc) would have to use the array to track
these bitfields for individual callbacks (probably idr of some sort).
I don't know, is this excercise worth it?
I was thinking of just doing this on HW stats dump. Drivers which don't
report stats by definition don't need to set any bit :)
quoted
Seems to me like we are overengineering this one a bit.
That's possible, the reporting could be added later... I mostly wanted
to have the discussion.
Okay.
quoted
Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.
In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.
Odd, no attribute meaning "any". I think it is polite to fillup the
attribute for dump if kernel supports the attribute. However, here, we
would not fill it up in case of "any". That is quite odd.
We can have a bit that would mean "any" though. What do you think?
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-03-03 17:25:34
On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote:
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
[...]
quoted
The real question is: if you think this tc counter+action scheme can
be used by netfilter, then please explain how.
In Jiri's latest patch set the counter type is per action, so just
"merge right" the counter info into the next action and the models
are converted.
The input "merge right" approach might work.
If user is silly and has multiple counter actions in a row - the
pipe/no-op action comes into play (that isn't part of this set,
as Jiri said).
Probably gact pipe action with counters can be mapped to the counter
action that netfilter needs. Is this a valid use-case you consider for
the tc hardware offload?
Can you give us examples of what wouldn't work? Can you for instance
share the counter across rules?
Yes, there might be counters that are shared accross rules, see
nfacct. Two different rules might refer to the same counter, IIRC
there is a way to do this in tc too.
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
You mean, in case that different counter types are specified, eg. one
action using delayed and another action using immediate?
AFAICT with Jiri's change we only need one check in the drivers to
convert from old scheme to new, with explicit action we need two
(additional one being ignoring the counter action). Not a big deal,
but 1 is less than 2 🤷♂️
What changes are expected to retrieve counter stats?
Will per-flow stats remain in place after this place?
Thank you.
From: Edward Cree <hidden> Date: 2020-03-03 18:56:12
On 02/03/2020 22:49, Jakub Kicinski wrote:
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
The technical reason is that having two ways to do things where one would
suffice means more code to be written, tested, debugged. So if you want
to add this you need to convince us that the existing way (a) doesn't
meet your needs and (b) can't be extended to cover them.
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
For the record, I produced a patch series[1] to support that, but it
wasn't acceptable because none of the in-tree drivers implemented the
facility. My hope is that we'll be upstreaming our new driver Real
Soon Now™, at which point I'll rebase and repost those changes.
Alternatively if any other vendor wants to support it in their driver
they could use those patches as a base.
-ed
[1]: http://patchwork.ozlabs.org/cover/1110071/ ("flow_offload: Re-add per-action statistics")
From: Edward Cree <hidden> Date: 2020-03-03 19:13:38
On 28/02/2020 17:24, Jiri Pirko wrote:
quoted hunk
From: Jiri Pirko <redacted>
Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to actions
- add mixed bool
---
include/net/flow_offload.h | 6 ++++++
1 file changed, 6 insertions(+)
Some sort of comment in the commit message the effect that this will
be set in patch #12 would be nice (and would have saved me some
reviewing time looking for it ;)
Strictly speaking this violates SPOT; I know a helper to calculate
this 'at runtime' in the driver would have to loop over actions,
but it's control-plane so performance doesn't matter :grin:
I'd suggest something like adding an internal-use-only MIXED value to
the enum, and then having a helper
enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action);
which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever
type all the actions have (except that the 'different' check might
be further complicated to ignore DISABLED, since most
flow_action_entries will be for TC actions with no .update_stats()
which thus don't want stats and can use DISABLED to express that).
That then avoids having to rely on the first entry having the stats
type (so flow_action_first_entry_get() goes away).
-ed
unsigned int num_entries;
struct flow_action_entry entries[0];
};
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-03 19:26:21
On Tue, 3 Mar 2020 18:55:54 +0000 Edward Cree wrote:
quoted
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
For the record, I produced a patch series[1] to support that, but it
wasn't acceptable because none of the in-tree drivers implemented the
facility. My hope is that we'll be upstreaming our new driver Real
Soon Now™, at which point I'll rebase and repost those changes.
Sorry, I wasn't completely fair :) Looking forward :)
Alternatively if any other vendor wants to support it in their driver
they could use those patches as a base.
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-03 19:35:02
On Tue, 3 Mar 2020 18:25:25 +0100 Pablo Neira Ayuso wrote:
On Mon, Mar 02, 2020 at 02:49:28PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
[...]
quoted
quoted
The real question is: if you think this tc counter+action scheme can
be used by netfilter, then please explain how.
In Jiri's latest patch set the counter type is per action, so just
"merge right" the counter info into the next action and the models
are converted.
The input "merge right" approach might work.
quoted
If user is silly and has multiple counter actions in a row - the
pipe/no-op action comes into play (that isn't part of this set,
as Jiri said).
Probably gact pipe action with counters can be mapped to the counter
action that netfilter needs. Is this a valid use-case you consider for
the tc hardware offload?
Once actions can be shared I think it'd be a pretty useful thing for tc
hardware offloads in case HW has limited counters.
quoted
Can you give us examples of what wouldn't work? Can you for instance
share the counter across rules?
Yes, there might be counters that are shared accross rules, see
nfacct. Two different rules might refer to the same counter, IIRC
there is a way to do this in tc too.
Yup, not implemented for offload, tho.
quoted
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
You mean, in case that different counter types are specified, eg. one
action using delayed and another action using immediate?
I meant the work Ed just pointed to, and what you ask about below.
quoted
AFAICT with Jiri's change we only need one check in the drivers to
convert from old scheme to new, with explicit action we need two
(additional one being ignoring the counter action). Not a big deal,
but 1 is less than 2 🤷♂️
What changes are expected to retrieve counter stats?
Will per-flow stats remain in place after this place?
In theory it doesn't have to, because action stats are more flexible.
In practice I doubt anyone will take on the conversion, so we'll have
to live with two ways for a while..
From: Jakub Kicinski <kuba@kernel.org> Date: 2020-03-03 19:48:30
On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote:
quoted
quoted
Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.
In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.
Odd, no attribute meaning "any".
OTOH it does match up with old kernel behavior quite nicely, today
there is no attribute and it means "any".
I think it is polite to fillup the attribute for dump if kernel
supports the attribute. However, here, we would not fill it up in
case of "any". That is quite odd.
I see, it does seem nice to report the attribute, but again, won't
the user space which wants to run on older kernels have to treat
no attr as "any"?
We can have a bit that would mean "any" though. What do you think?
It'd be a dead bit for the "stat types used" attribute, but I don't
mind it if you prefer to go this way.
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-03-03 20:27:46
On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
On 02/03/2020 22:49, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
The technical reason is that having two ways to do things where one would
suffice means more code to be written, tested, debugged. So if you want
to add this you need to convince us that the existing way (a) doesn't
meet your needs and (b) can't be extended to cover them.
One single unified way to express the hardware offload for _every_
supported frontend is the way to go. The flow_offload API provides a
framework to model all hardware offloads for each existing front-end.
I understand your motivation might be a specific front-end of your
choice, that's fair enough.
quoted
Also neither proposal addresses the problem of reporting _different_
counter values at different stages in the pipeline, i.e. moving from
stats per flow to per action. But nobody seems to be willing to work
on that.
For the record, I produced a patch series[1] to support that, but it
wasn't acceptable because none of the in-tree drivers implemented the
facility. My hope is that we'll be upstreaming our new driver Real
Soon Now™, at which point I'll rebase and repost those changes.
Alternatively if any other vendor wants to support it in their driver
they could use those patches as a base.
Great, I am very much looking forward to reviewing your upstream code.
Just keep in my mind that whatever proposal you make must work for
netfilter too.
Thank you.
From: Edward Cree <hidden> Date: 2020-03-03 21:07:06
On 03/03/2020 20:27, Pablo Neira Ayuso wrote:
On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
quoted
On 02/03/2020 22:49, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
The technical reason is that having two ways to do things where one would
suffice means more code to be written, tested, debugged. So if you want
to add this you need to convince us that the existing way (a) doesn't
meet your needs and (b) can't be extended to cover them.
One single unified way to express the hardware offload for _every_
supported frontend is the way to go. The flow_offload API provides a
framework to model all hardware offloads for each existing front-end.
I understand your motivation might be a specific front-end of your
choice, that's fair enough.
I think we've misunderstood each other (90% my fault).
When you wrote "restrict the API to tc" I read that as "restrict growth of
the API for flow offloading" (which I *do* want); I've now re-parsed and
believe you meant it as "limit the API so that only tc may use it" (which
is not my desire at all).
Thus, when I spoke of "two ways to do things" I meant that _within_ the
(unified) flow_offload API there should be a single approach to stats
(the counters attached to actions), to which levels above and below it
impedance-match as necessary (e.g. by merging netfilter count actions
onto the following action as Jakub described), rather than bundling
two interfaces (tc-style counters and separate counter actions) into
one API (which would mean that drivers would all need to write code to
handle both kinds, at no gain of expressiveness).
I was *not* referring to tc and netfilter as the "two different ways", but
I can see why you read it that way.
I hope that makes sense now.
-ed
From: Pablo Neira Ayuso <pablo@netfilter.org> Date: 2020-03-03 21:25:18
On Tue, Mar 03, 2020 at 09:06:48PM +0000, Edward Cree wrote:
On 03/03/2020 20:27, Pablo Neira Ayuso wrote:
quoted
On Tue, Mar 03, 2020 at 06:55:54PM +0000, Edward Cree wrote:
quoted
On 02/03/2020 22:49, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 22:46:59 +0100 Pablo Neira Ayuso wrote:
quoted
On Mon, Mar 02, 2020 at 12:18:52PM -0800, Jakub Kicinski wrote:
quoted
On Mon, 2 Mar 2020 20:24:37 +0100 Pablo Neira Ayuso wrote:
quoted
It looks to me that you want to restrict the API to tc for no good
_technical_ reason.
The technical reason is that having two ways to do things where one would
suffice means more code to be written, tested, debugged. So if you want
to add this you need to convince us that the existing way (a) doesn't
meet your needs and (b) can't be extended to cover them.
One single unified way to express the hardware offload for _every_
supported frontend is the way to go. The flow_offload API provides a
framework to model all hardware offloads for each existing front-end.
I understand your motivation might be a specific front-end of your
choice, that's fair enough.
I think we've misunderstood each other (90% my fault).
When you wrote "restrict the API to tc" I read that as "restrict growth of
the API for flow offloading" (which I *do* want); I've now re-parsed and
believe you meant it as "limit the API so that only tc may use it" (which
is not my desire at all).
Thus, when I spoke of "two ways to do things" I meant that _within_ the
(unified) flow_offload API there should be a single approach to stats
(the counters attached to actions), to which levels above and below it
impedance-match as necessary (e.g. by merging netfilter count actions
onto the following action as Jakub described) rather than bundling
two interfaces (tc-style counters and separate counter actions)
into one API (which would mean that drivers would all need to write
code to handle both kinds, at no gain of expressiveness).
It's not that natural to express counters like you prefer for
netfilter, but fair enough, we'll carry on that extra burden of
merging counters to actions.
Sometimes decisions just need a second round: I will expect broken
endianness in drivers because of the 32-bit word choice for the
payload mangling API. But that's a different story.
Noone to blame, there is still experimentation going on in this API.
I was *not* referring to tc and netfilter
as the "two different ways", but I can see why you read it that
way.
Tue, Mar 03, 2020 at 08:48:25PM CET, kuba@kernel.org wrote:
On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote:
quoted
quoted
quoted
Also there would be no "any" it would be type0|type1|type2 the user
would have to pass. If new type appears, the userspace would have to be
updated to do "any" again :/ This is inconvenient.
In my proposal above I was suggesting no attr to mean any. I think in
your current code ANY already doesn't include disabled so old user
space should not see any change.
Odd, no attribute meaning "any".
OTOH it does match up with old kernel behavior quite nicely, today
there is no attribute and it means "any".
quoted
I think it is polite to fillup the attribute for dump if kernel
supports the attribute. However, here, we would not fill it up in
case of "any". That is quite odd.
I see, it does seem nice to report the attribute, but again, won't
the user space which wants to run on older kernels have to treat
no attr as "any"?
Okay.
quoted
We can have a bit that would mean "any" though. What do you think?
It'd be a dead bit for the "stat types used" attribute, but I don't
mind it if you prefer to go this way.
Tue, Mar 03, 2020 at 08:13:23PM CET, ecree@solarflare.com wrote:
On 28/02/2020 17:24, Jiri Pirko wrote:
quoted
From: Jiri Pirko <redacted>
Initially, pass "ANY" (struct is zeroed) to the drivers as that is the
current implicit value coming down to flow_offload. Add a bool
indicating that entries have mixed HW stats type.
Signed-off-by: Jiri Pirko <redacted>
---
v1->v2:
- moved to actions
- add mixed bool
---
include/net/flow_offload.h | 6 ++++++
1 file changed, 6 insertions(+)
Some sort of comment in the commit message the effect that this will
be set in patch #12 would be nice (and would have saved me some
reviewing time looking for it ;)
Strictly speaking this violates SPOT; I know a helper to calculate
this 'at runtime' in the driver would have to loop over actions,
but it's control-plane so performance doesn't matter :grin:
That is what I wanted to avoid.
I'd suggest something like adding an internal-use-only MIXED value to
the enum, and then having a helper
enum flow_action_hw_state_type flow_action_single_stats_type(struct flow_action *action);
which could return FLOW_ACTION_HW_STATS_TYPE_MIXED or else whichever
type all the actions have (except that the 'different' check might
be further complicated to ignore DISABLED, since most
flow_action_entries will be for TC actions with no .update_stats()
which thus don't want stats and can use DISABLED to express that).
That then avoids having to rely on the first entry having the stats
type (so flow_action_first_entry_get() goes away).
No problem. I can call a helper that would go over the entries from
driver. As you say, it is a slow path.
Will do.
-ed
quoted
unsigned int num_entries;
struct flow_action_entry entries[0];
};