[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e3e3c75f-9aee-40b6-b5b8-08260a8bac06@bp.renesas.com>
Date: Mon, 15 Apr 2024 08:04:05 +0100
From: Paul Barker <paul.barker.ct@...renesas.com>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Sergey Shtylyov <s.shtylyov@....ru>, "David S. Miller"
<davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Geert Uytterhoeven <geert+renesas@...der.be>, netdev@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors
in R-Car RX path
On 14/04/2024 13:08, Niklas Söderlund wrote:
> Hi Paul,
>
> Thanks for your patch.
>
> On 2024-04-11 12:44:30 +0100, Paul Barker wrote:
>> The units of "work done" in the RX path should be packets instead of
>> descriptors.
>>
>> Descriptors which are used by the hardware to record error conditions or
>> are empty in the case of a DMA mapping error should not count towards
>> our RX work budget.
>>
>> Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
>> Signed-off-by: Paul Barker <paul.barker.ct@...renesas.com>
>> ---
>> drivers/net/ethernet/renesas/ravb_main.c | 20 ++++++++------------
>> 1 file changed, 8 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index ba01c8cc3c90..70f2900648d4 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -892,29 +892,25 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> struct ravb_private *priv = netdev_priv(ndev);
>> const struct ravb_hw_info *info = priv->info;
>> int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>> - int boguscnt = (priv->dirty_rx[q] + priv->num_rx_ring[q]) -
>> - priv->cur_rx[q];
>> struct net_device_stats *stats = &priv->stats[q];
>> struct ravb_ex_rx_desc *desc;
>> struct sk_buff *skb;
>> dma_addr_t dma_addr;
>> struct timespec64 ts;
>> + int rx_packets = 0;
>> u8 desc_status;
>> u16 pkt_len;
>> int limit;
>> + int i;
>
> The loop variable can never be negative, use unsigned int.
I matched the type we're comparing against - should we also convert
limit to an unsigned int?
>
>>
>> - boguscnt = min(boguscnt, *quota);
>> - limit = boguscnt;
>> + limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> desc = &priv->rx_ring[q].ex_desc[entry];
>> - while (desc->die_dt != DT_FEMPTY) {
>> + for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
>> /* Descriptor type must be checked before all other reads */
>> dma_rmb();
>> desc_status = desc->msc;
>> pkt_len = le16_to_cpu(desc->ds_cc) & RX_DS;
>>
>> - if (--boguscnt < 0)
>> - break;
>> -
>
> nit: It's a matter of taste, but I like this break condition in the code
> instead of modifying the loop as it's much clearer what's going on. But
> feel free to keep it as is as Sergey likes it.
>
>> /* We use 0-byte descriptors to mark the DMA mapping errors */
>> if (!pkt_len)
>> continue;
>> @@ -960,7 +956,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> if (ndev->features & NETIF_F_RXCSUM)
>> ravb_rx_csum(skb);
>> napi_gro_receive(&priv->napi[q], skb);
>> - stats->rx_packets++;
>> + rx_packets++;
>
> Why do you add this intermediary variable? Is it not confusing to treat
> rx_packets and rx_bytes differently? Why not instead decrement *quota
> here?
To me, it's simpler to count received packets once instead of twice
inside the loop (once by incrementing stats->rx_packets, a second time
by decrementing *quota). This also makes future refactoring simpler as
we already have the rx_packets count which we will need to be able to
return so that we can properly track work done in ravb_poll().
Thanks,
--
Paul Barker
Download attachment "OpenPGP_0x27F4B3459F002257.asc" of type "application/pgp-keys" (3521 bytes)
Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (237 bytes)
Powered by blists - more mailing lists