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