[ovs-dev] netdev-linux: Allow meter to work in tc software datapath

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.

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(-)

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
>>>