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@xxxxxxxxxxxxxx>
> >> ---
> >> 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@xxxxxxxxxxxx>
>
> Thanks,
>
> --
> Paul Barker
--
Kind Regards,
Niklas Söderlund
