On 15 Aug 2022, at 11:49, Simon Horman wrote:
> From: Tianyu Yuan <tianyu.y...@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.y...@corigine.com>
> Reviewed-by: Baowen Zheng <baowen.zh...@corigine.com>
> Signed-off-by: Simon Horman <simon.hor...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
