Re: [patch net-next 00/10] net: allow user specify TC filter HW stats type

[Jiri Pirko <jiri@resnulli.us>]

Fri, Feb 28, 2020 at 08:59:09PM CET, pablo@netfilter.org wrote:
>Hi Pirko,
>
>On Tue, Feb 25, 2020 at 05:22:03PM +0100, Jiri Pirko wrote:
>[...]
>> Eh, that is not that simple. The existing users are used to the fact
>> that the actions are providing counters by themselves. Having and
>> explicit counter action like this would break that expectation.
>> Also, I think it should be up to the driver implementation. Some HW
>> might only support stats per rule, not the actions. Driver should fit
>> into the existing abstraction, I think it is fine.
>
>Something like the sketch patch that I'm attaching?

But why? Actions are separate entities, with separate counters. The
driver is either able to offload that or not. Up to the driver to
abstract this out.


>
>The idea behind it is to provide a counter action through the
>flow_action API. Then, tc_setup_flow_action() checks if this action
>comes with counters in that case the counter action is added.
>
>My patch assumes tcf_vlan_counter() provides tells us what counter
>type the user wants, I just introduced this to provide an example.
>
>Thank you.

>diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
>index c6f7bd22db60..1a5006091edc 100644
>--- a/include/net/flow_offload.h
>+++ b/include/net/flow_offload.h
>@@ -138,9 +138,16 @@ enum flow_action_id {
> 	FLOW_ACTION_MPLS_PUSH,
> 	FLOW_ACTION_MPLS_POP,
> 	FLOW_ACTION_MPLS_MANGLE,
>+	FLOW_ACTION_COUNTER,
> 	NUM_FLOW_ACTIONS,
> };
> 
>+enum flow_action_counter_type {
>+	FLOW_COUNTER_DISABLED		= 0,
>+	FLOW_COUNTER_DELAYED,
>+	FLOW_COUNTER_IMMEDIATE,
>+};
>+
> /* This is mirroring enum pedit_header_type definition for easy mapping between
>  * tc pedit action. Legacy TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK is mapped to
>  * FLOW_ACT_MANGLE_UNSPEC, which is supported by no driver.
>@@ -213,6 +220,9 @@ struct flow_action_entry {
> 			u8		bos;
> 			u8		ttl;
> 		} mpls_mangle;
>+		struct {				/* FLOW_ACTION_COUNTER */
>+			enum flow_action_counter_type	type;
>+		} counter;
> 	};
> };
> 
>diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>index 13c33eaf1ca1..984f2129c760 100644
>--- a/net/sched/cls_api.c
>+++ b/net/sched/cls_api.c
>@@ -3435,6 +3435,7 @@ static void tcf_sample_get_group(struct flow_action_entry *entry,
> int tc_setup_flow_action(struct flow_action *flow_action,
> 			 const struct tcf_exts *exts)
> {
>+	enum flow_action_counter_type counter = FLOW_COUNTER_DISABLED;
> 	struct tc_action *act;
> 	int i, j, k, err = 0;
> 
>@@ -3489,6 +3490,7 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> 				err = -EOPNOTSUPP;
> 				goto err_out_locked;
> 			}
>+			counter = tcf_vlan_counter(act);
> 		} else if (is_tcf_tunnel_set(act)) {
> 			entry->id = FLOW_ACTION_TUNNEL_ENCAP;
> 			err = tcf_tunnel_encap_get_tunnel(entry, act);
>@@ -3567,10 +3569,19 @@ int tc_setup_flow_action(struct flow_action *flow_action,
> 			err = -EOPNOTSUPP;
> 			goto err_out_locked;
> 		}
>-		spin_unlock_bh(&act->tcfa_lock);
> 
> 		if (!is_tcf_pedit(act))
> 			j++;
>+
>+		if (counter) {
>+			struct flow_action_entry *entry;
>+
>+			entry = &flow_action->entries[j++];
>+			entry->id = FLOW_ACTION_COUNTER;
>+			entry->counter.type = counter;
>+			counter = FLOW_COUNTER_DISABLED;
>+		}
>+		spin_unlock_bh(&act->tcfa_lock);
> 	}
> 
> err_out: