Re: [ovs-dev] [PATCH v1 7/7] netdev-linux: support 64-bit rates in tc policing
On 2 Jun 2023, at 16:13, Adrian Moreno wrote:
> Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits.
>
> This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by
> netdev's API expressing kbps rates using 32-bit integers.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643
> Signed-off-by: Adrian Moreno
This patch looks good to me, however, I have one additional general issue with
this series.
For example, if the running kernel does not support TCA_POLICE_RATE64,
TCA_HTB_RATE64, or TCA_HTB_CEIL64 but the kernel it was compiled on does. This
will break the implementation. Maybe we need some probe in
netdev_tc_init_flow_api()?
However looking at the current code we already use TCA_POLICE_PKTRATE64 which
is later than TCA_POLICE_RATE64, so we should be good?! If this is true we also
do not need all the ‘#ifdef HAVE_TCA_POLICE_PKTRATE64’ and related code in the
this patch.
The HTB ones were added in 2013, so maybe we are good? If we are we can remove
all the ‘#ifdef HAVE_TCA_HTB_RATE64’ and related code in patch 4.
Simon/Ilya any opinions on this?
Cheers,
Eelco
> ---
> acinclude.m4| 10 ++
> lib/netdev-linux.c | 19 ---
> lib/netdev-linux.h | 2 +-
> lib/tc.c| 2 ++
> tests/atlocal.in| 1 +
> tests/system-traffic.at | 21 +
> 6 files changed, 47 insertions(+), 8 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index 1e5a9872d..3ac7072b5 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -221,6 +221,16 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [
> [Define to 1 if TCA_HTB_RATE64 is available.])],
> [AC_SUBST(HAVE_TCA_HTB_RATE64,no)]
> )
> +
> + AC_COMPILE_IFELSE([
> +AC_LANG_PROGRAM([#include ], [
> +int x = TCA_POLICE_PKTRATE64;
> +])],
> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,yes)
> + AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1],
> + [Define to 1 if TCA_POLICE_PKTRATE64 is available.])],
> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,no)]
> +)
> ])
>
> dnl OVS_CHECK_LINUX_SCTP_CT
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 3526fbfc3..6a9f36f1e 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -494,7 +494,7 @@ static struct tcmsg *netdev_linux_tc_make_request(const
> struct netdev *,
>unsigned int flags,
>struct ofpbuf *);
>
> -static int tc_add_policer(struct netdev *, uint32_t kbits_rate,
> +static int tc_add_policer(struct netdev *, uint64_t kbits_rate,
>uint32_t kbits_burst, uint32_t kpkts_rate,
>uint32_t kpkts_burst);
>
> @@ -2660,6 +2660,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
> index,
>uint64_t pkts_rate, uint64_t pkts_burst,
>uint32_t notexceed_act, bool single_action)
> {
> +uint64_t bytes_rate = kbits_rate / 8 * 1000;
> size_t offset, act_offset;
> struct tc_police police;
> uint32_t prio = 0;
> @@ -2674,8 +2675,13 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
> index,
> 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, 0);
> +tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate);
> }
> +#ifdef HAVE_TCA_POLICE_PKTRATE64
> +if (bytes_rate > UINT32_MAX) {
> +nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate);
> +}
> +#endif
> if (pkts_rate) {
> uint64_t pkt_burst_ticks;
> /* Here tc_bytes_to_ticks is used to convert packets rather than
> bytes
> @@ -2689,7 +2695,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
> index,
> }
>
> static int
> -tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate,
> +tc_add_matchall_policer(struct netdev *netdev, uint64_t kbits_rate,
> uint32_t kbits_burst, uint32_t kpkts_rate,
> uint32_t kpkts_burst)
> {
> @@ -5703,9 +5709,8 @@ tc_policer_init(struct tc_police *tc_police, uint64_t
> kbits_rate,
> * Returns 0 if successful, otherwise a positive errno value.
> */
> static int
> -tc_add_policer(struct netdev *netdev, uint32_t kbits_rate,
> - uint32_t kbits_burst, uint32_t kpkts_rate,
> - uint32_t kpkts_burst)
> +tc_add_policer(struct netdev *netdev, uint64_t kbits_rate,
> + uint32_t kbits_burst, uint32_t kpkts_rate, uint32_t
> kpkts_burst)
> {
> size_t basic_offset, police_offset;
> struct ofpbuf request;
> @@ -5739,7 +5744,7 @@ tc_add_policer(struct netdev *netdev, uint32_t
> kbits_rate,
> }
>
> int
> -tc_add_policer_action(uint32_t index, uint32_t kbits_rate,
> +tc_add_policer_action(uint
Re: [ovs-dev] [PATCH v1 7/7] netdev-linux: support 64-bit rates in tc policing
On 7/6/23 18:22, Eelco Chaudron wrote:
>
>
> On 2 Jun 2023, at 16:13, Adrian Moreno wrote:
>
>> Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits.
>>
>> This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by
>> netdev's API expressing kbps rates using 32-bit integers.
>>
>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643
>> Signed-off-by: Adrian Moreno
>
> This patch looks good to me, however, I have one additional general issue
> with this series.
>
> For example, if the running kernel does not support TCA_POLICE_RATE64,
> TCA_HTB_RATE64, or TCA_HTB_CEIL64 but the kernel it was compiled on does.
> This will break the implementation. Maybe we need some probe in
> netdev_tc_init_flow_api()?
netdev_tc_init_flow_api() will not be called if hw-offload is not enabled.
But anyway, we only add 64bit arguments if we have to. i.e. if the value
fits into 32bit range, 64bit argument will not be used. That should provide
required backward compatibility. If we need 64bit, but kernel doesn't
support, then we need to fail anyway. Current code will configure incorrect
values, there is no need preserving that.
>
> However looking at the current code we already use TCA_POLICE_PKTRATE64 which
> is later than TCA_POLICE_RATE64, so we should be good?! If this is true we
> also do not need all the ‘#ifdef HAVE_TCA_POLICE_PKTRATE64’ and related code
> in the this patch.
kpkts policing in a relatively new feature in OVS, so we do not expect it
to be used on old kernels. So, use of TCA_POLICE_PKTRATE64 should be fine.
>
> The HTB ones were added in 2013, so maybe we are good? If we are we can
> remove all the ‘#ifdef HAVE_TCA_HTB_RATE64’ and related code in patch 4.
>
> Simon/Ilya any opinions on this?
>
> Cheers,
>
> Eelco
>
>
>> ---
>> acinclude.m4| 10 ++
>> lib/netdev-linux.c | 19 ---
>> lib/netdev-linux.h | 2 +-
>> lib/tc.c| 2 ++
>> tests/atlocal.in| 1 +
>> tests/system-traffic.at | 21 +
>> 6 files changed, 47 insertions(+), 8 deletions(-)
>>
>> diff --git a/acinclude.m4 b/acinclude.m4
>> index 1e5a9872d..3ac7072b5 100644
>> --- a/acinclude.m4
>> +++ b/acinclude.m4
>> @@ -221,6 +221,16 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [
>> [Define to 1 if TCA_HTB_RATE64 is available.])],
>> [AC_SUBST(HAVE_TCA_HTB_RATE64,no)]
>> )
>> +
>> + AC_COMPILE_IFELSE([
>> +AC_LANG_PROGRAM([#include ], [
>> +int x = TCA_POLICE_PKTRATE64;
>> +])],
>> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,yes)
>> + AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1],
>> + [Define to 1 if TCA_POLICE_PKTRATE64 is available.])],
>> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,no)]
>> +)
>> ])
>>
>> dnl OVS_CHECK_LINUX_SCTP_CT
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index 3526fbfc3..6a9f36f1e 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -494,7 +494,7 @@ static struct tcmsg *netdev_linux_tc_make_request(const
>> struct netdev *,
>>unsigned int flags,
>>struct ofpbuf *);
>>
>> -static int tc_add_policer(struct netdev *, uint32_t kbits_rate,
>> +static int tc_add_policer(struct netdev *, uint64_t kbits_rate,
>>uint32_t kbits_burst, uint32_t kpkts_rate,
>>uint32_t kpkts_burst);
>>
>> @@ -2660,6 +2660,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
>> index,
>>uint64_t pkts_rate, uint64_t pkts_burst,
>>uint32_t notexceed_act, bool single_action)
>> {
>> +uint64_t bytes_rate = kbits_rate / 8 * 1000;
>> size_t offset, act_offset;
>> struct tc_police police;
>> uint32_t prio = 0;
>> @@ -2674,8 +2675,13 @@ nl_msg_put_act_police(struct ofpbuf *request,
>> uint32_t index,
>> 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, 0);
>> +tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate);
>> }
>> +#ifdef HAVE_TCA_POLICE_PKTRATE64
>> +if (bytes_rate > UINT32_MAX) {
>> +nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate);
>> +}
>> +#endif
>> if (pkts_rate) {
>> uint64_t pkt_burst_ticks;
>> /* Here tc_bytes_to_ticks is used to convert packets rather than
>> bytes
>> @@ -2689,7 +2695,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
>> index,
>> }
>>
>> static int
>> -tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate,
>> +tc_add_matchall_policer(struct netdev *netdev, uint64_t kbits_rate,
>> uint32_t kbits_burst, uint32_t kpkts_rate,
>> uint32_t kpkts_burst)
>> {
>> @@ -
Re: [ovs-dev] [PATCH v1 7/7] netdev-linux: support 64-bit rates in tc policing
On 7 Jul 2023, at 17:04, Ilya Maximets wrote:
> On 7/6/23 18:22, Eelco Chaudron wrote:
>>
>>
>> On 2 Jun 2023, at 16:13, Adrian Moreno wrote:
>>
>>> Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits.
>>>
>>> This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by
>>> netdev's API expressing kbps rates using 32-bit integers.
>>>
>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643
>>> Signed-off-by: Adrian Moreno
>>
>> This patch looks good to me, however, I have one additional general issue
>> with this series.
>>
>> For example, if the running kernel does not support TCA_POLICE_RATE64,
>> TCA_HTB_RATE64, or TCA_HTB_CEIL64 but the kernel it was compiled on does.
>> This will break the implementation. Maybe we need some probe in
>> netdev_tc_init_flow_api()?
>
> netdev_tc_init_flow_api() will not be called if hw-offload is not enabled.
> But anyway, we only add 64bit arguments if we have to. i.e. if the value
> fits into 32bit range, 64bit argument will not be used. That should provide
> required backward compatibility. If we need 64bit, but kernel doesn't
> support, then we need to fail anyway. Current code will configure incorrect
> values, there is no need preserving that.
>
>>
>> However looking at the current code we already use TCA_POLICE_PKTRATE64
>> which is later than TCA_POLICE_RATE64, so we should be good?! If this is
>> true we also do not need all the ‘#ifdef HAVE_TCA_POLICE_PKTRATE64’ and
>> related code in the this patch.
>
> kpkts policing in a relatively new feature in OVS, so we do not expect it
> to be used on old kernels. So, use of TCA_POLICE_PKTRATE64 should be fine.
Ok, I’m fine with both explanations on top of my own research, so:
Acked-by: Eelco Chaudron
>>
>> The HTB ones were added in 2013, so maybe we are good? If we are we can
>> remove all the ‘#ifdef HAVE_TCA_HTB_RATE64’ and related code in patch 4.
>>
>> Simon/Ilya any opinions on this?
>>
>> Cheers,
>>
>> Eelco
>>
>>
>>> ---
>>> acinclude.m4| 10 ++
>>> lib/netdev-linux.c | 19 ---
>>> lib/netdev-linux.h | 2 +-
>>> lib/tc.c| 2 ++
>>> tests/atlocal.in| 1 +
>>> tests/system-traffic.at | 21 +
>>> 6 files changed, 47 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/acinclude.m4 b/acinclude.m4
>>> index 1e5a9872d..3ac7072b5 100644
>>> --- a/acinclude.m4
>>> +++ b/acinclude.m4
>>> @@ -221,6 +221,16 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [
>>> [Define to 1 if TCA_HTB_RATE64 is available.])],
>>> [AC_SUBST(HAVE_TCA_HTB_RATE64,no)]
>>> )
>>> +
>>> + AC_COMPILE_IFELSE([
>>> +AC_LANG_PROGRAM([#include ], [
>>> +int x = TCA_POLICE_PKTRATE64;
>>> +])],
>>> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,yes)
>>> + AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1],
>>> + [Define to 1 if TCA_POLICE_PKTRATE64 is available.])],
>>> +[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,no)]
>>> +)
>>> ])
>>>
>>> dnl OVS_CHECK_LINUX_SCTP_CT
>>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>>> index 3526fbfc3..6a9f36f1e 100644
>>> --- a/lib/netdev-linux.c
>>> +++ b/lib/netdev-linux.c
>>> @@ -494,7 +494,7 @@ static struct tcmsg *netdev_linux_tc_make_request(const
>>> struct netdev *,
>>>unsigned int flags,
>>>struct ofpbuf *);
>>>
>>> -static int tc_add_policer(struct netdev *, uint32_t kbits_rate,
>>> +static int tc_add_policer(struct netdev *, uint64_t kbits_rate,
>>>uint32_t kbits_burst, uint32_t kpkts_rate,
>>>uint32_t kpkts_burst);
>>>
>>> @@ -2660,6 +2660,7 @@ nl_msg_put_act_police(struct ofpbuf *request,
>>> uint32_t index,
>>>uint64_t pkts_rate, uint64_t pkts_burst,
>>>uint32_t notexceed_act, bool single_action)
>>> {
>>> +uint64_t bytes_rate = kbits_rate / 8 * 1000;
>>> size_t offset, act_offset;
>>> struct tc_police police;
>>> uint32_t prio = 0;
>>> @@ -2674,8 +2675,13 @@ nl_msg_put_act_police(struct ofpbuf *request,
>>> uint32_t index,
>>> 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, 0);
>>> +tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate);
>>> }
>>> +#ifdef HAVE_TCA_POLICE_PKTRATE64
>>> +if (bytes_rate > UINT32_MAX) {
>>> +nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate);
>>> +}
>>> +#endif
>>> if (pkts_rate) {
>>> uint64_t pkt_burst_ticks;
>>> /* Here tc_bytes_to_ticks is used to convert packets rather than
>>> bytes
>>> @@ -2689,7 +2695,7 @@ nl_msg_put_act_police(struct ofpbuf *request,
>>> uint32_t index,
>>> }
>>>
>>> static int
>>> -tc_add_mat
Re: [ovs-dev] [PATCH v1 7/7] netdev-linux: support 64-bit rates in tc policing
On 7/7/23 17:04, Ilya Maximets wrote:
On 7/6/23 18:22, Eelco Chaudron wrote:
On 2 Jun 2023, at 16:13, Adrian Moreno wrote:
Use TCA_POLICE_RATE64 if the rate cannot be expressed using 32bits.
This breaks the 32Gbps barrier. The new barrier is ~4Tbps caused by
netdev's API expressing kbps rates using 32-bit integers.
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2137643
Signed-off-by: Adrian Moreno
This patch looks good to me, however, I have one additional general issue with
this series.
For example, if the running kernel does not support TCA_POLICE_RATE64,
TCA_HTB_RATE64, or TCA_HTB_CEIL64 but the kernel it was compiled on does. This
will break the implementation. Maybe we need some probe in
netdev_tc_init_flow_api()?
netdev_tc_init_flow_api() will not be called if hw-offload is not enabled.
But anyway, we only add 64bit arguments if we have to. i.e. if the value
fits into 32bit range, 64bit argument will not be used. That should provide
required backward compatibility. If we need 64bit, but kernel doesn't
support, then we need to fail anyway. Current code will configure incorrect
values, there is no need preserving that.
However looking at the current code we already use TCA_POLICE_PKTRATE64 which
is later than TCA_POLICE_RATE64, so we should be good?! If this is true we also
do not need all the ‘#ifdef HAVE_TCA_POLICE_PKTRATE64’ and related code in the
this patch.
kpkts policing in a relatively new feature in OVS, so we do not expect it
to be used on old kernels. So, use of TCA_POLICE_PKTRATE64 should be fine.
The HTB ones were added in 2013, so maybe we are good? If we are we can remove
all the ‘#ifdef HAVE_TCA_HTB_RATE64’ and related code in patch 4.
Simon/Ilya any opinions on this?
Cheers,
Eelco
---
acinclude.m4| 10 ++
lib/netdev-linux.c | 19 ---
lib/netdev-linux.h | 2 +-
lib/tc.c| 2 ++
tests/atlocal.in| 1 +
tests/system-traffic.at | 21 +
6 files changed, 47 insertions(+), 8 deletions(-)
diff --git a/acinclude.m4 b/acinclude.m4
index 1e5a9872d..3ac7072b5 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -221,6 +221,16 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [
[Define to 1 if TCA_HTB_RATE64 is available.])],
[AC_SUBST(HAVE_TCA_HTB_RATE64,no)]
)
+
+ AC_COMPILE_IFELSE([
+AC_LANG_PROGRAM([#include ], [
+int x = TCA_POLICE_PKTRATE64;
+])],
+[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,yes)
+ AC_DEFINE([HAVE_TCA_POLICE_PKTRATE64], [1],
+ [Define to 1 if TCA_POLICE_PKTRATE64 is available.])],
+[AC_SUBST(HAVE_TCA_POLICE_PKTRATE64,no)]
+)
])
dnl OVS_CHECK_LINUX_SCTP_CT
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 3526fbfc3..6a9f36f1e 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -494,7 +494,7 @@ static struct tcmsg *netdev_linux_tc_make_request(const
struct netdev *,
unsigned int flags,
struct ofpbuf *);
-static int tc_add_policer(struct netdev *, uint32_t kbits_rate,
+static int tc_add_policer(struct netdev *, uint64_t kbits_rate,
uint32_t kbits_burst, uint32_t kpkts_rate,
uint32_t kpkts_burst);
@@ -2660,6 +2660,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
index,
uint64_t pkts_rate, uint64_t pkts_burst,
uint32_t notexceed_act, bool single_action)
{
+uint64_t bytes_rate = kbits_rate / 8 * 1000;
size_t offset, act_offset;
struct tc_police police;
uint32_t prio = 0;
@@ -2674,8 +2675,13 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
index,
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, 0);
+tc_put_rtab(request, TCA_POLICE_RATE, &police.rate, bytes_rate);
}
+#ifdef HAVE_TCA_POLICE_PKTRATE64
+if (bytes_rate > UINT32_MAX) {
+nl_msg_put_u64(request, TCA_POLICE_RATE64, bytes_rate);
+}
+#endif
if (pkts_rate) {
uint64_t pkt_burst_ticks;
/* Here tc_bytes_to_ticks is used to convert packets rather than bytes
@@ -2689,7 +2695,7 @@ nl_msg_put_act_police(struct ofpbuf *request, uint32_t
index,
}
static int
-tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate,
+tc_add_matchall_policer(struct netdev *netdev, uint64_t kbits_rate,
uint32_t kbits_burst, uint32_t kpkts_rate,
uint32_t kpkts_burst)
{
@@ -5703,9 +5709,8 @@ tc_policer_init(struct tc_police *tc_police, uint64_t
kbits_rate,
* Returns 0 if successful, otherwise a positive errno value.
*/
static int
-tc_add_policer(struct netdev *netdev, uint32_t kbits_rate,
-
