| Message ID | 20210722091938.12956-4-simon.horman@corigine.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flow_offload: hardware offload of TC actions | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: jiri@resnulli.us |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 3621 this patch: 3621 |
| netdev/kdoc | success | Errors and warnings before: 4 this patch: 4 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 101 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 3776 this patch: 3776 |
| netdev/header_inline | success | Link |
On Thu 22 Jul 2021 at 12:19, Simon Horman <simon.horman@corigine.com> wrote: > 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(-) > > 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; > + Is it okay to call this inside preempt disable section? > 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; > + Having some way to distinguish offloaded actions would also be useful here to skip this function. I wonder how this affects dump rate when executed for every single action, even when none of them were offloaded through action API. > + 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. > */
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
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 --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. */