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

[Marcelo Ricardo Leitner]

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 at 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 at corigine.com>
> > [simon: reworked changelog]
> > Signed-off-by: Simon Horman <simon.horman at netronome.com>
> > Signed-off-by: Louis Peens <louis.peens at 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 at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev