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 <[email protected]> > > > > 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 <[email protected]> > > Signed-off-by: Simon Horman <[email protected]> > > 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
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
