| Message ID | 20200228172505.14386-13-jiri@resnulli.us |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: allow user specify TC action HW stats type | expand |
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote: > From: Jiri Pirko <jiri@mellanox.com> > > Currently, user who is adding an action expects HW to report stats, > however it does not have exact expectations about the stats types. > That is aligned with TCA_ACT_HW_STATS_TYPE_ANY. > > Allow user to specify the type of HW stats for an action and require it. > > Pass the information down to flow_offload layer. > > Signed-off-by: Jiri Pirko <jiri@mellanox.com> > --- > v1->v2: > - moved the stats attr from cls_flower (filter) to any action > - rebased on top of cookie offload changes > - adjusted the patch description a bit Thanks, this looks good... I mean I wish we could just share actions instead but this set is less objectionable than v1 :) > diff --git a/include/net/act_api.h b/include/net/act_api.h > index 71347a90a9d1..02b9bffa17ed 100644 > --- a/include/net/act_api.h > +++ b/include/net/act_api.h > @@ -39,6 +39,7 @@ struct tc_action { > struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw; > struct gnet_stats_queue __percpu *cpu_qstats; > struct tc_cookie __rcu *act_cookie; > + enum tca_act_hw_stats_type hw_stats_type; > struct tcf_chain __rcu *goto_chain; > u32 tcfa_flags; > }; > diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h > index 449a63971451..096ea59a090b 100644 > --- a/include/uapi/linux/pkt_cls.h > +++ b/include/uapi/linux/pkt_cls.h > @@ -17,6 +17,7 @@ enum { > TCA_ACT_PAD, > TCA_ACT_COOKIE, > TCA_ACT_FLAGS, > + TCA_ACT_HW_STATS_TYPE, > __TCA_ACT_MAX > }; > > @@ -118,6 +119,31 @@ enum tca_id { > > #define TCA_ID_MAX __TCA_ID_MAX > > +/* tca HW stats type */ > +enum tca_act_hw_stats_type { > + TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default > + * when user does not pass the attr. > + * Instructs the driver that user does not > + * care if the HW stats are "immediate" > + * or "delayed". > + */ > + TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets > + * the current HW stats state from > + * the device queried at the dump time. > + */ > + TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets > + * HW stats that might be out of date > + * for some time, maybe couple of > + * seconds. This is the case when driver > + * polls stats updates periodically > + * or when it gets async stats update > + * from the device. > + */ > + TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting > + * any HW statistics. > + */ > +}; On the ABI I wonder if we can redefine it a little bit.. Can we make the stat types into a bitfield? On request: - no attr -> any stats allowed but some stats must be provided * - 0 -> no stats requested / disabled - 0x1 -> must be stat type0 - 0x6 -> stat type1 or stat type2 are both fine * no attr kinda doesn't work 'cause u32 offload has no stats and this is action-level now, not flower-level :S What about u32 and matchall? We can add a separate attribute with "active" stat types: - no attr -> old kernel - 0 -> no stats are provided / stats disabled - 0x1 -> only stat type0 is used by drivers - 0x6 -> at least one driver is using type1 and one type2 That assumes that we may one day add another stat type which would not be just based on the reporting time. If we only foresee time-based reporting would it make sense to turn the attribute into max acceptable delay in ms? 0 -> only immediate / blocking stats (0, MAX) -> given reporting delay in ms is acceptable MAX -> don't care about stats at all > struct tc_police { > __u32 index; > int action; > diff --git a/net/sched/act_api.c b/net/sched/act_api.c > index 8c466a712cda..d6468b09b932 100644 > --- a/net/sched/act_api.c > +++ b/net/sched/act_api.c > @@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act) > return nla_total_size(0) /* action number nested */ > + nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */ > + cookie_len /* TCA_ACT_COOKIE */ > + + nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */ > + nla_total_size(0) /* TCA_ACT_STATS nested */ > + nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */ > /* TCA_STATS_BASIC */ > @@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref) > } > rcu_read_unlock(); > > + if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type)) > + goto nla_put_failure; > + > if (a->tcfa_flags) { > struct nla_bitfield32 flags = { a->tcfa_flags, > a->tcfa_flags, }; > @@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb) > return c; > } > > +static inline enum tca_act_hw_stats_type static inline in C source > +tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr) > +{ > + /* If the user did not pass the attr, that means he does > + * not care about the type. Return "any" in that case. > + */ > + return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) : > + TCA_ACT_HW_STATS_TYPE_ANY; > +} > + > static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS; > static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { > [TCA_ACT_KIND] = { .type = NLA_STRING }, > [TCA_ACT_INDEX] = { .type = NLA_U32 }, > [TCA_ACT_COOKIE] = { .type = NLA_BINARY, > .len = TC_COOKIE_MAX_SIZE }, > + [TCA_ACT_HW_STATS_TYPE] = { .type = NLA_U8 }, We can use a POLICY with MIN/MAX here, perhaps? > [TCA_ACT_OPTIONS] = { .type = NLA_NESTED }, > [TCA_ACT_FLAGS] = { .type = NLA_BITFIELD32, > .validation_data = &tca_act_flags_allowed }, > @@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > bool rtnl_held, > struct netlink_ext_ack *extack) > { > + enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY; > struct nla_bitfield32 flags = { 0, 0 }; > struct tc_action *a; > struct tc_action_ops *a_o; > @@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > goto err_out; > } > } > + hw_stats_type = > + tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]); > if (tb[TCA_ACT_FLAGS]) > flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]); > } else { > @@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, > if (!name && tb[TCA_ACT_COOKIE]) > tcf_set_action_cookie(&a->act_cookie, cookie); > > + if (!name) > + a->hw_stats_type = hw_stats_type; > + > /* module count goes up only when brand new policy is created > * if it exists and is only bound to in a_o->init() then > * ACT_P_CREATED is not returned (a zero is). > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c > index 4e766c5ab77a..21bf37242153 100644 > --- a/net/sched/cls_api.c > +++ b/net/sched/cls_api.c > @@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry, > #endif > } > > +static inline enum flow_action_hw_stats_type static inline in C source > +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type) > +{ > + switch (hw_stats_type) { > + default: > + WARN_ON(1); > + /* fall-through */ without the policy change this seems user-triggerable > + case TCA_ACT_HW_STATS_TYPE_ANY: > + return FLOW_ACTION_HW_STATS_TYPE_ANY; > + case TCA_ACT_HW_STATS_TYPE_IMMEDIATE: > + return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE; > + case TCA_ACT_HW_STATS_TYPE_DELAYED: > + return FLOW_ACTION_HW_STATS_TYPE_DELAYED; > + case TCA_ACT_HW_STATS_TYPE_DISABLED: > + return FLOW_ACTION_HW_STATS_TYPE_DISABLED; > + } > +} > + > int tc_setup_flow_action(struct flow_action *flow_action, > const struct tcf_exts *exts) > { > + enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type); > struct tc_action *act; > int i, j, k, err = 0; > > @@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action, > err = tcf_act_get_cookie(entry, act); > if (err) > goto err_out_locked; > + > + entry->hw_stats_type = > + tcf_flow_action_hw_stats_type(act->hw_stats_type); > + if (i && last_hw_stats_type != entry->hw_stats_type) > + flow_action->mixed_hw_stats_types = true; > + last_hw_stats_type = entry->hw_stats_type; > + > if (is_tcf_gact_ok(act)) { > entry->id = FLOW_ACTION_ACCEPT; > } else if (is_tcf_gact_shot(act)) {
Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote: >On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote: >> From: Jiri Pirko <jiri@mellanox.com> >> >> Currently, user who is adding an action expects HW to report stats, >> however it does not have exact expectations about the stats types. >> That is aligned with TCA_ACT_HW_STATS_TYPE_ANY. >> >> Allow user to specify the type of HW stats for an action and require it. >> >> Pass the information down to flow_offload layer. >> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com> >> --- >> v1->v2: >> - moved the stats attr from cls_flower (filter) to any action >> - rebased on top of cookie offload changes >> - adjusted the patch description a bit > >Thanks, this looks good... I mean I wish we could just share actions >instead but this set is less objectionable than v1 :) You can still share actions, this patchset does not stop you from doing that. > >> diff --git a/include/net/act_api.h b/include/net/act_api.h >> index 71347a90a9d1..02b9bffa17ed 100644 >> --- a/include/net/act_api.h >> +++ b/include/net/act_api.h >> @@ -39,6 +39,7 @@ struct tc_action { >> struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw; >> struct gnet_stats_queue __percpu *cpu_qstats; >> struct tc_cookie __rcu *act_cookie; >> + enum tca_act_hw_stats_type hw_stats_type; >> struct tcf_chain __rcu *goto_chain; >> u32 tcfa_flags; >> }; >> diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h >> index 449a63971451..096ea59a090b 100644 >> --- a/include/uapi/linux/pkt_cls.h >> +++ b/include/uapi/linux/pkt_cls.h >> @@ -17,6 +17,7 @@ enum { >> TCA_ACT_PAD, >> TCA_ACT_COOKIE, >> TCA_ACT_FLAGS, >> + TCA_ACT_HW_STATS_TYPE, >> __TCA_ACT_MAX >> }; >> >> @@ -118,6 +119,31 @@ enum tca_id { >> >> #define TCA_ID_MAX __TCA_ID_MAX >> >> +/* tca HW stats type */ >> +enum tca_act_hw_stats_type { >> + TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default >> + * when user does not pass the attr. >> + * Instructs the driver that user does not >> + * care if the HW stats are "immediate" >> + * or "delayed". >> + */ >> + TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets >> + * the current HW stats state from >> + * the device queried at the dump time. >> + */ >> + TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets >> + * HW stats that might be out of date >> + * for some time, maybe couple of >> + * seconds. This is the case when driver >> + * polls stats updates periodically >> + * or when it gets async stats update >> + * from the device. >> + */ >> + TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting >> + * any HW statistics. >> + */ >> +}; > >On the ABI I wonder if we can redefine it a little bit.. > >Can we make the stat types into a bitfield? > >On request: > - no attr -> any stats allowed but some stats must be provided * > - 0 -> no stats requested / disabled > - 0x1 -> must be stat type0 > - 0x6 -> stat type1 or stat type2 are both fine I was thinking about this of course. On the write side, this is ok however, this is very tricky on read side. See below. > >* no attr kinda doesn't work 'cause u32 offload has no stats and this > is action-level now, not flower-level :S What about u32 and matchall? The fact that cls does not implement stats offloading is a lack of feature of the particular cls. > >We can add a separate attribute with "active" stat types: > - no attr -> old kernel > - 0 -> no stats are provided / stats disabled > - 0x1 -> only stat type0 is used by drivers > - 0x6 -> at least one driver is using type1 and one type2 There are 2 problems: 1) There is a mismatch between write and read. User might pass different value than it eventually gets from kernel. I guess this might be fine. 2) Much bigger problem is, that since the same action may be offloaded by multiple drivers, the read would have to provide an array of bitfields, each array item would represent one offloaded driver. That is why I decided for simple value instead of bitfield which is the same on write and read. > >That assumes that we may one day add another stat type which would >not be just based on the reporting time. > >If we only foresee time-based reporting would it make sense to turn >the attribute into max acceptable delay in ms? > >0 -> only immediate / blocking stats >(0, MAX) -> given reporting delay in ms is acceptable >MAX -> don't care about stats at all Interesting, is this "delayed" granularity something that has a usecase? It might turn into a guessing game between user and driver during action insertion :/ > >> struct tc_police { >> __u32 index; >> int action; >> diff --git a/net/sched/act_api.c b/net/sched/act_api.c >> index 8c466a712cda..d6468b09b932 100644 >> --- a/net/sched/act_api.c >> +++ b/net/sched/act_api.c >> @@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act) >> return nla_total_size(0) /* action number nested */ >> + nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */ >> + cookie_len /* TCA_ACT_COOKIE */ >> + + nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */ >> + nla_total_size(0) /* TCA_ACT_STATS nested */ >> + nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */ >> /* TCA_STATS_BASIC */ >> @@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref) >> } >> rcu_read_unlock(); >> >> + if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type)) >> + goto nla_put_failure; >> + >> if (a->tcfa_flags) { >> struct nla_bitfield32 flags = { a->tcfa_flags, >> a->tcfa_flags, }; >> @@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb) >> return c; >> } >> >> +static inline enum tca_act_hw_stats_type > >static inline in C source Ah, I moved from .h and forgot. Thanks. > >> +tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr) >> +{ >> + /* If the user did not pass the attr, that means he does >> + * not care about the type. Return "any" in that case. >> + */ >> + return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) : >> + TCA_ACT_HW_STATS_TYPE_ANY; >> +} >> + >> static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS; >> static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { >> [TCA_ACT_KIND] = { .type = NLA_STRING }, >> [TCA_ACT_INDEX] = { .type = NLA_U32 }, >> [TCA_ACT_COOKIE] = { .type = NLA_BINARY, >> .len = TC_COOKIE_MAX_SIZE }, >> + [TCA_ACT_HW_STATS_TYPE] = { .type = NLA_U8 }, > >We can use a POLICY with MIN/MAX here, perhaps? Ok. > >> [TCA_ACT_OPTIONS] = { .type = NLA_NESTED }, >> [TCA_ACT_FLAGS] = { .type = NLA_BITFIELD32, >> .validation_data = &tca_act_flags_allowed }, >> @@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, >> bool rtnl_held, >> struct netlink_ext_ack *extack) >> { >> + enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY; >> struct nla_bitfield32 flags = { 0, 0 }; >> struct tc_action *a; >> struct tc_action_ops *a_o; >> @@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, >> goto err_out; >> } >> } >> + hw_stats_type = >> + tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]); >> if (tb[TCA_ACT_FLAGS]) >> flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]); >> } else { >> @@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, >> if (!name && tb[TCA_ACT_COOKIE]) >> tcf_set_action_cookie(&a->act_cookie, cookie); >> >> + if (!name) >> + a->hw_stats_type = hw_stats_type; >> + >> /* module count goes up only when brand new policy is created >> * if it exists and is only bound to in a_o->init() then >> * ACT_P_CREATED is not returned (a zero is). >> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c >> index 4e766c5ab77a..21bf37242153 100644 >> --- a/net/sched/cls_api.c >> +++ b/net/sched/cls_api.c >> @@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry, >> #endif >> } >> >> +static inline enum flow_action_hw_stats_type > >static inline in C source Right. > >> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type) >> +{ >> + switch (hw_stats_type) { >> + default: >> + WARN_ON(1); >> + /* fall-through */ > >without the policy change this seems user-triggerable Nope. tcf_action_hw_stats_type_get() takes care of setting TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed. > >> + case TCA_ACT_HW_STATS_TYPE_ANY: >> + return FLOW_ACTION_HW_STATS_TYPE_ANY; >> + case TCA_ACT_HW_STATS_TYPE_IMMEDIATE: >> + return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE; >> + case TCA_ACT_HW_STATS_TYPE_DELAYED: >> + return FLOW_ACTION_HW_STATS_TYPE_DELAYED; >> + case TCA_ACT_HW_STATS_TYPE_DISABLED: >> + return FLOW_ACTION_HW_STATS_TYPE_DISABLED; >> + } >> +} >> + >> int tc_setup_flow_action(struct flow_action *flow_action, >> const struct tcf_exts *exts) >> { >> + enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type); >> struct tc_action *act; >> int i, j, k, err = 0; >> >> @@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action, >> err = tcf_act_get_cookie(entry, act); >> if (err) >> goto err_out_locked; >> + >> + entry->hw_stats_type = >> + tcf_flow_action_hw_stats_type(act->hw_stats_type); >> + if (i && last_hw_stats_type != entry->hw_stats_type) >> + flow_action->mixed_hw_stats_types = true; >> + last_hw_stats_type = entry->hw_stats_type; >> + >> if (is_tcf_gact_ok(act)) { >> entry->id = FLOW_ACTION_ACCEPT; >> } else if (is_tcf_gact_shot(act)) { >
On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote: > Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote: > >On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote: > >> From: Jiri Pirko <jiri@mellanox.com> > >> +/* tca HW stats type */ > >> +enum tca_act_hw_stats_type { > >> + TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default > >> + * when user does not pass the attr. > >> + * Instructs the driver that user does not > >> + * care if the HW stats are "immediate" > >> + * or "delayed". > >> + */ > >> + TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets > >> + * the current HW stats state from > >> + * the device queried at the dump time. > >> + */ > >> + TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets > >> + * HW stats that might be out of date > >> + * for some time, maybe couple of > >> + * seconds. This is the case when driver > >> + * polls stats updates periodically > >> + * or when it gets async stats update > >> + * from the device. > >> + */ > >> + TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting > >> + * any HW statistics. > >> + */ > >> +}; > > > >On the ABI I wonder if we can redefine it a little bit.. > > > >Can we make the stat types into a bitfield? > > > >On request: > > - no attr -> any stats allowed but some stats must be provided * > > - 0 -> no stats requested / disabled > > - 0x1 -> must be stat type0 > > - 0x6 -> stat type1 or stat type2 are both fine > > I was thinking about this of course. On the write side, this is ok > however, this is very tricky on read side. See below. > > >* no attr kinda doesn't work 'cause u32 offload has no stats and this > > is action-level now, not flower-level :S What about u32 and matchall? > > The fact that cls does not implement stats offloading is a lack of > feature of the particular cls. Yeah, I wonder how that squares with strict netlink parsing. > >We can add a separate attribute with "active" stat types: > > - no attr -> old kernel > > - 0 -> no stats are provided / stats disabled > > - 0x1 -> only stat type0 is used by drivers > > - 0x6 -> at least one driver is using type1 and one type2 > > There are 2 problems: > 1) There is a mismatch between write and read. User might pass different > value than it eventually gets from kernel. I guess this might be fine. Separate attribute would work. > 2) Much bigger problem is, that since the same action may be offloaded > by multiple drivers, the read would have to provide an array of > bitfields, each array item would represent one offloaded driver. That is > why I decided for simple value instead of bitfield which is the same on > write and read. Why an array? The counter itself is added up from all the drivers. If the value is a bitfield all drivers can just OR-in their type. > >That assumes that we may one day add another stat type which would > >not be just based on the reporting time. > > > >If we only foresee time-based reporting would it make sense to turn > >the attribute into max acceptable delay in ms? > > > >0 -> only immediate / blocking stats > >(0, MAX) -> given reporting delay in ms is acceptable > >MAX -> don't care about stats at all > > Interesting, is this "delayed" granularity something that has a usecase? > It might turn into a guessing game between user and driver during action > insertion :/ Yeah, I don't like the guessing part too, worst case refresh time may be system dependent. With just "DELAYED" I'm worried users will think the delay may be too long for OvS. Or simply poll the statistics more often than the HW reports them, which would be pointless. For the latter case I guess the best case refresh time is needed, while the former needs worst case. Hopefully the two are not too far apart. Maybe some day drivers may also tweak the refresh rate based on user requests to save PCIe bandwidth and CPU.. Anyway.. maybe its not worth it today. > >> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type) > >> +{ > >> + switch (hw_stats_type) { > >> + default: > >> + WARN_ON(1); > >> + /* fall-through */ > > > >without the policy change this seems user-triggerable > > Nope. tcf_action_hw_stats_type_get() takes care of setting > TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed. I meant attribute is present but carries a large value. > >> + case TCA_ACT_HW_STATS_TYPE_ANY: > >> + return FLOW_ACTION_HW_STATS_TYPE_ANY; > >> + case TCA_ACT_HW_STATS_TYPE_IMMEDIATE: > >> + return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE; > >> + case TCA_ACT_HW_STATS_TYPE_DELAYED: > >> + return FLOW_ACTION_HW_STATS_TYPE_DELAYED; > >> + case TCA_ACT_HW_STATS_TYPE_DISABLED: > >> + return FLOW_ACTION_HW_STATS_TYPE_DISABLED;
Sat, Feb 29, 2020 at 09:14:52PM CET, kuba@kernel.org wrote: >On Sat, 29 Feb 2020 08:52:09 +0100 Jiri Pirko wrote: >> Fri, Feb 28, 2020 at 08:59:23PM CET, kuba@kernel.org wrote: >> >On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote: >> >> From: Jiri Pirko <jiri@mellanox.com> >> >> +/* tca HW stats type */ >> >> +enum tca_act_hw_stats_type { >> >> + TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default >> >> + * when user does not pass the attr. >> >> + * Instructs the driver that user does not >> >> + * care if the HW stats are "immediate" >> >> + * or "delayed". >> >> + */ >> >> + TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets >> >> + * the current HW stats state from >> >> + * the device queried at the dump time. >> >> + */ >> >> + TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets >> >> + * HW stats that might be out of date >> >> + * for some time, maybe couple of >> >> + * seconds. This is the case when driver >> >> + * polls stats updates periodically >> >> + * or when it gets async stats update >> >> + * from the device. >> >> + */ >> >> + TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting >> >> + * any HW statistics. >> >> + */ >> >> +}; >> > >> >On the ABI I wonder if we can redefine it a little bit.. >> > >> >Can we make the stat types into a bitfield? >> > >> >On request: >> > - no attr -> any stats allowed but some stats must be provided * >> > - 0 -> no stats requested / disabled >> > - 0x1 -> must be stat type0 >> > - 0x6 -> stat type1 or stat type2 are both fine >> >> I was thinking about this of course. On the write side, this is ok >> however, this is very tricky on read side. See below. >> >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this >> > is action-level now, not flower-level :S What about u32 and matchall? >> >> The fact that cls does not implement stats offloading is a lack of >> feature of the particular cls. > >Yeah, I wonder how that squares with strict netlink parsing. > >> >We can add a separate attribute with "active" stat types: >> > - no attr -> old kernel >> > - 0 -> no stats are provided / stats disabled >> > - 0x1 -> only stat type0 is used by drivers >> > - 0x6 -> at least one driver is using type1 and one type2 >> >> There are 2 problems: >> 1) There is a mismatch between write and read. User might pass different >> value than it eventually gets from kernel. I guess this might be fine. > >Separate attribute would work. > >> 2) Much bigger problem is, that since the same action may be offloaded >> by multiple drivers, the read would have to provide an array of >> bitfields, each array item would represent one offloaded driver. That is >> why I decided for simple value instead of bitfield which is the same on >> write and read. > >Why an array? The counter itself is added up from all the drivers. >If the value is a bitfield all drivers can just OR-in their type. Yeah, for uapi. Internally the array would be still needed. Also the driver would need to somehow "write-back" the value to the offload caller and someone (caller/tc) would have to use the array to track these bitfields for individual callbacks (probably idr of some sort). I don't know, is this excercise worth it? Seems to me like we are overengineering this one a bit. Also there would be no "any" it would be type0|type1|type2 the user would have to pass. If new type appears, the userspace would have to be updated to do "any" again :/ This is inconvenient. > >> >That assumes that we may one day add another stat type which would >> >not be just based on the reporting time. >> > >> >If we only foresee time-based reporting would it make sense to turn >> >the attribute into max acceptable delay in ms? >> > >> >0 -> only immediate / blocking stats >> >(0, MAX) -> given reporting delay in ms is acceptable >> >MAX -> don't care about stats at all >> >> Interesting, is this "delayed" granularity something that has a usecase? >> It might turn into a guessing game between user and driver during action >> insertion :/ > >Yeah, I don't like the guessing part too, worst case refresh time may >be system dependent. > >With just "DELAYED" I'm worried users will think the delay may be too >long for OvS. Or simply poll the statistics more often than the HW >reports them, which would be pointless. > >For the latter case I guess the best case refresh time is needed, >while the former needs worst case. Hopefully the two are not too far >apart. > >Maybe some day drivers may also tweak the refresh rate based on user >requests to save PCIe bandwidth and CPU.. > >Anyway.. maybe its not worth it today. > >> >> +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type) >> >> +{ >> >> + switch (hw_stats_type) { >> >> + default: >> >> + WARN_ON(1); >> >> + /* fall-through */ >> > >> >without the policy change this seems user-triggerable >> >> Nope. tcf_action_hw_stats_type_get() takes care of setting >> TCA_ACT_HW_STATS_TYPE_ANY when no attr is passed. > >I meant attribute is present but carries a large value. Ah, will sanitize this. > >> >> + case TCA_ACT_HW_STATS_TYPE_ANY: >> >> + return FLOW_ACTION_HW_STATS_TYPE_ANY; >> >> + case TCA_ACT_HW_STATS_TYPE_IMMEDIATE: >> >> + return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE; >> >> + case TCA_ACT_HW_STATS_TYPE_DELAYED: >> >> + return FLOW_ACTION_HW_STATS_TYPE_DELAYED; >> >> + case TCA_ACT_HW_STATS_TYPE_DISABLED: >> >> + return FLOW_ACTION_HW_STATS_TYPE_DISABLED; >
On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote: > >> >On request: > >> > - no attr -> any stats allowed but some stats must be provided * > >> > - 0 -> no stats requested / disabled > >> > - 0x1 -> must be stat type0 > >> > - 0x6 -> stat type1 or stat type2 are both fine > >> > >> I was thinking about this of course. On the write side, this is ok > >> however, this is very tricky on read side. See below. > >> > >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this > >> > is action-level now, not flower-level :S What about u32 and matchall? > >> > >> The fact that cls does not implement stats offloading is a lack of > >> feature of the particular cls. > > > >Yeah, I wonder how that squares with strict netlink parsing. > > > >> >We can add a separate attribute with "active" stat types: > >> > - no attr -> old kernel > >> > - 0 -> no stats are provided / stats disabled > >> > - 0x1 -> only stat type0 is used by drivers > >> > - 0x6 -> at least one driver is using type1 and one type2 > >> > >> There are 2 problems: > >> 1) There is a mismatch between write and read. User might pass different > >> value than it eventually gets from kernel. I guess this might be fine. > > > >Separate attribute would work. > > > >> 2) Much bigger problem is, that since the same action may be offloaded > >> by multiple drivers, the read would have to provide an array of > >> bitfields, each array item would represent one offloaded driver. That is > >> why I decided for simple value instead of bitfield which is the same on > >> write and read. > > > >Why an array? The counter itself is added up from all the drivers. > >If the value is a bitfield all drivers can just OR-in their type. > > Yeah, for uapi. Internally the array would be still needed. Also the > driver would need to somehow "write-back" the value to the offload > caller and someone (caller/tc) would have to use the array to track > these bitfields for individual callbacks (probably idr of some sort). > I don't know, is this excercise worth it? I was thinking of just doing this on HW stats dump. Drivers which don't report stats by definition don't need to set any bit :) > Seems to me like we are overengineering this one a bit. That's possible, the reporting could be added later... I mostly wanted to have the discussion. > Also there would be no "any" it would be type0|type1|type2 the user > would have to pass. If new type appears, the userspace would have to be > updated to do "any" again :/ This is inconvenient. In my proposal above I was suggesting no attr to mean any. I think in your current code ANY already doesn't include disabled so old user space should not see any change.
Mon, Mar 02, 2020 at 08:39:33PM CET, kuba@kernel.org wrote: >On Sun, 1 Mar 2020 09:57:56 +0100 Jiri Pirko wrote: >> >> >On request: >> >> > - no attr -> any stats allowed but some stats must be provided * >> >> > - 0 -> no stats requested / disabled >> >> > - 0x1 -> must be stat type0 >> >> > - 0x6 -> stat type1 or stat type2 are both fine >> >> >> >> I was thinking about this of course. On the write side, this is ok >> >> however, this is very tricky on read side. See below. >> >> >> >> >* no attr kinda doesn't work 'cause u32 offload has no stats and this >> >> > is action-level now, not flower-level :S What about u32 and matchall? >> >> >> >> The fact that cls does not implement stats offloading is a lack of >> >> feature of the particular cls. >> > >> >Yeah, I wonder how that squares with strict netlink parsing. >> > >> >> >We can add a separate attribute with "active" stat types: >> >> > - no attr -> old kernel >> >> > - 0 -> no stats are provided / stats disabled >> >> > - 0x1 -> only stat type0 is used by drivers >> >> > - 0x6 -> at least one driver is using type1 and one type2 >> >> >> >> There are 2 problems: >> >> 1) There is a mismatch between write and read. User might pass different >> >> value than it eventually gets from kernel. I guess this might be fine. >> > >> >Separate attribute would work. >> > >> >> 2) Much bigger problem is, that since the same action may be offloaded >> >> by multiple drivers, the read would have to provide an array of >> >> bitfields, each array item would represent one offloaded driver. That is >> >> why I decided for simple value instead of bitfield which is the same on >> >> write and read. >> > >> >Why an array? The counter itself is added up from all the drivers. >> >If the value is a bitfield all drivers can just OR-in their type. >> >> Yeah, for uapi. Internally the array would be still needed. Also the >> driver would need to somehow "write-back" the value to the offload >> caller and someone (caller/tc) would have to use the array to track >> these bitfields for individual callbacks (probably idr of some sort). >> I don't know, is this excercise worth it? > >I was thinking of just doing this on HW stats dump. Drivers which don't >report stats by definition don't need to set any bit :) > >> Seems to me like we are overengineering this one a bit. > >That's possible, the reporting could be added later... I mostly wanted >to have the discussion. Okay. > >> Also there would be no "any" it would be type0|type1|type2 the user >> would have to pass. If new type appears, the userspace would have to be >> updated to do "any" again :/ This is inconvenient. > >In my proposal above I was suggesting no attr to mean any. I think in >your current code ANY already doesn't include disabled so old user >space should not see any change. Odd, no attribute meaning "any". I think it is polite to fillup the attribute for dump if kernel supports the attribute. However, here, we would not fill it up in case of "any". That is quite odd. We can have a bit that would mean "any" though. What do you think?
On Fri, 28 Feb 2020 18:25:05 +0100 Jiri Pirko wrote: > static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS; > static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { > [TCA_ACT_KIND] = { .type = NLA_STRING }, > [TCA_ACT_INDEX] = { .type = NLA_U32 }, > [TCA_ACT_COOKIE] = { .type = NLA_BINARY, > .len = TC_COOKIE_MAX_SIZE }, > + [TCA_ACT_HW_STATS_TYPE] = { .type = NLA_U8 }, > [TCA_ACT_OPTIONS] = { .type = NLA_NESTED }, > [TCA_ACT_FLAGS] = { .type = NLA_BITFIELD32, > .validation_data = &tca_act_flags_allowed }, Ah, we will probably also want to set the strict checking up for new attrs here.
On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote: > >> Also there would be no "any" it would be type0|type1|type2 the user > >> would have to pass. If new type appears, the userspace would have to be > >> updated to do "any" again :/ This is inconvenient. > > > >In my proposal above I was suggesting no attr to mean any. I think in > >your current code ANY already doesn't include disabled so old user > >space should not see any change. > > Odd, no attribute meaning "any". OTOH it does match up with old kernel behavior quite nicely, today there is no attribute and it means "any". > I think it is polite to fillup the attribute for dump if kernel > supports the attribute. However, here, we would not fill it up in > case of "any". That is quite odd. I see, it does seem nice to report the attribute, but again, won't the user space which wants to run on older kernels have to treat no attr as "any"? > We can have a bit that would mean "any" though. What do you think? It'd be a dead bit for the "stat types used" attribute, but I don't mind it if you prefer to go this way.
Tue, Mar 03, 2020 at 08:48:25PM CET, kuba@kernel.org wrote: >On Tue, 3 Mar 2020 14:20:35 +0100 Jiri Pirko wrote: >> >> Also there would be no "any" it would be type0|type1|type2 the user >> >> would have to pass. If new type appears, the userspace would have to be >> >> updated to do "any" again :/ This is inconvenient. >> > >> >In my proposal above I was suggesting no attr to mean any. I think in >> >your current code ANY already doesn't include disabled so old user >> >space should not see any change. >> >> Odd, no attribute meaning "any". > >OTOH it does match up with old kernel behavior quite nicely, today >there is no attribute and it means "any". > >> I think it is polite to fillup the attribute for dump if kernel >> supports the attribute. However, here, we would not fill it up in >> case of "any". That is quite odd. > >I see, it does seem nice to report the attribute, but again, won't >the user space which wants to run on older kernels have to treat >no attr as "any"? Okay. > >> We can have a bit that would mean "any" though. What do you think? > >It'd be a dead bit for the "stat types used" attribute, but I don't >mind it if you prefer to go this way.
diff --git a/include/net/act_api.h b/include/net/act_api.h index 71347a90a9d1..02b9bffa17ed 100644 --- a/include/net/act_api.h +++ b/include/net/act_api.h @@ -39,6 +39,7 @@ struct tc_action { struct gnet_stats_basic_cpu __percpu *cpu_bstats_hw; struct gnet_stats_queue __percpu *cpu_qstats; struct tc_cookie __rcu *act_cookie; + enum tca_act_hw_stats_type hw_stats_type; struct tcf_chain __rcu *goto_chain; u32 tcfa_flags; }; diff --git a/include/uapi/linux/pkt_cls.h b/include/uapi/linux/pkt_cls.h index 449a63971451..096ea59a090b 100644 --- a/include/uapi/linux/pkt_cls.h +++ b/include/uapi/linux/pkt_cls.h @@ -17,6 +17,7 @@ enum { TCA_ACT_PAD, TCA_ACT_COOKIE, TCA_ACT_FLAGS, + TCA_ACT_HW_STATS_TYPE, __TCA_ACT_MAX }; @@ -118,6 +119,31 @@ enum tca_id { #define TCA_ID_MAX __TCA_ID_MAX +/* tca HW stats type */ +enum tca_act_hw_stats_type { + TCA_ACT_HW_STATS_TYPE_ANY, /* User does not care, it's default + * when user does not pass the attr. + * Instructs the driver that user does not + * care if the HW stats are "immediate" + * or "delayed". + */ + TCA_ACT_HW_STATS_TYPE_IMMEDIATE, /* Means that in dump, user gets + * the current HW stats state from + * the device queried at the dump time. + */ + TCA_ACT_HW_STATS_TYPE_DELAYED, /* Means that in dump, user gets + * HW stats that might be out of date + * for some time, maybe couple of + * seconds. This is the case when driver + * polls stats updates periodically + * or when it gets async stats update + * from the device. + */ + TCA_ACT_HW_STATS_TYPE_DISABLED, /* User is not interested in getting + * any HW statistics. + */ +}; + struct tc_police { __u32 index; int action; diff --git a/net/sched/act_api.c b/net/sched/act_api.c index 8c466a712cda..d6468b09b932 100644 --- a/net/sched/act_api.c +++ b/net/sched/act_api.c @@ -185,6 +185,7 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act) return nla_total_size(0) /* action number nested */ + nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */ + cookie_len /* TCA_ACT_COOKIE */ + + nla_total_size(sizeof(u8)) /* TCA_ACT_HW_STATS_TYPE */ + nla_total_size(0) /* TCA_ACT_STATS nested */ + nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */ /* TCA_STATS_BASIC */ @@ -788,6 +789,9 @@ tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int bind, int ref) } rcu_read_unlock(); + if (nla_put_u8(skb, TCA_ACT_HW_STATS_TYPE, a->hw_stats_type)) + goto nla_put_failure; + if (a->tcfa_flags) { struct nla_bitfield32 flags = { a->tcfa_flags, a->tcfa_flags, }; @@ -854,12 +858,23 @@ static struct tc_cookie *nla_memdup_cookie(struct nlattr **tb) return c; } +static inline enum tca_act_hw_stats_type +tcf_action_hw_stats_type_get(struct nlattr *hw_stats_type_attr) +{ + /* If the user did not pass the attr, that means he does + * not care about the type. Return "any" in that case. + */ + return hw_stats_type_attr ? nla_get_u8(hw_stats_type_attr) : + TCA_ACT_HW_STATS_TYPE_ANY; +} + static const u32 tca_act_flags_allowed = TCA_ACT_FLAGS_NO_PERCPU_STATS; static const struct nla_policy tcf_action_policy[TCA_ACT_MAX + 1] = { [TCA_ACT_KIND] = { .type = NLA_STRING }, [TCA_ACT_INDEX] = { .type = NLA_U32 }, [TCA_ACT_COOKIE] = { .type = NLA_BINARY, .len = TC_COOKIE_MAX_SIZE }, + [TCA_ACT_HW_STATS_TYPE] = { .type = NLA_U8 }, [TCA_ACT_OPTIONS] = { .type = NLA_NESTED }, [TCA_ACT_FLAGS] = { .type = NLA_BITFIELD32, .validation_data = &tca_act_flags_allowed }, @@ -871,6 +886,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, bool rtnl_held, struct netlink_ext_ack *extack) { + enum tca_act_hw_stats_type hw_stats_type = TCA_ACT_HW_STATS_TYPE_ANY; struct nla_bitfield32 flags = { 0, 0 }; struct tc_action *a; struct tc_action_ops *a_o; @@ -903,6 +919,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, goto err_out; } } + hw_stats_type = + tcf_action_hw_stats_type_get(tb[TCA_ACT_HW_STATS_TYPE]); if (tb[TCA_ACT_FLAGS]) flags = nla_get_bitfield32(tb[TCA_ACT_FLAGS]); } else { @@ -953,6 +971,9 @@ struct tc_action *tcf_action_init_1(struct net *net, struct tcf_proto *tp, if (!name && tb[TCA_ACT_COOKIE]) tcf_set_action_cookie(&a->act_cookie, cookie); + if (!name) + a->hw_stats_type = hw_stats_type; + /* module count goes up only when brand new policy is created * if it exists and is only bound to in a_o->init() then * ACT_P_CREATED is not returned (a zero is). diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c index 4e766c5ab77a..21bf37242153 100644 --- a/net/sched/cls_api.c +++ b/net/sched/cls_api.c @@ -3458,9 +3458,28 @@ static void tcf_sample_get_group(struct flow_action_entry *entry, #endif } +static inline enum flow_action_hw_stats_type +tcf_flow_action_hw_stats_type(enum tca_act_hw_stats_type hw_stats_type) +{ + switch (hw_stats_type) { + default: + WARN_ON(1); + /* fall-through */ + case TCA_ACT_HW_STATS_TYPE_ANY: + return FLOW_ACTION_HW_STATS_TYPE_ANY; + case TCA_ACT_HW_STATS_TYPE_IMMEDIATE: + return FLOW_ACTION_HW_STATS_TYPE_IMMEDIATE; + case TCA_ACT_HW_STATS_TYPE_DELAYED: + return FLOW_ACTION_HW_STATS_TYPE_DELAYED; + case TCA_ACT_HW_STATS_TYPE_DISABLED: + return FLOW_ACTION_HW_STATS_TYPE_DISABLED; + } +} + int tc_setup_flow_action(struct flow_action *flow_action, const struct tcf_exts *exts) { + enum flow_action_hw_stats_type uninitialized_var(last_hw_stats_type); struct tc_action *act; int i, j, k, err = 0; @@ -3476,6 +3495,13 @@ int tc_setup_flow_action(struct flow_action *flow_action, err = tcf_act_get_cookie(entry, act); if (err) goto err_out_locked; + + entry->hw_stats_type = + tcf_flow_action_hw_stats_type(act->hw_stats_type); + if (i && last_hw_stats_type != entry->hw_stats_type) + flow_action->mixed_hw_stats_types = true; + last_hw_stats_type = entry->hw_stats_type; + if (is_tcf_gact_ok(act)) { entry->id = FLOW_ACTION_ACCEPT; } else if (is_tcf_gact_shot(act)) {