| Message ID | 20220907093606.33656-1-simon.horman@corigine.com |
|---|---|
| State | Accepted |
| Commit | dd9881ed55e6bf98b77a8c2347ec6001b0bee3f8 |
| 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 |
On Wed, Sep 07, 2022 at 11:36:06AM +0200, 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> > --- > > Changes between v2 and v3 > * Add comment to explain why we skip stats update only when act_police is meter > > Changes between v1 and v2 > * Fix checkpatch errors Sorry, I forgot to mark this patch as v3 when posting it. > > --- > lib/tc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/lib/tc.c b/lib/tc.c > index f8fbe44bf244..0cf6817d3ca9 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1904,6 +1904,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, > @@ -1941,6 +1943,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; > @@ -1955,6 +1958,16 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > flower->act_cookie.len = nl_attr_get_size(act_cookie); > } > > + /* > + * Skip the stats update when act_police is meter, since there are always > + * some other actions following meter. For other potential kind of police, > + * whose stats could not be skipped (e.g. filter has only one police > + * action), update the action stats to flow rule > + */ > + if (is_meter) { > + return 0; > + } > + > return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], > &flower->stats_sw, &flower->stats_hw, NULL); > } > -- > 2.30.2 >
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: 71, 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 7 Sep 2022, at 11:36, 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> > --- > > Changes between v2 and v3 > * Add comment to explain why we skip stats update only when act_police is meter > > Changes between v1 and v2 > * Fix checkpatch errors > > --- > lib/tc.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/lib/tc.c b/lib/tc.c > index f8fbe44bf244..0cf6817d3ca9 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1904,6 +1904,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, > @@ -1941,6 +1943,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; > @@ -1955,6 +1958,16 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > flower->act_cookie.len = nl_attr_get_size(act_cookie); > } > > + /* > + * Skip the stats update when act_police is meter, since there are always > + * some other actions following meter. For other potential kind of police, > + * whose stats could not be skipped (e.g. filter has only one police > + * action), update the action stats to flow rule > + */ Small nit: + /* Skip the stats update when act_police is meter since there are always + * some other actions following meter. For other potential kinds of + * act_police actions, whose stats could not be skipped (e.g. filter has + * only one police action), update the action stats to the flow rule. */ The rest looks good to me. Acked-by: Eelco Chaudron <echaudro@redhat.com> > + if (is_meter) { > + 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 Mon, Sep 12, 2022 at 03:02:06PM +0200, Eelco Chaudron wrote: > > > On 7 Sep 2022, at 11:36, 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> ... > > + /* > > + * Skip the stats update when act_police is meter, since there are always > > + * some other actions following meter. For other potential kind of police, > > + * whose stats could not be skipped (e.g. filter has only one police > > + * action), update the action stats to flow rule > > + */ > > Small nit: > > > + /* Skip the stats update when act_police is meter since there are always > + * some other actions following meter. For other potential kinds of > + * act_police actions, whose stats could not be skipped (e.g. filter has > + * only one police action), update the action stats to the flow rule. */ > > The rest looks good to me. > > Acked-by: Eelco Chaudron <echaudro@redhat.com> Thanks Eelco, I've made the change suggested above and applied the patch to master and branch-3.0. * on master dd9881ed55e6 ("tc: Fix stats dump when using same meter table") https://github.com/openvswitch/ovs/commit/dd9881ed55e6bf98b77a8c2347ec6001b0bee3f8 * on branch-3.0 787648996993 ("tc: Fix stats dump when using same meter table") https://github.com/openvswitch/ovs/commit/dd9881ed55e6bf98b77a8c2347ec6001b0bee3f8
On 9/13/22 09:51, Simon Horman wrote: > On Mon, Sep 12, 2022 at 03:02:06PM +0200, Eelco Chaudron wrote: >> >> >> On 7 Sep 2022, at 11:36, 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> > > ... > >>> + /* >>> + * Skip the stats update when act_police is meter, since there are always >>> + * some other actions following meter. For other potential kind of police, >>> + * whose stats could not be skipped (e.g. filter has only one police >>> + * action), update the action stats to flow rule >>> + */ >> >> Small nit: >> >> >> + /* Skip the stats update when act_police is meter since there are always >> + * some other actions following meter. For other potential kinds of >> + * act_police actions, whose stats could not be skipped (e.g. filter has >> + * only one police action), update the action stats to the flow rule. */ >> >> The rest looks good to me. >> >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > Thanks Eelco, > > I've made the change suggested above and applied the patch to master and > branch-3.0. Hi, Simon and Eelco. This commit made offload tests consistently fail due to incorrectly counted flow stats: # make check-offloads TESTSUITEFLAGS='8' 8. system-offloads-traffic.at:230: testing offloads - check interface meter offloading - offloads enabled ... ./system-offloads-traffic.at:266: ovs-appctl dpctl/dump-flows | grep "meter" | sed -e 's/used:\([0-9.]*s\|never\)/used:0.001s/;s/eth( src=[a-z0-9:]*,dst=[a-z0-9:]*)/eth(macs)/;s/actions:[0-9,]\+/actions:output/;s/recirc_id(0),//' | sort --- - 2022-09-13 06:37:19.306764602 -0400 +++ /root/ovs/tests/system-offloads-testsuite.dir/at-groups/8/stdout 2022-09-13 06:37:19.304198787 -0400 @@ -1,2 +1,2 @@ -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:10, bytes:330, used:0.001s, actions:meter(0),3 +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:1, bytes:33, used:0.001s, actions:meter(0),3 Could you, please, take a look? I see this on my rhel8 VM. Best regards, Ilya Maximets.
On Tue, Sep 13, 2022 at 12:41:33PM +0200, Ilya Maximets wrote: > On 9/13/22 09:51, Simon Horman wrote: > > On Mon, Sep 12, 2022 at 03:02:06PM +0200, Eelco Chaudron wrote: > >> > >> > >> On 7 Sep 2022, at 11:36, 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> > > > > ... > > > >>> + /* > >>> + * Skip the stats update when act_police is meter, since there are always > >>> + * some other actions following meter. For other potential kind of police, > >>> + * whose stats could not be skipped (e.g. filter has only one police > >>> + * action), update the action stats to flow rule > >>> + */ > >> > >> Small nit: > >> > >> > >> + /* Skip the stats update when act_police is meter since there are always > >> + * some other actions following meter. For other potential kinds of > >> + * act_police actions, whose stats could not be skipped (e.g. filter has > >> + * only one police action), update the action stats to the flow rule. */ > >> > >> The rest looks good to me. > >> > >> Acked-by: Eelco Chaudron <echaudro@redhat.com> > > > > Thanks Eelco, > > > > I've made the change suggested above and applied the patch to master and > > branch-3.0. > > Hi, Simon and Eelco. > > This commit made offload tests consistently fail due to incorrectly counted > flow stats: > > # make check-offloads TESTSUITEFLAGS='8' > 8. system-offloads-traffic.at:230: testing offloads - check interface meter offloading - offloads enabled ... > > ./system-offloads-traffic.at:266: ovs-appctl dpctl/dump-flows | grep "meter" | sed -e 's/used:\([0-9.]*s\|never\)/used:0.001s/;s/eth( > src=[a-z0-9:]*,dst=[a-z0-9:]*)/eth(macs)/;s/actions:[0-9,]\+/actions:output/;s/recirc_id(0),//' | sort > --- - 2022-09-13 06:37:19.306764602 -0400 > +++ /root/ovs/tests/system-offloads-testsuite.dir/at-groups/8/stdout 2022-09-13 06:37:19.304198787 -0400 > @@ -1,2 +1,2 @@ > -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:10, bytes:330, used:0.001s, actions:meter(0),3 > +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:1, bytes:33, used:0.001s, actions:meter(0),3 > > Could you, please, take a look? > > I see this on my rhel8 VM. Thanks for letting me know and sorry about this. We'll look into it.
On Tue, Sep 13, 2022 at 01:47:31PM +0200, Simon Horman wrote: > On Tue, Sep 13, 2022 at 12:41:33PM +0200, Ilya Maximets wrote: > > On 9/13/22 09:51, Simon Horman wrote: > > > On Mon, Sep 12, 2022 at 03:02:06PM +0200, Eelco Chaudron wrote: ... > > > Thanks Eelco, > > > > > > I've made the change suggested above and applied the patch to master and > > > branch-3.0. > > > > Hi, Simon and Eelco. > > > > This commit made offload tests consistently fail due to incorrectly counted > > flow stats: > > > > # make check-offloads TESTSUITEFLAGS='8' > > 8. system-offloads-traffic.at:230: testing offloads - check interface meter offloading - offloads enabled ... > > > > ./system-offloads-traffic.at:266: ovs-appctl dpctl/dump-flows | grep "meter" | sed -e 's/used:\([0-9.]*s\|never\)/used:0.001s/;s/eth( > > src=[a-z0-9:]*,dst=[a-z0-9:]*)/eth(macs)/;s/actions:[0-9,]\+/actions:output/;s/recirc_id(0),//' | sort > > --- - 2022-09-13 06:37:19.306764602 -0400 > > +++ /root/ovs/tests/system-offloads-testsuite.dir/at-groups/8/stdout 2022-09-13 06:37:19.304198787 -0400 > > @@ -1,2 +1,2 @@ > > -in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:10, bytes:330, used:0.001s, actions:meter(0),3 > > +in_port(2),eth(macs),eth_type(0x0800),ipv4(proto=17,frag=no), packets:1, bytes:33, used:0.001s, actions:meter(0),3 > > > > Could you, please, take a look? > > > > I see this on my rhel8 VM. > > Thanks for letting me know and sorry about this. > We'll look into it. Hi again, it seems to us that the problem here is that the test suite should have been updated, but was not. We have posted a patch to address this: * https://mail.openvswitch.org/pipermail/ovs-dev/2022-September/397685.html
diff --git a/lib/tc.c b/lib/tc.c index f8fbe44bf244..0cf6817d3ca9 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1904,6 +1904,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, @@ -1941,6 +1943,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; @@ -1955,6 +1958,16 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, flower->act_cookie.len = nl_attr_get_size(act_cookie); } + /* + * Skip the stats update when act_police is meter, since there are always + * some other actions following meter. For other potential kind of police, + * whose stats could not be skipped (e.g. filter has only one police + * action), update the action stats to flow rule + */ + if (is_meter) { + return 0; + } + return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], &flower->stats_sw, &flower->stats_hw, NULL); }