| Message ID | 20221014113850.239348-1-simon.horman@corigine.com |
|---|---|
| State | Accepted |
| Commit | 743499607bdd0dcb3541a179ba2bb41ea10c4b3b |
| Headers | show |
| Series | [ovs-dev] Revert "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: <20221014113850.239348-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: 62, 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 10/14/22 13:38, Simon Horman wrote: > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > This reverts commit dd9881ed55e6 ('tc: Fix stats dump when > using same meter table') > > This patch doesn't solve the tc flow stats update issue and > will lead to failure of system-offloads-traffic testsuite, it > only counts packets surviving after the tc filter, rather than > hitting the filter > > A following patch will come up to solve this flow stats update > issue > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > lib/tc.c | 11 ----------- > 1 file changed, 11 deletions(-) Hi. I'm not really sure what is worse - the undercounting or overcounting. If you think that revert is a good temporary solution then it's fine by me: Acked-by: Ilya Maximets <i.maximets@ovn.org> It might make sense to add a note to the 'Known TC flow offload limitations' in Documentation/howto/tc-offload.rst about this issue though. Best regards, Ilya Maximets. > > diff --git a/lib/tc.c b/lib/tc.c > index 94044cde6060..f8fbe44bf244 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1904,8 +1904,6 @@ 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, > @@ -1943,7 +1941,6 @@ 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; > @@ -1958,14 +1955,6 @@ 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 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. */ > - if (is_meter) { > - return 0; > - } > - > return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], > &flower->stats_sw, &flower->stats_hw, NULL); > }
On Fri, Oct 14, 2022 at 03:09:56PM +0200, Ilya Maximets wrote: > On 10/14/22 13:38, Simon Horman wrote: > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > This reverts commit dd9881ed55e6 ('tc: Fix stats dump when > > using same meter table') > > > > This patch doesn't solve the tc flow stats update issue and > > will lead to failure of system-offloads-traffic testsuite, it > > only counts packets surviving after the tc filter, rather than > > hitting the filter > > > > A following patch will come up to solve this flow stats update > > issue > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > --- > > lib/tc.c | 11 ----------- > > 1 file changed, 11 deletions(-) > > > Hi. I'm not really sure what is worse - the undercounting or overcounting. > If you think that revert is a good temporary solution then it's fine by me: > > Acked-by: Ilya Maximets <i.maximets@ovn.org> Thanks. I see that this could go either way. And to be honest I don't feel strongly about it. But our position is that the fix wasn't a fix. So it's better to revert it. And work on a proper solution. > It might make sense to add a note to the 'Known TC flow offload limitations' > in Documentation/howto/tc-offload.rst about this issue though. Yes, good idea. We'll follow-up on that. Kind regards, Simon
On Thu, Oct 20, 2022 at 09:52:15AM +0100, Simon Horman wrote: > On Fri, Oct 14, 2022 at 03:09:56PM +0200, Ilya Maximets wrote: > > On 10/14/22 13:38, Simon Horman wrote: > > > From: Tianyu Yuan <tianyu.yuan@corigine.com> > > > > > > This reverts commit dd9881ed55e6 ('tc: Fix stats dump when > > > using same meter table') > > > > > > This patch doesn't solve the tc flow stats update issue and > > > will lead to failure of system-offloads-traffic testsuite, it > > > only counts packets surviving after the tc filter, rather than > > > hitting the filter > > > > > > A following patch will come up to solve this flow stats update > > > issue > > > > > > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com> > > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > > --- > > > lib/tc.c | 11 ----------- > > > 1 file changed, 11 deletions(-) > > > > > > Hi. I'm not really sure what is worse - the undercounting or overcounting. > > If you think that revert is a good temporary solution then it's fine by me: > > > > Acked-by: Ilya Maximets <i.maximets@ovn.org> > > Thanks. I see that this could go either way. And to be honest I don't feel > strongly about it. But our position is that the fix wasn't a fix. So it's > better to revert it. And work on a proper solution. Sorry for the delay. I've leant towards the revert and and have applied this patch accordingly. * 743499607bdd ("Revert "tc: Fix stats dump when using same meter table"") https://github.com/openvswitch/ovs/commit/743499607bdd And backported to branch-3.0: * d2ebceaff97e ("Revert "tc: Fix stats dump when using same meter table"") https://github.com/openvswitch/ovs/commit/d2ebceaff97e
diff --git a/lib/tc.c b/lib/tc.c index 94044cde6060..f8fbe44bf244 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1904,8 +1904,6 @@ 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, @@ -1943,7 +1941,6 @@ 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; @@ -1958,14 +1955,6 @@ 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 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. */ - if (is_meter) { - return 0; - } - return nl_parse_action_stats(action_attrs[TCA_ACT_STATS], &flower->stats_sw, &flower->stats_hw, NULL); }