| Message ID | 20220815094916.301466-1-simon.horman@corigine.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] tc: Fix stats dump when using same meter table | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
References: <20220815094916.301466-1-simon.horman@corigine.com>
Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@corigine.com>
ERROR: Inappropriate bracing around statement
#55 FILE: lib/tc.c:1939:
if (is_meter)
Lines checked: 63, Warnings: 1, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
On 15 Aug 2022, at 11:49, Simon Horman wrote: > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > When we apply meter police on both directions of TCP traffic, the > dumped stats is shown same (as shown below). This issue is introduced > by modifying the stats update strategy. > > ...,in_port(6),eth(),eth_type(0x0800),ipv4(frag=no), packets:1488557, > bytes:2089059644, used:0.040s, actions:meter(0),9 > ...,in_port(9),eth(),eth_type(0x0800),ipv4(frag=no), packets:1488557, > bytes:2089059644, used:0.040s, actions:meter(0),6 > > In previous patch, after parsing police action, the flower stats will > be updated by dumped meter table stats, which will result in the issue > above. > > Thus, the stats of meter table should not be used when dumping flow > stats. Ignore the stats update when police.index belongs to meter. > > Fixes: a9b8cdde69de ("tc: Add support parsing tc police action") > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > Reviewed-by: Baowen Zheng <baowen.zheng@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> I did not do any testing, but here are some style comments. //Eelco > --- > lib/tc.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/lib/tc.c b/lib/tc.c > index aaeb7708cc7b..6c9ca10d48af 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1883,6 +1883,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > struct nlattr *act_cookie; > const char *act_kind; > struct nlattr *action_attrs[ARRAY_SIZE(act_policy)]; > + int act_index = flower->action_count; > + int is_meter = 0; bool is_meter = False; > int err = 0; > > if (!nl_parse_nested(action, act_policy, action_attrs, > @@ -1920,6 +1922,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > nl_parse_act_ct(act_options, flower); > } else if (!strcmp(act_kind, "police")) { > nl_parse_act_police(act_options, flower); > + is_meter = tc_is_meter_index(flower->actions[act_index].police.index); Should checking for the action type not be enough here? i.e. action->type == TC_ACT_POLICE? > } else { > VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); > err = EINVAL; > @@ -1933,6 +1936,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > flower->act_cookie.data = nl_attr_get(act_cookie); > flower->act_cookie.len = nl_attr_get_size(act_cookie); > } > + if (is_meter) > + return 0; if (is_meter) { return 0; } However, if the previous comment is true, all we need is if (action->type == TC_ACT_POLICE) { /* ADD A COMMENT ON WHY WE SKIP HERE */ return 0; } > > return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], > &flower->stats_sw, &flower->stats_hw, NULL); > -- > 2.30.2
diff --git a/lib/tc.c b/lib/tc.c index aaeb7708cc7b..6c9ca10d48af 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1883,6 +1883,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, struct nlattr *act_cookie; const char *act_kind; struct nlattr *action_attrs[ARRAY_SIZE(act_policy)]; + int act_index = flower->action_count; + int is_meter = 0; int err = 0; if (!nl_parse_nested(action, act_policy, action_attrs, @@ -1920,6 +1922,7 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, nl_parse_act_ct(act_options, flower); } else if (!strcmp(act_kind, "police")) { nl_parse_act_police(act_options, flower); + is_meter = tc_is_meter_index(flower->actions[act_index].police.index); } else { VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); err = EINVAL; @@ -1933,6 +1936,8 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, flower->act_cookie.data = nl_attr_get(act_cookie); flower->act_cookie.len = nl_attr_get_size(act_cookie); } + if (is_meter) + return 0; return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], &flower->stats_sw, &flower->stats_hw, NULL);