RE: [PATCH v6 net-next 08/12] flow_offload: add process to update action stats from hardware

On December 12, 2021 3:52 AM, Jamal Hadi Salim wrote:

On 2021-12-09 04:28, Simon Horman wrote:

quoted

include/net/act_api.h |  1 +
  include/net/pkt_cls.h | 18 ++++++++++--------
  net/sched/act_api.c   | 34 ++++++++++++++++++++++++++++++++++
  3 files changed, 45 insertions(+), 8 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h index

7e4e79b50216..ce094e79f722 100644

--- a/include/net/act_api.h
+++ b/include/net/act_api.h

@@ -253,6 +253,7 @@ void tcf_action_update_stats(struct tc_action *a,

u64 bytes, u64 packets,

quoted

  			     u64 drops, bool hw);
  int tcf_action_copy_stats(struct sk_buff *, struct tc_action *,
int);

+int tcf_action_update_hw_stats(struct tc_action *action);
  int tcf_action_check_ctrlact(int action, struct tcf_proto *tp,
  			     struct tcf_chain **handle,
  			     struct netlink_ext_ack *newchain); diff --git
a/include/net/pkt_cls.h b/include/net/pkt_cls.h index
13f0e4a3a136..1942fe72b3e3 100644

--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h

@@ -269,18 +269,20 @@ tcf_exts_stats_update(const struct tcf_exts *exts,
  #ifdef CONFIG_NET_CLS_ACT
  	int i;

-	preempt_disable();
-
  	for (i = 0; i < exts->nr_actions; i++) {
  		struct tc_action *a = exts->actions[i];

-		tcf_action_stats_update(a, bytes, packets, drops,
-					lastuse, true);
-		a->used_hw_stats = used_hw_stats;
-		a->used_hw_stats_valid = used_hw_stats_valid;
-	}
+		/* if stats from hw, just skip */
+		if (tcf_action_update_hw_stats(a)) {
+			preempt_disable();
+			tcf_action_stats_update(a, bytes, packets, drops,
+						lastuse, true);
+			preempt_enable();

-	preempt_enable();
+			a->used_hw_stats = used_hw_stats;
+			a->used_hw_stats_valid = used_hw_stats_valid;
+		}
+	}
  #endif
  }

Sorry - didnt quiet follow this one even after reading to the end.
I may have missed the obvious in the equivalence:
In the old code we did the preempt first then collect. The changed version only
does it if tcf_action_update_hw_stats() is true.

Hi Jamal, for this change, this is because for the function of tcf_action_update_hw_stats, it will try to retrieve hw stats fron hardware. But in he process of retrieving stats information, the driver may have
Lock or other sleeping function. So we should not call tcf_action_update_hw_stats function in context of preempt_disable.
Actually, since there is no vendor to support update single action stats from hardware, so it is not obvious, we will post our implement support after these patches set. 
Do you think if it make sense?

cheers,
jamal