| Message ID | 20240411114434.26186-2-paul.barker.ct@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ravb Ethernet driver bugfixes | expand |
On 4/11/24 2:44 PM, 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> Reviewed-by: Sergey Shtylyov <s.shtylyov@omp.ru> [...] MBR, Sergey
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. > > - 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? > stats->rx_bytes += pkt_len; > } > > @@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q) > desc->die_dt = DT_FEMPTY; > } > > - *quota -= limit - (++boguscnt); > - > - return boguscnt <= 0; > + stats->rx_packets += rx_packets; > + *quota -= rx_packets; > + return *quota == 0; > } > > /* Packet receive function for Ethernet AVB */ > -- > 2.39.2 >
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? > >> >> - 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,
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
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; - 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; - /* 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++; stats->rx_bytes += pkt_len; } @@ -995,9 +991,9 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q) desc->die_dt = DT_FEMPTY; } - *quota -= limit - (++boguscnt); - - return boguscnt <= 0; + stats->rx_packets += rx_packets; + *quota -= rx_packets; + return *quota == 0; } /* Packet receive function for Ethernet AVB */
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(-)