| Message ID | 20240415094804.8016-3-paul.barker.ct@bp.renesas.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve GbEth performance on Renesas RZ/G2L and related SoCs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-1 |
Hi Paul, Thanks for your work. On 2024-04-15 10:47:59 +0100, Paul Barker wrote: > Call napi_complete_done() in accordance with the documentation in > `Documentation/networking/napi.rst`. The way I understand it napi_complete() is still OK to use, it's just a wrapper for napi_complete_done(napi, 0). But of course using napi_complete_done() is better if you want to use the busypolling status returned. Maybe the commit message can be updated to reflect this change, how about? net: ravb: Consider busypolling status when re-enabling interrupts Make use of the busypolling status returned from NAPI complete to decide if interrupts shall be re-enabled or not. This is useful to reduce the interrupt overhead. While at it switch to using napi_complete_done() as it take into account the work done when providing the busypolling status. > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 26 ++++++++++-------------- > 1 file changed, 11 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 33f8043143c1..1ac599a044b2 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -1340,23 +1340,19 @@ static int ravb_poll(struct napi_struct *napi, int budget) > if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors) > ndev->stats.rx_fifo_errors = priv->rx_fifo_errors; > > - if (work_done == budget) > - goto out; > - > - napi_complete(napi); > - > - /* Re-enable RX/TX interrupts */ > - spin_lock_irqsave(&priv->lock, flags); > - if (!info->irq_en_dis) { > - ravb_modify(ndev, RIC0, mask, mask); > - ravb_modify(ndev, TIC, mask, mask); > - } else { > - ravb_write(ndev, mask, RIE0); > - ravb_write(ndev, mask, TIE); > + if (work_done < budget && napi_complete_done(napi, work_done)) { > + /* Re-enable RX/TX interrupts */ > + spin_lock_irqsave(&priv->lock, flags); > + if (!info->irq_en_dis) { > + ravb_modify(ndev, RIC0, mask, mask); > + ravb_modify(ndev, TIC, mask, mask); > + } else { > + ravb_write(ndev, mask, RIE0); > + ravb_write(ndev, mask, TIE); > + } > + spin_unlock_irqrestore(&priv->lock, flags); > } > - spin_unlock_irqrestore(&priv->lock, flags); > > -out: > return work_done; > } > > -- > 2.39.2 >
On 15/04/2024 12:44, Niklas Söderlund wrote: > Hi Paul, > > Thanks for your work. > > On 2024-04-15 10:47:59 +0100, Paul Barker wrote: >> Call napi_complete_done() in accordance with the documentation in >> `Documentation/networking/napi.rst`. > > The way I understand it napi_complete() is still OK to use, it's just a > wrapper for napi_complete_done(napi, 0). But of course using > napi_complete_done() is better if you want to use the busypolling status > returned. Maybe the commit message can be updated to reflect this > change, how about? > > net: ravb: Consider busypolling status when re-enabling interrupts > > Make use of the busypolling status returned from NAPI complete to decide > if interrupts shall be re-enabled or not. This is useful to reduce the > interrupt overhead. > > While at it switch to using napi_complete_done() as it take into account > the work done when providing the busypolling status. That sounds good to me, especially as the motivation for this change was to support busy polling/software IRQ coalescing. I'll update the commit message in the next version of the series. Thanks,
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 33f8043143c1..1ac599a044b2 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -1340,23 +1340,19 @@ static int ravb_poll(struct napi_struct *napi, int budget) if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors) ndev->stats.rx_fifo_errors = priv->rx_fifo_errors; - if (work_done == budget) - goto out; - - napi_complete(napi); - - /* Re-enable RX/TX interrupts */ - spin_lock_irqsave(&priv->lock, flags); - if (!info->irq_en_dis) { - ravb_modify(ndev, RIC0, mask, mask); - ravb_modify(ndev, TIC, mask, mask); - } else { - ravb_write(ndev, mask, RIE0); - ravb_write(ndev, mask, TIE); + if (work_done < budget && napi_complete_done(napi, work_done)) { + /* Re-enable RX/TX interrupts */ + spin_lock_irqsave(&priv->lock, flags); + if (!info->irq_en_dis) { + ravb_modify(ndev, RIC0, mask, mask); + ravb_modify(ndev, TIC, mask, mask); + } else { + ravb_write(ndev, mask, RIE0); + ravb_write(ndev, mask, TIE); + } + spin_unlock_irqrestore(&priv->lock, flags); } - spin_unlock_irqrestore(&priv->lock, flags); -out: return work_done; }
Call napi_complete_done() in accordance with the documentation in `Documentation/networking/napi.rst`. Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> --- drivers/net/ethernet/renesas/ravb_main.c | 26 ++++++++++-------------- 1 file changed, 11 insertions(+), 15 deletions(-)