Received-SPF: pass (google.com: domain of linux-kernel+bounces-144851-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Feedback-ID: i80c9496c:Fastmail
Date: Mon, 15 Apr 2024 11:44:40 +0200
From: Niklas =?utf-8?Q?S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se>
To: Paul Barker <paul.barker.ct@bp.renesas.com>
Cc: Sergey Shtylyov <s.shtylyov@omp.ru>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 1/4] net: ravb: Count packets instead of descriptors
 in R-Car RX path
Message-ID: <20240415094440.GB3156415@ragnatech.se>
References: <20240411114434.26186-1-paul.barker.ct@bp.renesas.com>
 <20240411114434.26186-2-paul.barker.ct@bp.renesas.com>
 <20240414120843.GA2860391@ragnatech.se>
 <e3e3c75f-9aee-40b6-b5b8-08260a8bac06@bp.renesas.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e3e3c75f-9aee-40b6-b5b8-08260a8bac06@bp.renesas.com>

Hi Paul,

On 2024-04-15 08:04:05 +0100, Paul Barker wrote:
> 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@bp.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?

If it can't be negative I think that is a good idea.

> 
> > 
> >>  
> >> -	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().

I see your point, I think my point was made with the R-Car code path in 
mind as it do not yet support splitting a packet over multiple 
descriptors. And I agree there is value in trying to keep the two code 
paths as close together as possible so we eventually can merge them.

With the unsigned issue above fixed,

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> 
> Thanks,
> 
> -- 
> Paul Barker


-- 
Kind Regards,
Niklas Söderlund