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

[Marcelo Ricardo Leitner]

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.

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