Message ID | 20221011093527.367783-1-simon.horman@corigine.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev] netdev-linux: Allow meter to work in tc software datapath | 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: <20221011093527.367783-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 Tue, Oct 11, 2022 at 11:35:27AM +0200, 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. > > In this patch, 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. Gentle ping for review of this. I believe this issue is independent from other discussions around metering that are ongoing. > 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 | 3 +++ > 5 files changed, 48 insertions(+), 13 deletions(-) > > 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..c6422aacefeb 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 independ from filter, we need to > + * add action flag according to tc-policy. */ > + if (single_action) { > + nl_msg_put_tc_action_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..e46f5cc73c8e 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_tc_action_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..8e3226e89bd4 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -399,4 +399,7 @@ 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_tc_action_flag(struct ofpbuf *request); > + > #endif /* tc.h */ > -- > 2.30.2 >
On 10/19/22 10:20, Simon Horman wrote: > On Tue, Oct 11, 2022 at 11:35:27AM +0200, 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. >> >> In this patch, 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. > > Gentle ping for review of this. > > I believe this issue is independent from other discussions > around metering that are ongoing. > >> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> >> Signed-off-by: Simon Horman <simon.horman@corigine.com> Hi. Thanks for the patch! It looks logically correct to me, but I didn't test. See some comments below. First thing is that the subject line for the patch seems a bit misleading. Meters do work in TC software datapath, they will not work if tc-policy is specified. I think, this needs to be clarified in the subject line. Also, we may technically classify that change as a bug fix, so the 'Fixes:' tag may be appropriate. >> --- >> acinclude.m4 | 6 +++--- >> include/linux/pkt_cls.h | 11 +++++++---- >> lib/netdev-linux.c | 20 ++++++++++++++------ >> lib/tc.c | 21 +++++++++++++++++++++ >> lib/tc.h | 3 +++ >> 5 files changed, 48 insertions(+), 13 deletions(-) >> >> 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..c6422aacefeb 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 independ from filter, we need to "independently" ? >> + * add action flag according to tc-policy. */ >> + if (single_action) { >> + nl_msg_put_tc_action_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..e46f5cc73c8e 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_tc_action_flag(struct ofpbuf *request) We have already a function named nl_msg_put_act_flags() which is very similar to this one. And they kind of have the same purpose, but with their own specifics. I think, these functions should either be merged or the new one should be re-named to better describe its purpose. Maybe nl_msg_put_act_tc_policy_flag() ? >> +{ >> + 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..8e3226e89bd4 100644 >> --- a/lib/tc.h >> +++ b/lib/tc.h >> @@ -399,4 +399,7 @@ 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_tc_action_flag(struct ofpbuf *request); This should be on a single line. grep '^function_name' should point to function implementations, not the prototypes. If the line break is necessary, it should be done in arguments, if possible. >> + >> #endif /* tc.h */ >> -- >> 2.30.2 >>
On October 20, 2022 7:54 PM, Ilya wrote: >On 10/19/22 10:20, Simon Horman wrote: >> On Tue, Oct 11, 2022 at 11:35:27AM +0200, 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. >>> >>> In this patch, 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. >> >> Gentle ping for review of this. >> >> I believe this issue is independent from other discussions around >> metering that are ongoing. >> >>> Signed-off-by: Baowen Zheng <baowen.zheng@corigine.com> >>> Signed-off-by: Simon Horman <simon.horman@corigine.com> > >Hi. Thanks for the patch! >It looks logically correct to me, but I didn't test. >See some comments below. > >First thing is that the subject line for the patch seems a bit misleading. Meters >do work in TC software datapath, they will not work if tc-policy is specified. I >think, this needs to be clarified in the subject line. > >Also, we may technically classify that change as a bug fix, so the 'Fixes:' tag >may be appropriate. Ok, I will supplement the commit message as your suggestion, thanks. > >>> --- >>> acinclude.m4 | 6 +++--- >>> include/linux/pkt_cls.h | 11 +++++++---- >>> lib/netdev-linux.c | 20 ++++++++++++++------ >>> lib/tc.c | 21 +++++++++++++++++++++ >>> lib/tc.h | 3 +++ >>> 5 files changed, 48 insertions(+), 13 deletions(-) >>> >>> 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..c6422aacefeb 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 independ from filter, we need to > >"independently" ? Ok, I will make the change. > >>> + * add action flag according to tc-policy. */ >>> + if (single_action) { >>> + nl_msg_put_tc_action_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..e46f5cc73c8e 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_tc_action_flag(struct ofpbuf *request) > >We have already a function named nl_msg_put_act_flags() which is very >similar to this one. And they kind of have the same purpose, but with their >own specifics. > >I think, these functions should either be merged or the new one should be re- >named to better describe its purpose. >Maybe nl_msg_put_act_tc_policy_flag() ? Thanks, I will rename the new function to mark this function is for independent actions. > >>> +{ >>> + 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..8e3226e89bd4 100644 >>> --- a/lib/tc.h >>> +++ b/lib/tc.h >>> @@ -399,4 +399,7 @@ 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_tc_action_flag(struct ofpbuf *request); > >This should be on a single line. >grep '^function_name' should point to function implementations, not the >prototypes. If the line break is necessary, it should be done in arguments, if >possible. Ok, will make the change. > >>> + >>> #endif /* tc.h */ >>> -- >>> 2.30.2 >>>
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..c6422aacefeb 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 independ from filter, we need to + * add action flag according to tc-policy. */ + if (single_action) { + nl_msg_put_tc_action_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..e46f5cc73c8e 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_tc_action_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..8e3226e89bd4 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -399,4 +399,7 @@ 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_tc_action_flag(struct ofpbuf *request); + #endif /* tc.h */