| Message ID | 20221021091031.52856-1-simon.horman@corigine.com |
|---|---|
| State | Accepted |
| Commit | ffcb6f115fe5e00be3ca8fb9a940a3224e687e23 |
| Headers | show |
| Series | [ovs-dev,v2] netdev-linux: Allow meter to work in tc software datapath when tc-policy is specified | 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: <20221021091031.52856-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: 188, 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 October 21, 2022 5:11 PM, Simon wrote: >From: Baowen Zheng <baowen.zheng@corigine.com> > >Add tc action flags when adding police action to offload meter table. > >There is a restriction that the flag of skip_sw/skip_hw should be same for filter >rule and the independent created tc actions the rule uses. In this case, if we >configure the tc-policy as skip_hw, filter rule will be created with skip_hw flag >and the police action according to meter table will have no action flag, then >flower rule will fail to add to tc kernel system. > >To fix this issue, we will add tc action flag when adding police action to offload >a meter table, so it will allow meter table to work in tc software datapath. > Hi all, gentle ping for review, thanks. >Fixes: 5c039ddc64ff ("netdev-linux: Add functions to manipulate tc police >action") >Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> >Signed-off-by: Simon Horman <simon.horman@corigine.com> >--- > acinclude.m4 | 6 +++--- > include/linux/pkt_cls.h | 11 +++++++---- > lib/netdev-linux.c | 20 ++++++++++++++------ > lib/tc.c | 21 +++++++++++++++++++++ > lib/tc.h | 2 ++ > 5 files changed, 47 insertions(+), 13 deletions(-) > > v2: > * Address review of v1 > - Update patch description: problem depends on setting tc-policy > - Correct typo in comment: "independently" > - Reformat nl_msg_put_tc_action_flag() declaration > - Rename helper as nl_msg_put_act_tc_policy_flag() and correct > line wrapping of declaration. > >diff --git a/acinclude.m4 b/acinclude.m4 index ad07989ac29c..aa9af55062f0 >100644 >--- a/acinclude.m4 >+++ b/acinclude.m4 >@@ -163,10 +163,10 @@ dnl Configure Linux tc compat. > AC_DEFUN([OVS_CHECK_LINUX_TC], [ > AC_COMPILE_IFELSE([ > AC_LANG_PROGRAM([#include <linux/pkt_cls.h>], [ >- int x = TCA_POLICE_PKTRATE64; >+ int x = TCA_ACT_FLAGS_SKIP_HW; > ])], >- [AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1], >- [Define to 1 if TCA_POLICE_PKTRATE64 is available.])]) >+ [AC_DEFINE([HAVE_TCA_ACT_FLAGS_SKIP_HW], [1], >+ [Define to 1 if TCA_ACT_FLAGS_SKIP_HW is available.])]) > > AC_CHECK_MEMBERS([struct tcf_t.firstuse], [], [], [#include ><linux/pkt_cls.h>]) > >diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h index >ba82e690eba9..a8cd8db5bf88 100644 >--- a/include/linux/pkt_cls.h >+++ b/include/linux/pkt_cls.h >@@ -1,7 +1,7 @@ > #ifndef __LINUX_PKT_CLS_WRAPPER_H > #define __LINUX_PKT_CLS_WRAPPER_H 1 > >-#if defined(__KERNEL__) || defined(HAVE_TCA_POLICE_PKTRATE64) >+#if defined(__KERNEL__) || defined(HAVE_TCA_ACT_FLAGS_SKIP_HW) > #include_next <linux/pkt_cls.h> > #else > >@@ -21,9 +21,12 @@ enum { > __TCA_ACT_MAX > }; > >-#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator >for >- * actions stats. >- */ >+/* See other TCA_ACT_FLAGS_ * flags in include/net/act_api.h. */ >+#define TCA_ACT_FLAGS_NO_PERCPU_STATS (1 << 0) /* Don't use percpu >allocator for >+ * actions stats. >+ */ >+#define TCA_ACT_FLAGS_SKIP_HW (1 << 1) /* don't offload action to HW >*/ >+#define TCA_ACT_FLAGS_SKIP_SW (1 << 2) /* don't use action in SW */ > > #define TCA_ACT_MAX __TCA_ACT_MAX > #define TCA_OLD_COMPAT (TCA_ACT_MAX+1) >diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index >cdc66246ced3..7ea4070c23ab 100644 >--- a/lib/netdev-linux.c >+++ b/lib/netdev-linux.c >@@ -2623,10 +2623,17 @@ tc_matchall_fill_police(uint32_t kbits_rate, >uint32_t kbits_burst) > > static void > nl_msg_act_police_start_nest(struct ofpbuf *request, uint32_t prio, >- size_t *offset, size_t *act_offset) >+ size_t *offset, size_t *act_offset, >+ bool single_action) > { > *act_offset = nl_msg_start_nested(request, prio); > nl_msg_put_string(request, TCA_ACT_KIND, "police"); >+ >+ /* If police action is added independently from filter, we need to >+ * add action flag according to tc-policy. */ >+ if (single_action) { >+ nl_msg_put_act_tc_policy_flag(request); >+ } > *offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS); } > >@@ -2642,7 +2649,7 @@ nl_msg_act_police_end_nest(struct ofpbuf >*request, size_t offset, static void nl_msg_put_act_police(struct ofpbuf >*request, struct tc_police *police, > uint64_t pkts_rate, uint64_t pkts_burst, >- uint32_t notexceed_act) >+ uint32_t notexceed_act, bool single_action) > { > size_t offset, act_offset; > uint32_t prio = 0; >@@ -2651,7 +2658,8 @@ nl_msg_put_act_police(struct ofpbuf *request, >struct tc_police *police, > return; > } > >- nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); >+ nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset, >+ single_action); > if (police->rate.rate) { > tc_put_rtab(request, TCA_POLICE_RATE, &police->rate); > } >@@ -2698,7 +2706,7 @@ tc_add_matchall_policer(struct netdev *netdev, >uint32_t kbits_rate, > basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); > action_offset = nl_msg_start_nested(&request, TCA_MATCHALL_ACT); > nl_msg_put_act_police(&request, &pol_act, kpkts_rate * 1000, >- kpkts_burst * 1000, TC_ACT_UNSPEC); >+ kpkts_burst * 1000, TC_ACT_UNSPEC, false); > nl_msg_end_nested(&request, action_offset); > nl_msg_end_nested(&request, basic_offset); > >@@ -5667,7 +5675,7 @@ tc_add_policer(struct netdev *netdev, uint32_t >kbits_rate, > police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT); > tc_policer_init(&tc_police, kbits_rate, kbits_burst); > nl_msg_put_act_police(&request, &tc_police, kpkts_rate * 1000ULL, >- kpkts_burst * 1000ULL, TC_ACT_UNSPEC); >+ kpkts_burst * 1000ULL, TC_ACT_UNSPEC, false); > nl_msg_end_nested(&request, police_offset); > nl_msg_end_nested(&request, basic_offset); > >@@ -5702,7 +5710,7 @@ tc_add_policer_action(uint32_t index, uint32_t >kbits_rate, > > offset = nl_msg_start_nested(&request, TCA_ACT_TAB); > nl_msg_put_act_police(&request, &tc_police, pkts_rate, pkts_burst, >- TC_ACT_PIPE); >+ TC_ACT_PIPE, true); > nl_msg_end_nested(&request, offset); > > error = tc_transact(&request, NULL); diff --git a/lib/tc.c b/lib/tc.c index >94044cde6060..66dc4208888c 100644 >--- a/lib/tc.c >+++ b/lib/tc.c >@@ -3808,3 +3808,24 @@ tc_set_policy(const char *policy) > > VLOG_INFO("tc: Using policy '%s'", policy); } >+ >+void >+nl_msg_put_act_tc_policy_flag(struct ofpbuf *request) { >+ int flag = 0; >+ >+ if (!request) { >+ return; >+ } >+ >+ if (tc_policy == TC_POLICY_SKIP_HW) { >+ flag = TCA_ACT_FLAGS_SKIP_HW; >+ } else if (tc_policy == TC_POLICY_SKIP_SW) { >+ flag = TCA_ACT_FLAGS_SKIP_SW; >+ } >+ >+ if (flag) { >+ struct nla_bitfield32 flags = { flag, flag }; >+ nl_msg_put_unspec(request, TCA_ACT_FLAGS, &flags, sizeof flags); >+ } >+} >diff --git a/lib/tc.h b/lib/tc.h >index 2e64ad372592..161f438124b0 100644 >--- a/lib/tc.h >+++ b/lib/tc.h >@@ -399,4 +399,6 @@ int tc_parse_action_stats(struct nlattr *action, int >tc_dump_tc_action_start(char *name, struct nl_dump *dump); int >parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]); > >+void nl_msg_put_act_tc_policy_flag(struct ofpbuf *request); >+ > #endif /* tc.h */ >-- >2.30.2
On 10/21/22 11:10, Simon Horman wrote: > From: Baowen Zheng <baowen.zheng@corigine.com> > > Add tc action flags when adding police action to offload meter table. > > There is a restriction that the flag of skip_sw/skip_hw should be same for > filter rule and the independent created tc actions the rule uses. In this > case, if we configure the tc-policy as skip_hw, filter rule will be created > with skip_hw flag and the police action according to meter table will have > no action flag, then flower rule will fail to add to tc kernel system. > > To fix this issue, we will add tc action flag when adding police action to > offload a meter table, so it will allow meter table to work in tc software > datapath. > > Fixes: 5c039ddc64ff ("netdev-linux: Add functions to manipulate tc police action") > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > acinclude.m4 | 6 +++--- > include/linux/pkt_cls.h | 11 +++++++---- > lib/netdev-linux.c | 20 ++++++++++++++------ > lib/tc.c | 21 +++++++++++++++++++++ > lib/tc.h | 2 ++ > 5 files changed, 47 insertions(+), 13 deletions(-) > > v2: > * Address review of v1 > - Update patch description: problem depends on setting tc-policy > - Correct typo in comment: "independently" > - Reformat nl_msg_put_tc_action_flag() declaration > - Rename helper as nl_msg_put_act_tc_policy_flag() and correct > line wrapping of declaration. Hi. Thanks for v2! The code looks correct to me, but I didn't test it. With that in mind: Acked-by: Ilya Maximets <i.maximets@ovn.org>
On Mon, Oct 31, 2022 at 02:07:46PM +0100, Ilya Maximets wrote: > On 10/21/22 11:10, Simon Horman wrote: > > From: Baowen Zheng <baowen.zheng@corigine.com> > > > > Add tc action flags when adding police action to offload meter table. > > > > There is a restriction that the flag of skip_sw/skip_hw should be same for > > filter rule and the independent created tc actions the rule uses. In this > > case, if we configure the tc-policy as skip_hw, filter rule will be created > > with skip_hw flag and the police action according to meter table will have > > no action flag, then flower rule will fail to add to tc kernel system. > > > > To fix this issue, we will add tc action flag when adding police action to > > offload a meter table, so it will allow meter table to work in tc software > > datapath. > > > > Fixes: 5c039ddc64ff ("netdev-linux: Add functions to manipulate tc police action") > > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > --- > > acinclude.m4 | 6 +++--- > > include/linux/pkt_cls.h | 11 +++++++---- > > lib/netdev-linux.c | 20 ++++++++++++++------ > > lib/tc.c | 21 +++++++++++++++++++++ > > lib/tc.h | 2 ++ > > 5 files changed, 47 insertions(+), 13 deletions(-) > > > > v2: > > * Address review of v1 > > - Update patch description: problem depends on setting tc-policy > > - Correct typo in comment: "independently" > > - Reformat nl_msg_put_tc_action_flag() declaration > > - Rename helper as nl_msg_put_act_tc_policy_flag() and correct > > line wrapping of declaration. > > Hi. Thanks for v2! > > The code looks correct to me, but I didn't test it. > With that in mind: > > Acked-by: Ilya Maximets <i.maximets@ovn.org> Thanks Ilya, I will plan to apply this a little later today. I expect that it also needs to be backported to branch-3.0, and I'll plan to do that too. Kind regards, Simon
On Tue, Nov 01, 2022 at 10:05:32AM +0100, Simon Horman wrote: > On Mon, Oct 31, 2022 at 02:07:46PM +0100, Ilya Maximets wrote: > > On 10/21/22 11:10, Simon Horman wrote: > > > From: Baowen Zheng <baowen.zheng@corigine.com> > > > > > > Add tc action flags when adding police action to offload meter table. > > > > > > There is a restriction that the flag of skip_sw/skip_hw should be same for > > > filter rule and the independent created tc actions the rule uses. In this > > > case, if we configure the tc-policy as skip_hw, filter rule will be created > > > with skip_hw flag and the police action according to meter table will have > > > no action flag, then flower rule will fail to add to tc kernel system. > > > > > > To fix this issue, we will add tc action flag when adding police action to > > > offload a meter table, so it will allow meter table to work in tc software > > > datapath. > > > > > > Fixes: 5c039ddc64ff ("netdev-linux: Add functions to manipulate tc police action") > > > Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> > > > Signed-off-by: Simon Horman <simon.horman@corigine.com> > > > --- > > > acinclude.m4 | 6 +++--- > > > include/linux/pkt_cls.h | 11 +++++++---- > > > lib/netdev-linux.c | 20 ++++++++++++++------ > > > lib/tc.c | 21 +++++++++++++++++++++ > > > lib/tc.h | 2 ++ > > > 5 files changed, 47 insertions(+), 13 deletions(-) > > > > > > v2: > > > * Address review of v1 > > > - Update patch description: problem depends on setting tc-policy > > > - Correct typo in comment: "independently" > > > - Reformat nl_msg_put_tc_action_flag() declaration > > > - Rename helper as nl_msg_put_act_tc_policy_flag() and correct > > > line wrapping of declaration. > > > > Hi. Thanks for v2! > > > > The code looks correct to me, but I didn't test it. > > With that in mind: > > > > Acked-by: Ilya Maximets <i.maximets@ovn.org> > > Thanks Ilya, > > I will plan to apply this a little later today. > I expect that it also needs to be backported to branch-3.0, > and I'll plan to do that too. Applied: * ffcb6f115fe5 ("netdev-linux: Allow meter to work in tc software datapath when tc-policy is specified") https://github.com/openvswitch/ovs/commit/ffcb6f115fe5 Backport to branch-3.0. * 4a2fe9f1e3c9 ("netdev-linux: Allow meter to work in tc software datapath when tc-policy is specified") https://github.com/openvswitch/ovs/commit/4a2fe9f1e3c9
diff --git a/acinclude.m4 b/acinclude.m4 index ad07989ac29c..aa9af55062f0 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -163,10 +163,10 @@ dnl Configure Linux tc compat. AC_DEFUN([OVS_CHECK_LINUX_TC], [ AC_COMPILE_IFELSE([ AC_LANG_PROGRAM([#include <linux/pkt_cls.h>], [ - int x = TCA_POLICE_PKTRATE64; + int x = TCA_ACT_FLAGS_SKIP_HW; ])], - [AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1], - [Define to 1 if TCA_POLICE_PKTRATE64 is available.])]) + [AC_DEFINE([HAVE_TCA_ACT_FLAGS_SKIP_HW], [1], + [Define to 1 if TCA_ACT_FLAGS_SKIP_HW is available.])]) AC_CHECK_MEMBERS([struct tcf_t.firstuse], [], [], [#include <linux/pkt_cls.h>]) diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h index ba82e690eba9..a8cd8db5bf88 100644 --- a/include/linux/pkt_cls.h +++ b/include/linux/pkt_cls.h @@ -1,7 +1,7 @@ #ifndef __LINUX_PKT_CLS_WRAPPER_H #define __LINUX_PKT_CLS_WRAPPER_H 1 -#if defined(__KERNEL__) || defined(HAVE_TCA_POLICE_PKTRATE64) +#if defined(__KERNEL__) || defined(HAVE_TCA_ACT_FLAGS_SKIP_HW) #include_next <linux/pkt_cls.h> #else @@ -21,9 +21,12 @@ enum { __TCA_ACT_MAX }; -#define TCA_ACT_FLAGS_NO_PERCPU_STATS 1 /* Don't use percpu allocator for - * actions stats. - */ +/* See other TCA_ACT_FLAGS_ * flags in include/net/act_api.h. */ +#define TCA_ACT_FLAGS_NO_PERCPU_STATS (1 << 0) /* Don't use percpu allocator for + * actions stats. + */ +#define TCA_ACT_FLAGS_SKIP_HW (1 << 1) /* don't offload action to HW */ +#define TCA_ACT_FLAGS_SKIP_SW (1 << 2) /* don't use action in SW */ #define TCA_ACT_MAX __TCA_ACT_MAX #define TCA_OLD_COMPAT (TCA_ACT_MAX+1) diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index cdc66246ced3..7ea4070c23ab 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2623,10 +2623,17 @@ tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst) static void nl_msg_act_police_start_nest(struct ofpbuf *request, uint32_t prio, - size_t *offset, size_t *act_offset) + size_t *offset, size_t *act_offset, + bool single_action) { *act_offset = nl_msg_start_nested(request, prio); nl_msg_put_string(request, TCA_ACT_KIND, "police"); + + /* If police action is added independently from filter, we need to + * add action flag according to tc-policy. */ + if (single_action) { + nl_msg_put_act_tc_policy_flag(request); + } *offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS); } @@ -2642,7 +2649,7 @@ nl_msg_act_police_end_nest(struct ofpbuf *request, size_t offset, static void nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, uint64_t pkts_rate, uint64_t pkts_burst, - uint32_t notexceed_act) + uint32_t notexceed_act, bool single_action) { size_t offset, act_offset; uint32_t prio = 0; @@ -2651,7 +2658,8 @@ nl_msg_put_act_police(struct ofpbuf *request, struct tc_police *police, return; } - nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset); + nl_msg_act_police_start_nest(request, ++prio, &offset, &act_offset, + single_action); if (police->rate.rate) { tc_put_rtab(request, TCA_POLICE_RATE, &police->rate); } @@ -2698,7 +2706,7 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS); action_offset = nl_msg_start_nested(&request, TCA_MATCHALL_ACT); nl_msg_put_act_police(&request, &pol_act, kpkts_rate * 1000, - kpkts_burst * 1000, TC_ACT_UNSPEC); + kpkts_burst * 1000, TC_ACT_UNSPEC, false); nl_msg_end_nested(&request, action_offset); nl_msg_end_nested(&request, basic_offset); @@ -5667,7 +5675,7 @@ tc_add_policer(struct netdev *netdev, uint32_t kbits_rate, police_offset = nl_msg_start_nested(&request, TCA_BASIC_ACT); tc_policer_init(&tc_police, kbits_rate, kbits_burst); nl_msg_put_act_police(&request, &tc_police, kpkts_rate * 1000ULL, - kpkts_burst * 1000ULL, TC_ACT_UNSPEC); + kpkts_burst * 1000ULL, TC_ACT_UNSPEC, false); nl_msg_end_nested(&request, police_offset); nl_msg_end_nested(&request, basic_offset); @@ -5702,7 +5710,7 @@ tc_add_policer_action(uint32_t index, uint32_t kbits_rate, offset = nl_msg_start_nested(&request, TCA_ACT_TAB); nl_msg_put_act_police(&request, &tc_police, pkts_rate, pkts_burst, - TC_ACT_PIPE); + TC_ACT_PIPE, true); nl_msg_end_nested(&request, offset); error = tc_transact(&request, NULL); diff --git a/lib/tc.c b/lib/tc.c index 94044cde6060..66dc4208888c 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -3808,3 +3808,24 @@ tc_set_policy(const char *policy) VLOG_INFO("tc: Using policy '%s'", policy); } + +void +nl_msg_put_act_tc_policy_flag(struct ofpbuf *request) +{ + int flag = 0; + + if (!request) { + return; + } + + if (tc_policy == TC_POLICY_SKIP_HW) { + flag = TCA_ACT_FLAGS_SKIP_HW; + } else if (tc_policy == TC_POLICY_SKIP_SW) { + flag = TCA_ACT_FLAGS_SKIP_SW; + } + + if (flag) { + struct nla_bitfield32 flags = { flag, flag }; + nl_msg_put_unspec(request, TCA_ACT_FLAGS, &flags, sizeof flags); + } +} diff --git a/lib/tc.h b/lib/tc.h index 2e64ad372592..161f438124b0 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -399,4 +399,6 @@ int tc_parse_action_stats(struct nlattr *action, int tc_dump_tc_action_start(char *name, struct nl_dump *dump); int parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]); +void nl_msg_put_act_tc_policy_flag(struct ofpbuf *request); + #endif /* tc.h */