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(+)

diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 4e864c34a1b0..eee1cbc5db3c 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h

@@ -154,6 +154,10 @@ enum flow_action_mangle_base {
 	FLOW_ACT_MANGLE_HDR_TYPE_UDP,
 };
 
+enum flow_action_hw_stats_type {
+	FLOW_ACTION_HW_STATS_TYPE_ANY,
+};
+
 typedef void (*action_destr)(void *priv);
 
 struct flow_action_cookie {

@@ -168,6 +172,7 @@ void flow_action_cookie_destroy(struct flow_action_cookie *cookie);
 
 struct flow_action_entry {
 	enum flow_action_id		id;
+	enum flow_action_hw_stats_type	hw_stats_type;
 	action_destr			destructor;
 	void				*destructor_priv;
 	union {

@@ -228,6 +233,7 @@ struct flow_action_entry {
 };
 
 struct flow_action {
+	bool				mixed_hw_stats_types;

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];
 };