netdev - RE: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate limiter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 30 May 2023 11:45:32 +0000
From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
CC: "s.shtylyov@....ru" <s.shtylyov@....ru>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"krzysztof.kozlowski+dt@...aro.org" <krzysztof.kozlowski+dt@...aro.org>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "geert+renesas@...der.be"
	<geert+renesas@...der.be>, "magnus.damm@...il.com" <magnus.damm@...il.com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-renesas-soc@...r.kernel.org" <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next 5/5] net: renesas: rswitch: Use per-queue rate
 limiter

Hi Geert-san,

> From: Geert Uytterhoeven, Sent: Tuesday, May 30, 2023 4:33 PM
> 
> Hi Shimoda-san,
> 
> On Mon, May 29, 2023 at 10:08 AM Yoshihiro Shimoda
> <yoshihiro.shimoda.uh@...esas.com> wrote:
> > Use per-queue rate limiter instead of global rate limiter. Otherwise
> > TX performance will be low when we use multiple ports at the same time.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
> 
> Thanks for your patch!

Thank you for your review!

> > --- a/drivers/net/ethernet/renesas/rswitch.c
> > +++ b/drivers/net/ethernet/renesas/rswitch.c
> > @@ -156,22 +156,31 @@ static int rswitch_gwca_axi_ram_reset(struct rswitch_private *priv)
> >         return rswitch_reg_wait(priv->addr, GWARIRM, GWARIRM_ARR, GWARIRM_ARR);
> >  }
> >
> > -static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv, int rate)
> > +static void rswitch_gwca_set_rate_limit(struct rswitch_private *priv,
> > +                                       struct rswitch_gwca_queue *txq)
> >  {
> > -       u32 gwgrlulc, gwgrlc;
> > +       u64 period_ps;
> > +       unsigned long rate;
> > +       u32 gwrlc;
> >
> > -       switch (rate) {
> > -       case 1000:
> > -               gwgrlulc = 0x0000005f;
> > -               gwgrlc = 0x00010260;
> > -               break;
> > -       default:
> > -               dev_err(&priv->pdev->dev, "%s: This rate is not supported (%d)\n", __func__, rate);
> > -               return;
> > -       }
> > +       rate = clk_get_rate(priv->aclk);
> > +       if (!rate)
> > +               rate = RSWITCH_ACLK_DEFAULT;
> > +
> > +       period_ps = div64_u64(1000000000000ULL, rate);
> 
> div64_ul, as rate is unsigned long.

I see.

> > +
> > +       /* GWRLC value = 256 * ACLK_period[ns] * maxBandwidth[Gbps] */
> > +       gwrlc = 256 * period_ps * txq->speed / 1000000;
> 
> This contains an open-coded 64-by-32 division, causing link failures
> on 32-bit platforms, so you should use div_u64() instead.  However,
> because of the premultiplication by speed, which is 32-bit, you can
> use the mul_u64_u32_div() helper.

Thank you for your comment! After I got emails from kernel test robot,
I realized that I should use such a macro here.

> Combining this with the calculation of period_ps above, you can simplify
> this to:
> 
>     gwrlc = div64_ul(256000000ULL * txq->speed, rate);

Thank you for your suggestion! I'll fix it on v2.

Best regards,
Yoshihiro Shimoda

> > +
> > +       /* To avoid overflow internally, the value should be 97% */
> > +       gwrlc = gwrlc * 97 / 100;
> >
> > -       iowrite32(gwgrlulc, priv->addr + GWGRLULC);
> > -       iowrite32(gwgrlc, priv->addr + GWGRLC);
> > +       dev_dbg(&priv->pdev->dev,
> > +               "%s: index = %d, speed = %d, rate = %ld, gwrlc = %08x\n",
> > +               __func__, txq->index_trim, txq->speed, rate, gwrlc);
> > +
> > +       iowrite32(GWRLULC_NOT_REQUIRED, priv->addr + GWRLULC(txq->index_trim));
> > +       iowrite32(gwrlc | GWRLC_RLE, priv->addr + GWRLC(txq->index_trim));
> >  }
> >
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds