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 <[email protected]> >>> >>> 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 <[email protected]> >>> Signed-off-by: Simon Horman <[email protected]> > >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 >>>
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
