diff mbox series

[net-next,3/3] flow_offload: add process to update action stats from hardware

Message ID 20210722091938.12956-4-simon.horman@corigine.com
State New
Headers show
Series flow_offload: hardware offload of TC actions | expand

Commit Message

Simon Horman July 22, 2021, 9:19 a.m. UTC 
From: Baowen Zheng <baowen.zheng@corigine.com>

When collecting stats for actions update them using both
both hardware and software counters.

Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com>
Signed-off-by: Louis Peens <louis.peens@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 include/net/act_api.h      |  1 +
 include/net/flow_offload.h |  2 +-
 include/net/pkt_cls.h      |  4 ++++
 net/sched/act_api.c        | 49 ++++++++++++++++++++++++++++++++++----
 4 files changed, 51 insertions(+), 5 deletions(-)

Comments

Jamal Hadi Salim Aug. 3, 2021, 11:24 a.m. UTC | #1 
On 2021-07-22 5:19 a.m., Simon Horman wrote:

[..]

>   /* offload the tc command after deleted */

>   int tcf_action_offload_del_post(struct flow_offload_action *fl_act,

>   				struct tc_action *actions[],

> @@ -1255,6 +1293,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,

>   	if (p == NULL)

>   		goto errout;

>   

> +	/* update hw stats for this action */

> +	tcf_action_update_hw_stats(p);


This is more a curiosity than a comment on the patch: Is the
driver polling for these stats synchronously and what we get here
is the last update or do we end up invoking beyond
the driver when requesting for the stats?

Overall commentary from looking at the patch set:
I believe your patches will support the individual tc actions
add/del/get/dump command line requests.
What is missing is an example usage all the way to the driver. I am sure
you have additional patches that put this to good  use. My suggestion
is to test that cli with that pov against your overall patches and
show this in your commit logs - even if those patches are to follow
later.


cheers,
jamal
Simon Horman Aug. 3, 2021, 11:35 a.m. UTC | #2 
On Tue, Aug 03, 2021 at 07:24:47AM -0400, Jamal Hadi Salim wrote:
> On 2021-07-22 5:19 a.m., Simon Horman wrote:

> 

> [..]

> 

> >   /* offload the tc command after deleted */

> >   int tcf_action_offload_del_post(struct flow_offload_action *fl_act,

> >   				struct tc_action *actions[],

> > @@ -1255,6 +1293,9 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,

> >   	if (p == NULL)

> >   		goto errout;

> > +	/* update hw stats for this action */

> > +	tcf_action_update_hw_stats(p);

> 

> This is more a curiosity than a comment on the patch: Is the

> driver polling for these stats synchronously and what we get here

> is the last update or do we end up invoking beyond

> the driver when requesting for the stats?


I would have to double check but I believe the driver will report
back stats already received from the HW, rather than going all the way
to HW when the above call is made.

> Overall commentary from looking at the patch set:

> I believe your patches will support the individual tc actions

> add/del/get/dump command line requests.


Yes, that is the aim of this patchset.

> What is missing is an example usage all the way to the driver. I am sure

> you have additional patches that put this to good  use. My suggestion

> is to test that cli with that pov against your overall patches and

> show this in your commit logs - even if those patches are to follow

> later.


Thanks, I'll see about making that so.

Just to be clear. We do have patches for the driver. And we do plan to post
them for inclusion in mainline. But I do believe that from a review
perspective its easier if one thing follows another.
diff mbox series

Patch

diff --git a/include/net/act_api.h b/include/net/act_api.h
index 086b291e9530..fe8331b5efce 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -233,6 +233,7 @@  static inline void tcf_action_inc_overlimit_qstats(struct tc_action *a)
 void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
 			     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,
diff --git a/include/net/flow_offload.h b/include/net/flow_offload.h
index 26644596fd54..467688fff7ce 100644
--- a/include/net/flow_offload.h
+++ b/include/net/flow_offload.h
@@ -560,7 +560,7 @@  enum flow_act_command {
 };
 
 struct flow_offload_action {
-	struct netlink_ext_ack *extack;
+	struct netlink_ext_ack *extack; /* NULL in FLOW_ACT_STATS process*/
 	enum flow_act_command command;
 	struct flow_stats stats;
 	struct flow_action action;
diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 03dae225d64f..569c9294b15b 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -282,6 +282,10 @@  tcf_exts_stats_update(const struct tcf_exts *exts,
 	for (i = 0; i < exts->nr_actions; i++) {
 		struct tc_action *a = exts->actions[i];
 
+		/* if stats from hw, just skip */
+		if (!tcf_action_update_hw_stats(a))
+			continue;
+
 		tcf_action_stats_update(a, bytes, packets, drops,
 					lastuse, true);
 		a->used_hw_stats = used_hw_stats;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 23a4538916af..7d5535bc2c13 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1089,15 +1089,18 @@  int tcf_action_offload_cmd_pre(struct tc_action *actions[],
 EXPORT_SYMBOL(tcf_action_offload_cmd_pre);
 
 int tcf_action_offload_cmd_post(struct flow_offload_action *fl_act,
-				struct netlink_ext_ack *extack)
+				struct netlink_ext_ack *extack,
+				bool keep_fl_act)
 {
 	if (IS_ERR(fl_act))
 		return PTR_ERR(fl_act);
 
 	flow_indr_dev_setup_offload(NULL, NULL, TC_SETUP_ACT, fl_act, NULL, NULL);
 
-	tc_cleanup_flow_action(&fl_act->action);
-	kfree(fl_act);
+	if (!keep_fl_act) {
+		tc_cleanup_flow_action(&fl_act->action);
+		kfree(fl_act);
+	}
 	return 0;
 }
 
@@ -1115,10 +1118,45 @@  int tcf_action_offload_cmd(struct tc_action *actions[],
 	if (err)
 		return err;
 
-	return tcf_action_offload_cmd_post(fl_act, extack);
+	return tcf_action_offload_cmd_post(fl_act, extack, false);
 }
 EXPORT_SYMBOL(tcf_action_offload_cmd);
 
+int tcf_action_update_hw_stats(struct tc_action *action)
+{
+	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
+		[0] = action,
+	};
+	struct flow_offload_action *fl_act;
+	int err = 0;
+
+	err = tcf_action_offload_cmd_pre(actions,
+					 FLOW_ACT_STATS,
+					 NULL,
+					 &fl_act);
+	if (err)
+		goto err_out;
+
+	err = tcf_action_offload_cmd_post(fl_act, NULL, true);
+
+	if (fl_act->stats.lastused) {
+		tcf_action_stats_update(action, fl_act->stats.bytes,
+					fl_act->stats.pkts,
+					fl_act->stats.drops,
+					fl_act->stats.lastused,
+					true);
+		err = 0;
+	} else {
+		err = -EOPNOTSUPP;
+	}
+	tc_cleanup_flow_action(&fl_act->action);
+	kfree(fl_act);
+
+err_out:
+	return err;
+}
+EXPORT_SYMBOL(tcf_action_update_hw_stats);
+
 /* offload the tc command after deleted */
 int tcf_action_offload_del_post(struct flow_offload_action *fl_act,
 				struct tc_action *actions[],
@@ -1255,6 +1293,9 @@  int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *p,
 	if (p == NULL)
 		goto errout;
 
+	/* update hw stats for this action */
+	tcf_action_update_hw_stats(p);
+
 	/* compat_mode being true specifies a call that is supposed
 	 * to add additional backward compatibility statistic TLVs.
 	 */