diff mbox series

[ovs-dev,v2,1/2] netdev-linux: correct unit of burst parameter

Message ID 20210312115917.6838-2-simon.horman@netronome.com
State Changes Requested
Headers show
Series netdev-linux: correct unit of burst parameter | expand

Commit Message

Simon Horman March 12, 2021, 11:59 a.m. UTC 
From: "Yong.Xu" <yong.xu@corigine.com>

Correct calculation of burst parameter used when configuring TC policer
action for ingress port-based policing in the case where TC offload is in
use. This now matches the value calculated for the case where TC offload is
not in use.

The division by 8 is to convert from bits to bytes.
Its unclear why 64 was previously used.

Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
Signed-off-by: Yong.Xu <yong.xu@corigine.com>
[simon: reworked changelog]
Signed-off-by: Simon Horman <simon.horman@netronome.com>
Signed-off-by: Louis Peens <louis.peens@netronome.com>
---
 lib/netdev-linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

0-day Robot March 12, 2021, 1:58 p.m. UTC | #1 
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@netronome.com>, Louis Peens <louis.peens@netronome.com>
Lines checked: 38, 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
Marcelo Leitner March 25, 2021, 6:51 p.m. UTC | #2 
On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman wrote:
> From: "Yong.Xu" <yong.xu@corigine.com>
> 
> Correct calculation of burst parameter used when configuring TC policer
> action for ingress port-based policing in the case where TC offload is in
> use. This now matches the value calculated for the case where TC offload is
> not in use.
> 
> The division by 8 is to convert from bits to bytes.
> Its unclear why 64 was previously used.

Yeah.. I have the feeling that it might be related to kernel's:
  /* Avoid doing 64 bit divide */
  #define PSCHED_SHIFT                    6
  #define PSCHED_TICKS2NS(x)              ((s64)(x) << PSCHED_SHIFT)
  #define PSCHED_NS2TICKS(x)              ((x) >> PSCHED_SHIFT)
but I can't confirm it.

> 
> Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
> Signed-off-by: Yong.Xu <yong.xu@corigine.com>
> [simon: reworked changelog]
> Signed-off-by: Simon Horman <simon.horman@netronome.com>
> Signed-off-by: Louis Peens <louis.peens@netronome.com>
> ---
>  lib/netdev-linux.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 15b25084b..f87a20075 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2572,7 +2572,7 @@ exit:
>  static struct tc_police
>  tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
>  {
> -    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> +    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
>      unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;

I know that the patch is not changing this but, while at this, why for
bsize the 'k' is 1024, while for bps it's 1000?

AFAICT it backtracks to
      netdev_set_policing(iface->netdev,
                          MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
                          MIN(UINT32_MAX, iface->cfg->ingress_policing_burst));
in iface_configure_qos() and I don't see a reason for them being
different.

qos.rst states:
``ingress_policing_rate``
  the maximum rate (in Kbps) that this VM should be allowed to send

``ingress_policing_burst``
  a parameter to the policing algorithm to indicate the maximum amount of data
  (in Kb) that this interface can send beyond the policing rate.

Both with capital K. So if the 64 was bothering, the 1024 is likely
doing so as well.

Nevertheless, as the patch is fixing the /64, patch LGTM.

>      struct tc_police police;
>      struct tc_ratespec rate;
> -- 
> 2.20.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Marcelo Leitner March 25, 2021, 6:54 p.m. UTC | #3 
On Thu, Mar 25, 2021 at 03:51:11PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman wrote:
> > From: "Yong.Xu" <yong.xu@corigine.com>
> > 
> > Correct calculation of burst parameter used when configuring TC policer
> > action for ingress port-based policing in the case where TC offload is in
> > use. This now matches the value calculated for the case where TC offload is
> > not in use.
> > 
> > The division by 8 is to convert from bits to bytes.
> > Its unclear why 64 was previously used.
> 
> Yeah.. I have the feeling that it might be related to kernel's:
>   /* Avoid doing 64 bit divide */
>   #define PSCHED_SHIFT                    6
>   #define PSCHED_TICKS2NS(x)              ((s64)(x) << PSCHED_SHIFT)
>   #define PSCHED_NS2TICKS(x)              ((x) >> PSCHED_SHIFT)
> but I can't confirm it.
> 
> > 
> > Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
> > Signed-off-by: Yong.Xu <yong.xu@corigine.com>
> > [simon: reworked changelog]
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > Signed-off-by: Louis Peens <louis.peens@netronome.com>
> > ---
> >  lib/netdev-linux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 15b25084b..f87a20075 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2572,7 +2572,7 @@ exit:
> >  static struct tc_police
> >  tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> >  {
> > -    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> > +    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
> >      unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;
> 
> I know that the patch is not changing this but, while at this, why for
> bsize the 'k' is 1024, while for bps it's 1000?
> 
> AFAICT it backtracks to
>       netdev_set_policing(iface->netdev,
>                           MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
>                           MIN(UINT32_MAX, iface->cfg->ingress_policing_burst));
> in iface_configure_qos() and I don't see a reason for them being
> different.
> 
> qos.rst states:
> ``ingress_policing_rate``
>   the maximum rate (in Kbps) that this VM should be allowed to send
> 
> ``ingress_policing_burst``
>   a parameter to the policing algorithm to indicate the maximum amount of data
>   (in Kb) that this interface can send beyond the policing rate.
> 
> Both with capital K. So if the 64 was bothering, the 1024 is likely
> doing so as well.

While vswitchd/vswitch.xml says:
      <column name="ingress_policing_rate">
        <p>
          Maximum rate for data received on this interface, in kbps.  Data

      <column name="ingress_policing_burst">
        <p>Maximum burst size for data received on this interface, in kb.  The
        default burst size if set to <code>0</code> is 8000 kbit.  This value

I'm not sure which one has preference here.

> 
> Nevertheless, as the patch is fixing the /64, patch LGTM.
> 
> >      struct tc_police police;
> >      struct tc_ratespec rate;
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Simon Horman April 1, 2021, 7:06 a.m. UTC | #4 
On Thu, Mar 25, 2021 at 03:51:11PM -0300, Marcelo Ricardo Leitner wrote:
> On Fri, Mar 12, 2021 at 12:59:16PM +0100, Simon Horman wrote:
> > From: "Yong.Xu" <yong.xu@corigine.com>
> > 
> > Correct calculation of burst parameter used when configuring TC policer
> > action for ingress port-based policing in the case where TC offload is in
> > use. This now matches the value calculated for the case where TC offload is
> > not in use.
> > 
> > The division by 8 is to convert from bits to bytes.
> > Its unclear why 64 was previously used.
> 
> Yeah.. I have the feeling that it might be related to kernel's:
>   /* Avoid doing 64 bit divide */
>   #define PSCHED_SHIFT                    6
>   #define PSCHED_TICKS2NS(x)              ((s64)(x) << PSCHED_SHIFT)
>   #define PSCHED_NS2TICKS(x)              ((x) >> PSCHED_SHIFT)
> but I can't confirm it.

That is a pretty good point, and I suspect you are correct.
But I honestly don't know either.

> > Fixes: e7f6ba220 ("lib/tc: add ingress ratelimiting support for tc-offload")
> > Signed-off-by: Yong.Xu <yong.xu@corigine.com>
> > [simon: reworked changelog]
> > Signed-off-by: Simon Horman <simon.horman@netronome.com>
> > Signed-off-by: Louis Peens <louis.peens@netronome.com>
> > ---
> >  lib/netdev-linux.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index 15b25084b..f87a20075 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -2572,7 +2572,7 @@ exit:
> >  static struct tc_police
> >  tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
> >  {
> > -    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
> > +    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
> >      unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;
> 
> I know that the patch is not changing this but, while at this, why for
> bsize the 'k' is 1024, while for bps it's 1000?
> 
> AFAICT it backtracks to
>       netdev_set_policing(iface->netdev,
>                           MIN(UINT32_MAX, iface->cfg->ingress_policing_rate),
>                           MIN(UINT32_MAX, iface->cfg->ingress_policing_burst));
> in iface_configure_qos() and I don't see a reason for them being
> different.
> 
> qos.rst states:
> ``ingress_policing_rate``
>   the maximum rate (in Kbps) that this VM should be allowed to send
> 
> ``ingress_policing_burst``
>   a parameter to the policing algorithm to indicate the maximum amount of data
>   (in Kb) that this interface can send beyond the policing rate.
> 
> Both with capital K. So if the 64 was bothering, the 1024 is likely
> doing so as well.

Thanks, I seem to recall looking into this once. I will dig once again and
try and answer the question. Possibly then we will then
decide to change things. Let's see.

> Nevertheless, as the patch is fixing the /64, patch LGTM.
> 
> >      struct tc_police police;
> >      struct tc_ratespec rate;
> > -- 
> > 2.20.1
> > 
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 15b25084b..f87a20075 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2572,7 +2572,7 @@  exit:
 static struct tc_police
 tc_matchall_fill_police(uint32_t kbits_rate, uint32_t kbits_burst)
 {
-    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 64;
+    unsigned int bsize = MIN(UINT32_MAX / 1024, kbits_burst) * 1024 / 8;
     unsigned int bps = ((uint64_t) kbits_rate * 1000) / 8;
     struct tc_police police;
     struct tc_ratespec rate;