| Message ID | 164329050340.2583748.1716649852891765076.stgit@ebuild |
|---|---|
| State | Superseded |
| Headers | show |
| Series | netdev-offload-tc: Fix various tc-offload related problems | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 2022-01-27 3:35 PM, Eelco Chaudron wrote: > Statistics should be captured on the first action in a list > of actions, to do this we need to also capture the last update > time stamp from the first action. Why stats should be captured on the first action? this patch breaks some offloading. like tunnel rules. On tc hw offloaded rules stats are being updated only on actions implementing stats_update() cb. Some actions, like tunnel_key, don't implement this and stats are kept 0 but they are not usually the last action. last actions like drop, goto, mirred do implement this and have the stats. > > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > --- > lib/tc.c | 119 +++++++++++++++++++++++++++++++++++++++++++++----------------- > 1 file changed, 86 insertions(+), 33 deletions(-) > > diff --git a/lib/tc.c b/lib/tc.c > index b37dce22f..b430f1bad 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -994,12 +994,35 @@ nl_parse_flower_flags(struct nlattr **attrs, struct tc_flower *flower) > flower->offloaded_state = nl_get_flower_offloaded_state(attrs); > } > > +static int > +get_user_hz(void) > +{ > + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > + static int user_hz = 100; > + > + if (ovsthread_once_start(&once)) { > + user_hz = sysconf(_SC_CLK_TCK); > + ovsthread_once_done(&once); > + } > + > + return user_hz; > +} > + > +static void > +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) > +{ > + flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); > +} > + > static const struct nl_policy pedit_policy[] = { > - [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, > - .min_len = sizeof(struct tc_pedit), > - .optional = false, }, > - [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, > - .optional = false, }, > + [TCA_PEDIT_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > + [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tc_pedit), > + .optional = false, }, > + [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, > + .optional = false, }, > }; > > static int > @@ -1094,6 +1117,9 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower) > action = &flower->actions[flower->action_count++]; > action->type = TC_ACT_PEDIT; > > + nl_parse_tcf(nl_attr_get_unspec(pe_attrs[TCA_PEDIT_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1101,6 +1127,9 @@ static const struct nl_policy tunnel_key_policy[] = { > [TCA_TUNNEL_KEY_PARMS] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct tc_tunnel_key), > .optional = false, }, > + [TCA_TUNNEL_KEY_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > [TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NL_A_U32, .optional = true, }, > [TCA_TUNNEL_KEY_ENC_IPV4_DST] = { .type = NL_A_U32, .optional = true, }, > [TCA_TUNNEL_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC, > @@ -1275,6 +1304,10 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower) > tun->action, tun->t_action); > return EINVAL; > } > + > + nl_parse_tcf(nl_attr_get_unspec(tun_attrs[TCA_TUNNEL_KEY_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1287,26 +1320,6 @@ static const struct nl_policy gact_policy[] = { > .optional = false, }, > }; > > -static int > -get_user_hz(void) > -{ > - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; > - static int user_hz = 100; > - > - if (ovsthread_once_start(&once)) { > - user_hz = sysconf(_SC_CLK_TCK); > - ovsthread_once_done(&once); > - } > - > - return user_hz; > -} > - > -static void > -nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) > -{ > - flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); > -} > - > static int > nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) > { > @@ -1394,18 +1407,21 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) > > static const struct nl_policy ct_policy[] = { > [TCA_CT_PARMS] = { .type = NL_A_UNSPEC, > - .min_len = sizeof(struct tc_ct), > - .optional = false, }, > + .min_len = sizeof(struct tc_ct), > + .optional = false, }, > + [TCA_CT_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > [TCA_CT_ACTION] = { .type = NL_A_U16, > - .optional = true, }, > + .optional = true, }, > [TCA_CT_ZONE] = { .type = NL_A_U16, > .optional = true, }, > [TCA_CT_MARK] = { .type = NL_A_U32, > .optional = true, }, > [TCA_CT_MARK_MASK] = { .type = NL_A_U32, > - .optional = true, }, > + .optional = true, }, > [TCA_CT_LABELS] = { .type = NL_A_UNSPEC, > - .optional = true, }, > + .optional = true, }, > [TCA_CT_LABELS_MASK] = { .type = NL_A_UNSPEC, > .optional = true, }, > [TCA_CT_NAT_IPV4_MIN] = { .type = NL_A_U32, > @@ -1517,6 +1533,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower *flower) > } > action->type = TC_ACT_CT; > > + nl_parse_tcf(nl_attr_get_unspec(ct_attrs[TCA_CT_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1524,6 +1543,9 @@ static const struct nl_policy vlan_policy[] = { > [TCA_VLAN_PARMS] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct tc_vlan), > .optional = false, }, > + [TCA_VLAN_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > [TCA_VLAN_PUSH_VLAN_ID] = { .type = NL_A_U16, .optional = true, }, > [TCA_VLAN_PUSH_VLAN_PROTOCOL] = { .type = NL_A_U16, .optional = true, }, > [TCA_VLAN_PUSH_VLAN_PRIORITY] = { .type = NL_A_U8, .optional = true, }, > @@ -1562,6 +1584,10 @@ nl_parse_act_vlan(struct nlattr *options, struct tc_flower *flower) > v->action, v->v_action); > return EINVAL; > } > + > + nl_parse_tcf(nl_attr_get_unspec(vlan_attrs[TCA_VLAN_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1569,6 +1595,9 @@ static const struct nl_policy mpls_policy[] = { > [TCA_MPLS_PARMS] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct tc_mpls), > .optional = false, }, > + [TCA_MPLS_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > [TCA_MPLS_PROTO] = { .type = NL_A_U16, .optional = true, }, > [TCA_MPLS_LABEL] = { .type = NL_A_U32, .optional = true, }, > [TCA_MPLS_TC] = { .type = NL_A_U8, .optional = true, }, > @@ -1655,6 +1684,9 @@ nl_parse_act_mpls(struct nlattr *options, struct tc_flower *flower) > return EINVAL; > } > > + nl_parse_tcf(nl_attr_get_unspec(mpls_attrs[TCA_MPLS_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1662,6 +1694,9 @@ static const struct nl_policy csum_policy[] = { > [TCA_CSUM_PARMS] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct tc_csum), > .optional = false, }, > + [TCA_CSUM_TM] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct tcf_t), > + .optional = false, }, > }; > > static int > @@ -1695,6 +1730,9 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower) > return EINVAL; > } > > + nl_parse_tcf(nl_attr_get_unspec(csum_attrs[TCA_CSUM_TM], > + sizeof(struct tcf_t)), > + flower); > return 0; > } > > @@ -1713,7 +1751,7 @@ static const struct nl_policy stats_policy[] = { > > static int > nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > - bool terse) > + bool terse, bool update_stats) > { > struct nlattr *act_options; > struct nlattr *act_stats; > @@ -1779,7 +1817,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > } > > bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); > - if (bs->packets) { > + if (bs->packets && update_stats) { > put_32aligned_u64(&stats->n_packets, bs->packets); > put_32aligned_u64(&stats->n_bytes, bs->bytes); > } > @@ -1797,6 +1835,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, > static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; > struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; > const int max_size = ARRAY_SIZE(actions_orders_policy); > + bool first_action = true; > + uint64_t lastused = 0; > > for (int i = TCA_ACT_MIN_PRIO; i < max_size; i++) { > actions_orders_policy[i].type = NL_A_NESTED; > @@ -1817,14 +1857,27 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, > VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); > return EOPNOTSUPP; > } > - err = nl_parse_single_action(actions_orders[i], flower, terse); > + err = nl_parse_single_action(actions_orders[i], flower, terse, > + first_action); > > if (err) { > return err; > } > + > + if (first_action) { > + /* We need to store the last used timestamp from the first > + * action, i.e., the action that was used the track the > + * statistics, as all remaining action decoding overwrite > + * this value. We also need to use the first actions statistics > + * as conditional actions can exist. */ > + lastused = flower->lastused; > + first_action = false; > + } > } > } > > + flower->lastused = lastused; > + > if (flower->csum_update_flags) { > VLOG_WARN_RL(&error_rl, > "expected act csum with flags: 0x%x", > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 30 Jan 2022, at 14:30, Roi Dayan wrote: > On 2022-01-27 3:35 PM, Eelco Chaudron wrote: >> Statistics should be captured on the first action in a list >> of actions, to do this we need to also capture the last update >> time stamp from the first action. > > Why stats should be captured on the first action? > > this patch breaks some offloading. like tunnel rules. > > On tc hw offloaded rules stats > are being updated only on actions implementing stats_update() cb. > Some actions, like tunnel_key, don't implement this and stats are > kept 0 but they are not usually the last action. > last actions like drop, goto, mirred do implement this and have > the stats. Thanks, I added this patch as it was part of a debug effort. So I’ll remove it from this patch set (and add a new one I found on Friday ;). Some background info on why I kept this in the first place… In general, OVS needs stats on the match, not the actions. So with the current code/implementations available, the last action would do, as OVS will execute all actions. As you know we are looking at the check_pkt_len action for OVS, in which case a different set of actions could be executed, rendering the statistics on the last action taken being wrong as there are two potential different paths. But I’ll figure out how to solve that when I get there :) Thanks for the review, and I’ll send out a V2 soon! //Eelco >> >> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >> --- >> lib/tc.c | 119 +++++++++++++++++++++++++++++++++++++++++++++----------------- >> 1 file changed, 86 insertions(+), 33 deletions(-) >> >> diff --git a/lib/tc.c b/lib/tc.c >> index b37dce22f..b430f1bad 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -994,12 +994,35 @@ nl_parse_flower_flags(struct nlattr **attrs, struct tc_flower *flower) >> flower->offloaded_state = nl_get_flower_offloaded_state(attrs); >> } >> +static int >> +get_user_hz(void) >> +{ >> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> + static int user_hz = 100; >> + >> + if (ovsthread_once_start(&once)) { >> + user_hz = sysconf(_SC_CLK_TCK); >> + ovsthread_once_done(&once); >> + } >> + >> + return user_hz; >> +} >> + >> +static void >> +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >> +{ >> + flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >> +} >> + >> static const struct nl_policy pedit_policy[] = { >> - [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, >> - .min_len = sizeof(struct tc_pedit), >> - .optional = false, }, >> - [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, >> - .optional = false, }, >> + [TCA_PEDIT_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> + [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tc_pedit), >> + .optional = false, }, >> + [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, >> + .optional = false, }, >> }; >> static int >> @@ -1094,6 +1117,9 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower) >> action = &flower->actions[flower->action_count++]; >> action->type = TC_ACT_PEDIT; >> + nl_parse_tcf(nl_attr_get_unspec(pe_attrs[TCA_PEDIT_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1101,6 +1127,9 @@ static const struct nl_policy tunnel_key_policy[] = { >> [TCA_TUNNEL_KEY_PARMS] = { .type = NL_A_UNSPEC, >> .min_len = sizeof(struct tc_tunnel_key), >> .optional = false, }, >> + [TCA_TUNNEL_KEY_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> [TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NL_A_U32, .optional = true, }, >> [TCA_TUNNEL_KEY_ENC_IPV4_DST] = { .type = NL_A_U32, .optional = true, }, >> [TCA_TUNNEL_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC, >> @@ -1275,6 +1304,10 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower) >> tun->action, tun->t_action); >> return EINVAL; >> } >> + >> + nl_parse_tcf(nl_attr_get_unspec(tun_attrs[TCA_TUNNEL_KEY_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1287,26 +1320,6 @@ static const struct nl_policy gact_policy[] = { >> .optional = false, }, >> }; >> -static int >> -get_user_hz(void) >> -{ >> - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >> - static int user_hz = 100; >> - >> - if (ovsthread_once_start(&once)) { >> - user_hz = sysconf(_SC_CLK_TCK); >> - ovsthread_once_done(&once); >> - } >> - >> - return user_hz; >> -} >> - >> -static void >> -nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >> -{ >> - flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >> -} >> - >> static int >> nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) >> { >> @@ -1394,18 +1407,21 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) >> static const struct nl_policy ct_policy[] = { >> [TCA_CT_PARMS] = { .type = NL_A_UNSPEC, >> - .min_len = sizeof(struct tc_ct), >> - .optional = false, }, >> + .min_len = sizeof(struct tc_ct), >> + .optional = false, }, >> + [TCA_CT_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> [TCA_CT_ACTION] = { .type = NL_A_U16, >> - .optional = true, }, >> + .optional = true, }, >> [TCA_CT_ZONE] = { .type = NL_A_U16, >> .optional = true, }, >> [TCA_CT_MARK] = { .type = NL_A_U32, >> .optional = true, }, >> [TCA_CT_MARK_MASK] = { .type = NL_A_U32, >> - .optional = true, }, >> + .optional = true, }, >> [TCA_CT_LABELS] = { .type = NL_A_UNSPEC, >> - .optional = true, }, >> + .optional = true, }, >> [TCA_CT_LABELS_MASK] = { .type = NL_A_UNSPEC, >> .optional = true, }, >> [TCA_CT_NAT_IPV4_MIN] = { .type = NL_A_U32, >> @@ -1517,6 +1533,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower *flower) >> } >> action->type = TC_ACT_CT; >> + nl_parse_tcf(nl_attr_get_unspec(ct_attrs[TCA_CT_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1524,6 +1543,9 @@ static const struct nl_policy vlan_policy[] = { >> [TCA_VLAN_PARMS] = { .type = NL_A_UNSPEC, >> .min_len = sizeof(struct tc_vlan), >> .optional = false, }, >> + [TCA_VLAN_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> [TCA_VLAN_PUSH_VLAN_ID] = { .type = NL_A_U16, .optional = true, }, >> [TCA_VLAN_PUSH_VLAN_PROTOCOL] = { .type = NL_A_U16, .optional = true, }, >> [TCA_VLAN_PUSH_VLAN_PRIORITY] = { .type = NL_A_U8, .optional = true, }, >> @@ -1562,6 +1584,10 @@ nl_parse_act_vlan(struct nlattr *options, struct tc_flower *flower) >> v->action, v->v_action); >> return EINVAL; >> } >> + >> + nl_parse_tcf(nl_attr_get_unspec(vlan_attrs[TCA_VLAN_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1569,6 +1595,9 @@ static const struct nl_policy mpls_policy[] = { >> [TCA_MPLS_PARMS] = { .type = NL_A_UNSPEC, >> .min_len = sizeof(struct tc_mpls), >> .optional = false, }, >> + [TCA_MPLS_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> [TCA_MPLS_PROTO] = { .type = NL_A_U16, .optional = true, }, >> [TCA_MPLS_LABEL] = { .type = NL_A_U32, .optional = true, }, >> [TCA_MPLS_TC] = { .type = NL_A_U8, .optional = true, }, >> @@ -1655,6 +1684,9 @@ nl_parse_act_mpls(struct nlattr *options, struct tc_flower *flower) >> return EINVAL; >> } >> + nl_parse_tcf(nl_attr_get_unspec(mpls_attrs[TCA_MPLS_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1662,6 +1694,9 @@ static const struct nl_policy csum_policy[] = { >> [TCA_CSUM_PARMS] = { .type = NL_A_UNSPEC, >> .min_len = sizeof(struct tc_csum), >> .optional = false, }, >> + [TCA_CSUM_TM] = { .type = NL_A_UNSPEC, >> + .min_len = sizeof(struct tcf_t), >> + .optional = false, }, >> }; >> static int >> @@ -1695,6 +1730,9 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower) >> return EINVAL; >> } >> + nl_parse_tcf(nl_attr_get_unspec(csum_attrs[TCA_CSUM_TM], >> + sizeof(struct tcf_t)), >> + flower); >> return 0; >> } >> @@ -1713,7 +1751,7 @@ static const struct nl_policy stats_policy[] = { >> static int >> nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, >> - bool terse) >> + bool terse, bool update_stats) >> { >> struct nlattr *act_options; >> struct nlattr *act_stats; >> @@ -1779,7 +1817,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, >> } >> bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); >> - if (bs->packets) { >> + if (bs->packets && update_stats) { >> put_32aligned_u64(&stats->n_packets, bs->packets); >> put_32aligned_u64(&stats->n_bytes, bs->bytes); >> } >> @@ -1797,6 +1835,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, >> static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; >> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; >> const int max_size = ARRAY_SIZE(actions_orders_policy); >> + bool first_action = true; >> + uint64_t lastused = 0; >> for (int i = TCA_ACT_MIN_PRIO; i < max_size; i++) { >> actions_orders_policy[i].type = NL_A_NESTED; >> @@ -1817,14 +1857,27 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, >> VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >> return EOPNOTSUPP; >> } >> - err = nl_parse_single_action(actions_orders[i], flower, terse); >> + err = nl_parse_single_action(actions_orders[i], flower, terse, >> + first_action); >> if (err) { >> return err; >> } >> + >> + if (first_action) { >> + /* We need to store the last used timestamp from the first >> + * action, i.e., the action that was used the track the >> + * statistics, as all remaining action decoding overwrite >> + * this value. We also need to use the first actions statistics >> + * as conditional actions can exist. */ >> + lastused = flower->lastused; >> + first_action = false; >> + } >> } >> } >> + flower->lastused = lastused; >> + >> if (flower->csum_update_flags) { >> VLOG_WARN_RL(&error_rl, >> "expected act csum with flags: 0x%x", >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 2022-01-31 12:16 PM, Eelco Chaudron wrote: > > > On 30 Jan 2022, at 14:30, Roi Dayan wrote: > >> On 2022-01-27 3:35 PM, Eelco Chaudron wrote: >>> Statistics should be captured on the first action in a list >>> of actions, to do this we need to also capture the last update >>> time stamp from the first action. >> >> Why stats should be captured on the first action? >> >> this patch breaks some offloading. like tunnel rules. >> >> On tc hw offloaded rules stats >> are being updated only on actions implementing stats_update() cb. >> Some actions, like tunnel_key, don't implement this and stats are >> kept 0 but they are not usually the last action. >> last actions like drop, goto, mirred do implement this and have >> the stats. > > Thanks, I added this patch as it was part of a debug effort. So I’ll remove it from this patch set (and add a new one I found on Friday ;). > > Some background info on why I kept this in the first place… In general, OVS needs stats on the match, not the actions. So with the current code/implementations available, the last action would do, as OVS will execute all actions. > > As you know we are looking at the check_pkt_len action for OVS, in which case a different set of actions could be executed, rendering the statistics on the last action taken being wrong as there are two potential different paths. But I’ll figure out how to solve that when I get there :) > > Thanks for the review, and I’ll send out a V2 soon! > > //Eelco great thanks. > >>> >>> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> >>> --- >>> lib/tc.c | 119 +++++++++++++++++++++++++++++++++++++++++++++----------------- >>> 1 file changed, 86 insertions(+), 33 deletions(-) >>> >>> diff --git a/lib/tc.c b/lib/tc.c >>> index b37dce22f..b430f1bad 100644 >>> --- a/lib/tc.c >>> +++ b/lib/tc.c >>> @@ -994,12 +994,35 @@ nl_parse_flower_flags(struct nlattr **attrs, struct tc_flower *flower) >>> flower->offloaded_state = nl_get_flower_offloaded_state(attrs); >>> } >>> +static int >>> +get_user_hz(void) >>> +{ >>> + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>> + static int user_hz = 100; >>> + >>> + if (ovsthread_once_start(&once)) { >>> + user_hz = sysconf(_SC_CLK_TCK); >>> + ovsthread_once_done(&once); >>> + } >>> + >>> + return user_hz; >>> +} >>> + >>> +static void >>> +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >>> +{ >>> + flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >>> +} >>> + >>> static const struct nl_policy pedit_policy[] = { >>> - [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, >>> - .min_len = sizeof(struct tc_pedit), >>> - .optional = false, }, >>> - [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, >>> - .optional = false, }, >>> + [TCA_PEDIT_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> + [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tc_pedit), >>> + .optional = false, }, >>> + [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, >>> + .optional = false, }, >>> }; >>> static int >>> @@ -1094,6 +1117,9 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower) >>> action = &flower->actions[flower->action_count++]; >>> action->type = TC_ACT_PEDIT; >>> + nl_parse_tcf(nl_attr_get_unspec(pe_attrs[TCA_PEDIT_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1101,6 +1127,9 @@ static const struct nl_policy tunnel_key_policy[] = { >>> [TCA_TUNNEL_KEY_PARMS] = { .type = NL_A_UNSPEC, >>> .min_len = sizeof(struct tc_tunnel_key), >>> .optional = false, }, >>> + [TCA_TUNNEL_KEY_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> [TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NL_A_U32, .optional = true, }, >>> [TCA_TUNNEL_KEY_ENC_IPV4_DST] = { .type = NL_A_U32, .optional = true, }, >>> [TCA_TUNNEL_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC, >>> @@ -1275,6 +1304,10 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower) >>> tun->action, tun->t_action); >>> return EINVAL; >>> } >>> + >>> + nl_parse_tcf(nl_attr_get_unspec(tun_attrs[TCA_TUNNEL_KEY_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1287,26 +1320,6 @@ static const struct nl_policy gact_policy[] = { >>> .optional = false, }, >>> }; >>> -static int >>> -get_user_hz(void) >>> -{ >>> - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; >>> - static int user_hz = 100; >>> - >>> - if (ovsthread_once_start(&once)) { >>> - user_hz = sysconf(_SC_CLK_TCK); >>> - ovsthread_once_done(&once); >>> - } >>> - >>> - return user_hz; >>> -} >>> - >>> -static void >>> -nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) >>> -{ >>> - flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); >>> -} >>> - >>> static int >>> nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) >>> { >>> @@ -1394,18 +1407,21 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) >>> static const struct nl_policy ct_policy[] = { >>> [TCA_CT_PARMS] = { .type = NL_A_UNSPEC, >>> - .min_len = sizeof(struct tc_ct), >>> - .optional = false, }, >>> + .min_len = sizeof(struct tc_ct), >>> + .optional = false, }, >>> + [TCA_CT_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> [TCA_CT_ACTION] = { .type = NL_A_U16, >>> - .optional = true, }, >>> + .optional = true, }, >>> [TCA_CT_ZONE] = { .type = NL_A_U16, >>> .optional = true, }, >>> [TCA_CT_MARK] = { .type = NL_A_U32, >>> .optional = true, }, >>> [TCA_CT_MARK_MASK] = { .type = NL_A_U32, >>> - .optional = true, }, >>> + .optional = true, }, >>> [TCA_CT_LABELS] = { .type = NL_A_UNSPEC, >>> - .optional = true, }, >>> + .optional = true, }, >>> [TCA_CT_LABELS_MASK] = { .type = NL_A_UNSPEC, >>> .optional = true, }, >>> [TCA_CT_NAT_IPV4_MIN] = { .type = NL_A_U32, >>> @@ -1517,6 +1533,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower *flower) >>> } >>> action->type = TC_ACT_CT; >>> + nl_parse_tcf(nl_attr_get_unspec(ct_attrs[TCA_CT_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1524,6 +1543,9 @@ static const struct nl_policy vlan_policy[] = { >>> [TCA_VLAN_PARMS] = { .type = NL_A_UNSPEC, >>> .min_len = sizeof(struct tc_vlan), >>> .optional = false, }, >>> + [TCA_VLAN_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> [TCA_VLAN_PUSH_VLAN_ID] = { .type = NL_A_U16, .optional = true, }, >>> [TCA_VLAN_PUSH_VLAN_PROTOCOL] = { .type = NL_A_U16, .optional = true, }, >>> [TCA_VLAN_PUSH_VLAN_PRIORITY] = { .type = NL_A_U8, .optional = true, }, >>> @@ -1562,6 +1584,10 @@ nl_parse_act_vlan(struct nlattr *options, struct tc_flower *flower) >>> v->action, v->v_action); >>> return EINVAL; >>> } >>> + >>> + nl_parse_tcf(nl_attr_get_unspec(vlan_attrs[TCA_VLAN_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1569,6 +1595,9 @@ static const struct nl_policy mpls_policy[] = { >>> [TCA_MPLS_PARMS] = { .type = NL_A_UNSPEC, >>> .min_len = sizeof(struct tc_mpls), >>> .optional = false, }, >>> + [TCA_MPLS_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> [TCA_MPLS_PROTO] = { .type = NL_A_U16, .optional = true, }, >>> [TCA_MPLS_LABEL] = { .type = NL_A_U32, .optional = true, }, >>> [TCA_MPLS_TC] = { .type = NL_A_U8, .optional = true, }, >>> @@ -1655,6 +1684,9 @@ nl_parse_act_mpls(struct nlattr *options, struct tc_flower *flower) >>> return EINVAL; >>> } >>> + nl_parse_tcf(nl_attr_get_unspec(mpls_attrs[TCA_MPLS_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1662,6 +1694,9 @@ static const struct nl_policy csum_policy[] = { >>> [TCA_CSUM_PARMS] = { .type = NL_A_UNSPEC, >>> .min_len = sizeof(struct tc_csum), >>> .optional = false, }, >>> + [TCA_CSUM_TM] = { .type = NL_A_UNSPEC, >>> + .min_len = sizeof(struct tcf_t), >>> + .optional = false, }, >>> }; >>> static int >>> @@ -1695,6 +1730,9 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower) >>> return EINVAL; >>> } >>> + nl_parse_tcf(nl_attr_get_unspec(csum_attrs[TCA_CSUM_TM], >>> + sizeof(struct tcf_t)), >>> + flower); >>> return 0; >>> } >>> @@ -1713,7 +1751,7 @@ static const struct nl_policy stats_policy[] = { >>> static int >>> nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, >>> - bool terse) >>> + bool terse, bool update_stats) >>> { >>> struct nlattr *act_options; >>> struct nlattr *act_stats; >>> @@ -1779,7 +1817,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, >>> } >>> bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); >>> - if (bs->packets) { >>> + if (bs->packets && update_stats) { >>> put_32aligned_u64(&stats->n_packets, bs->packets); >>> put_32aligned_u64(&stats->n_bytes, bs->bytes); >>> } >>> @@ -1797,6 +1835,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, >>> static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; >>> struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; >>> const int max_size = ARRAY_SIZE(actions_orders_policy); >>> + bool first_action = true; >>> + uint64_t lastused = 0; >>> for (int i = TCA_ACT_MIN_PRIO; i < max_size; i++) { >>> actions_orders_policy[i].type = NL_A_NESTED; >>> @@ -1817,14 +1857,27 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, >>> VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); >>> return EOPNOTSUPP; >>> } >>> - err = nl_parse_single_action(actions_orders[i], flower, terse); >>> + err = nl_parse_single_action(actions_orders[i], flower, terse, >>> + first_action); >>> if (err) { >>> return err; >>> } >>> + >>> + if (first_action) { >>> + /* We need to store the last used timestamp from the first >>> + * action, i.e., the action that was used the track the >>> + * statistics, as all remaining action decoding overwrite >>> + * this value. We also need to use the first actions statistics >>> + * as conditional actions can exist. */ >>> + lastused = flower->lastused; >>> + first_action = false; >>> + } >>> } >>> } >>> + flower->lastused = lastused; >>> + >>> if (flower->csum_update_flags) { >>> VLOG_WARN_RL(&error_rl, >>> "expected act csum with flags: 0x%x", >>> >>> _______________________________________________ >>> dev mailing list >>> dev@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/tc.c b/lib/tc.c index b37dce22f..b430f1bad 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -994,12 +994,35 @@ nl_parse_flower_flags(struct nlattr **attrs, struct tc_flower *flower) flower->offloaded_state = nl_get_flower_offloaded_state(attrs); } +static int +get_user_hz(void) +{ + static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; + static int user_hz = 100; + + if (ovsthread_once_start(&once)) { + user_hz = sysconf(_SC_CLK_TCK); + ovsthread_once_done(&once); + } + + return user_hz; +} + +static void +nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) +{ + flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); +} + static const struct nl_policy pedit_policy[] = { - [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, - .min_len = sizeof(struct tc_pedit), - .optional = false, }, - [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, - .optional = false, }, + [TCA_PEDIT_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, + [TCA_PEDIT_PARMS_EX] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tc_pedit), + .optional = false, }, + [TCA_PEDIT_KEYS_EX] = { .type = NL_A_NESTED, + .optional = false, }, }; static int @@ -1094,6 +1117,9 @@ nl_parse_act_pedit(struct nlattr *options, struct tc_flower *flower) action = &flower->actions[flower->action_count++]; action->type = TC_ACT_PEDIT; + nl_parse_tcf(nl_attr_get_unspec(pe_attrs[TCA_PEDIT_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1101,6 +1127,9 @@ static const struct nl_policy tunnel_key_policy[] = { [TCA_TUNNEL_KEY_PARMS] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct tc_tunnel_key), .optional = false, }, + [TCA_TUNNEL_KEY_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, [TCA_TUNNEL_KEY_ENC_IPV4_SRC] = { .type = NL_A_U32, .optional = true, }, [TCA_TUNNEL_KEY_ENC_IPV4_DST] = { .type = NL_A_U32, .optional = true, }, [TCA_TUNNEL_KEY_ENC_IPV6_SRC] = { .type = NL_A_UNSPEC, @@ -1275,6 +1304,10 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower) tun->action, tun->t_action); return EINVAL; } + + nl_parse_tcf(nl_attr_get_unspec(tun_attrs[TCA_TUNNEL_KEY_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1287,26 +1320,6 @@ static const struct nl_policy gact_policy[] = { .optional = false, }, }; -static int -get_user_hz(void) -{ - static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER; - static int user_hz = 100; - - if (ovsthread_once_start(&once)) { - user_hz = sysconf(_SC_CLK_TCK); - ovsthread_once_done(&once); - } - - return user_hz; -} - -static void -nl_parse_tcf(const struct tcf_t *tm, struct tc_flower *flower) -{ - flower->lastused = time_msec() - (tm->lastuse * 1000 / get_user_hz()); -} - static int nl_parse_act_gact(struct nlattr *options, struct tc_flower *flower) { @@ -1394,18 +1407,21 @@ nl_parse_act_mirred(struct nlattr *options, struct tc_flower *flower) static const struct nl_policy ct_policy[] = { [TCA_CT_PARMS] = { .type = NL_A_UNSPEC, - .min_len = sizeof(struct tc_ct), - .optional = false, }, + .min_len = sizeof(struct tc_ct), + .optional = false, }, + [TCA_CT_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, [TCA_CT_ACTION] = { .type = NL_A_U16, - .optional = true, }, + .optional = true, }, [TCA_CT_ZONE] = { .type = NL_A_U16, .optional = true, }, [TCA_CT_MARK] = { .type = NL_A_U32, .optional = true, }, [TCA_CT_MARK_MASK] = { .type = NL_A_U32, - .optional = true, }, + .optional = true, }, [TCA_CT_LABELS] = { .type = NL_A_UNSPEC, - .optional = true, }, + .optional = true, }, [TCA_CT_LABELS_MASK] = { .type = NL_A_UNSPEC, .optional = true, }, [TCA_CT_NAT_IPV4_MIN] = { .type = NL_A_U32, @@ -1517,6 +1533,9 @@ nl_parse_act_ct(struct nlattr *options, struct tc_flower *flower) } action->type = TC_ACT_CT; + nl_parse_tcf(nl_attr_get_unspec(ct_attrs[TCA_CT_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1524,6 +1543,9 @@ static const struct nl_policy vlan_policy[] = { [TCA_VLAN_PARMS] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct tc_vlan), .optional = false, }, + [TCA_VLAN_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, [TCA_VLAN_PUSH_VLAN_ID] = { .type = NL_A_U16, .optional = true, }, [TCA_VLAN_PUSH_VLAN_PROTOCOL] = { .type = NL_A_U16, .optional = true, }, [TCA_VLAN_PUSH_VLAN_PRIORITY] = { .type = NL_A_U8, .optional = true, }, @@ -1562,6 +1584,10 @@ nl_parse_act_vlan(struct nlattr *options, struct tc_flower *flower) v->action, v->v_action); return EINVAL; } + + nl_parse_tcf(nl_attr_get_unspec(vlan_attrs[TCA_VLAN_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1569,6 +1595,9 @@ static const struct nl_policy mpls_policy[] = { [TCA_MPLS_PARMS] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct tc_mpls), .optional = false, }, + [TCA_MPLS_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, [TCA_MPLS_PROTO] = { .type = NL_A_U16, .optional = true, }, [TCA_MPLS_LABEL] = { .type = NL_A_U32, .optional = true, }, [TCA_MPLS_TC] = { .type = NL_A_U8, .optional = true, }, @@ -1655,6 +1684,9 @@ nl_parse_act_mpls(struct nlattr *options, struct tc_flower *flower) return EINVAL; } + nl_parse_tcf(nl_attr_get_unspec(mpls_attrs[TCA_MPLS_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1662,6 +1694,9 @@ static const struct nl_policy csum_policy[] = { [TCA_CSUM_PARMS] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct tc_csum), .optional = false, }, + [TCA_CSUM_TM] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tcf_t), + .optional = false, }, }; static int @@ -1695,6 +1730,9 @@ nl_parse_act_csum(struct nlattr *options, struct tc_flower *flower) return EINVAL; } + nl_parse_tcf(nl_attr_get_unspec(csum_attrs[TCA_CSUM_TM], + sizeof(struct tcf_t)), + flower); return 0; } @@ -1713,7 +1751,7 @@ static const struct nl_policy stats_policy[] = { static int nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, - bool terse) + bool terse, bool update_stats) { struct nlattr *act_options; struct nlattr *act_stats; @@ -1779,7 +1817,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, } bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); - if (bs->packets) { + if (bs->packets && update_stats) { put_32aligned_u64(&stats->n_packets, bs->packets); put_32aligned_u64(&stats->n_bytes, bs->bytes); } @@ -1797,6 +1835,8 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, static struct nl_policy actions_orders_policy[TCA_ACT_MAX_NUM + 1] = {}; struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; const int max_size = ARRAY_SIZE(actions_orders_policy); + bool first_action = true; + uint64_t lastused = 0; for (int i = TCA_ACT_MIN_PRIO; i < max_size; i++) { actions_orders_policy[i].type = NL_A_NESTED; @@ -1817,14 +1857,27 @@ nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, VLOG_DBG_RL(&error_rl, "Can only support %d actions", TCA_ACT_MAX_NUM); return EOPNOTSUPP; } - err = nl_parse_single_action(actions_orders[i], flower, terse); + err = nl_parse_single_action(actions_orders[i], flower, terse, + first_action); if (err) { return err; } + + if (first_action) { + /* We need to store the last used timestamp from the first + * action, i.e., the action that was used the track the + * statistics, as all remaining action decoding overwrite + * this value. We also need to use the first actions statistics + * as conditional actions can exist. */ + lastused = flower->lastused; + first_action = false; + } } } + flower->lastused = lastused; + if (flower->csum_update_flags) { VLOG_WARN_RL(&error_rl, "expected act csum with flags: 0x%x",
Statistics should be captured on the first action in a list of actions, to do this we need to also capture the last update time stamp from the first action. Signed-off-by: Eelco Chaudron <echaudro@redhat.com> --- lib/tc.c | 119 +++++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 86 insertions(+), 33 deletions(-)