| Message ID | 20220817112948.59356-1-simon.horman@corigine.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] tc: Fix stats dump when using same meter table | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/intel-ovs-compilation | success | test: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
References: <20220817112948.59356-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> Lines checked: 64, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 17 Aug 2022, at 13:29, 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> > Signed-off-by: Simon Horman <simon.horman@corigine.com> I missed the v2, so I commented on the v1, so let me repeat the comments here (as you already fixed some in v2 ;) V1> “I did not do any testing, but here are some style comments.“ //Eelco > --- > lib/tc.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > v2 > * Address checkpatch warning regarding missing {} for if clause > * Initialise bool is_meter as false rather than 0 > > diff --git a/lib/tc.c b/lib/tc.c > index aaeb7708cc7b..70a05def3c5f 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; > + 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. is_meter = action->type == TC_ACT_POLICE? > } else { > VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); > err = EINVAL; > @@ -1934,6 +1937,10 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > flower->act_cookie.len = nl_attr_get_size(act_cookie); > } > > + if (is_meter) { > + return 0; > + } 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 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 1 Sep 2022, at 16:45, Tianyu Yuan wrote: Sorry to not reply to your review in time and thanks for your review > On 17 Aug 2022, at 13:29, 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> > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > I missed the v2, so I commented on the v1, so let me repeat the > comments here (as you already fixed some in v2 ;) > > V1> “I did not do any testing, but here are some style comments.“ > > //Eelco > > > --- > > lib/tc.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > v2 > > * Address checkpatch warning regarding missing {} for if clause > > * Initialise bool is_meter as false rather than 0 > > > > diff --git a/lib/tc.c b/lib/tc.c > > index aaeb7708cc7b..70a05def3c5f 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; > > + 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. is_meter = action->type == TC_ACT_POLICE? > For now the police in tc_action is only used for meter in tc flower. However, some other kinds of police could be introduced in future, whose stats cannot be skipped when dumping rule stats (f.e. only a single police with continue flag in a tc filter to match the next filter). > > } else { > > VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); > > err = EINVAL; > > @@ -1934,6 +1937,10 @@ nl_parse_single_action(struct nlattr *action, > struct tc_flower *flower, > > flower->act_cookie.len = nl_attr_get_size(act_cookie); > > } > > > > + if (is_meter) { > > + return 0; > > + } > > > If the previous comment is true, all we need is > > if (action->type == TC_ACT_POLICE) { As explanation before, action type is enough for now but not strict enough. > /* ADD A COMMENT ON WHY WE SKIP HERE */ I'll add a comment in v3 patch > 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..70a05def3c5f 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; + 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); } else { VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); err = EINVAL; @@ -1934,6 +1937,10 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, 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); }