diff mbox series

[1/2] net: ravb: Always process TX descriptor ring

Message ID 20240326083740.23364-1-paul.barker.ct@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [1/2] net: ravb: Always process TX descriptor ring | expand

Commit Message

Paul Barker March 26, 2024, 8:37 a.m. UTC 
The TX queue should be serviced each time the poll function is called,
even if the full RX work budget has been consumed. This prevents
starvation of the TX queue when RX bandwidth usage is high.

Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)


base-commit: 4cece764965020c22cff7665b18a012006359095

Comments

Niklas Söderlund March 26, 2024, 9:38 a.m. UTC | #1 
Hi Paul,

Thanks for your work.

On 2024-03-26 08:37:39 +0000, Paul Barker wrote:
> The TX queue should be serviced each time the poll function is called,
> even if the full RX work budget has been consumed. This prevents
> starvation of the TX queue when RX bandwidth usage is high.

Is this not a design decision? That the driver should prioritize Rx over 
Tx if there is contention. I have no opinion on if this design is good 
or bad, I let Sergey judge that.

> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

However, I do not think it is a bug and should not have a fixes tag.  
Also this fixes tag is incorrect, this points to the commit where ravb.c 
was renamed ravb_main.c. ravb_poll() is not touched by this commit.

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index d1be030c8848..4f98e4e2badb 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1324,12 +1324,12 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
>  	int quota = budget;
> +	bool rearm = true;
>  
>  	/* Processing RX Descriptor Ring */
>  	/* Clear RX interrupt */
>  	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> -	if (ravb_rx(ndev, &quota, q))
> -		goto out;
> +	rearm = !ravb_rx(ndev, &quota, q);
>  
>  	/* Processing TX Descriptor Ring */
>  	spin_lock_irqsave(&priv->lock, flags);
> @@ -1339,6 +1339,9 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	netif_wake_subqueue(ndev, q);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> +	if (!rearm)
> +		goto out;
> +
>  	napi_complete(napi);
>  
>  	/* Re-enable RX/TX interrupts */
> 
> base-commit: 4cece764965020c22cff7665b18a012006359095
> -- 
> 2.44.0
>
Paul Barker March 26, 2024, 9:54 a.m. UTC | #2 
On 26/03/2024 09:38, Niklas Söderlund wrote:
> Hi Paul,
> 
> Thanks for your work.
> 
> On 2024-03-26 08:37:39 +0000, Paul Barker wrote:
>> The TX queue should be serviced each time the poll function is called,
>> even if the full RX work budget has been consumed. This prevents
>> starvation of the TX queue when RX bandwidth usage is high.
> 
> Is this not a design decision? That the driver should prioritize Rx over 
> Tx if there is contention. I have no opinion on if this design is good 
> or bad, I let Sergey judge that.
> 
>>
>> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> 
> However, I do not think it is a bug and should not have a fixes tag.  
> Also this fixes tag is incorrect, this points to the commit where ravb.c 
> was renamed ravb_main.c. ravb_poll() is not touched by this commit.

Sergey identified these as bugfixes in the following emails:
  https://lore.kernel.org/netdev/a364963f-4e4f-dba5-cb59-b2125c14e8fc@omp.ru/
  https://lore.kernel.org/netdev/c58ab319-222b-5ab0-0924-7774a473e276@omp.ru/

I got the wrong fixes tag though, it should be:
  Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")

Thanks,
Niklas Söderlund March 26, 2024, 1:09 p.m. UTC | #3 
On 2024-03-26 09:54:04 +0000, Paul Barker wrote:
> On 26/03/2024 09:38, Niklas Söderlund wrote:
> > Hi Paul,
> > 
> > Thanks for your work.
> > 
> > On 2024-03-26 08:37:39 +0000, Paul Barker wrote:
> >> The TX queue should be serviced each time the poll function is called,
> >> even if the full RX work budget has been consumed. This prevents
> >> starvation of the TX queue when RX bandwidth usage is high.
> > 
> > Is this not a design decision? That the driver should prioritize Rx over 
> > Tx if there is contention. I have no opinion on if this design is good 
> > or bad, I let Sergey judge that.
> > 
> >>
> >> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> > 
> > However, I do not think it is a bug and should not have a fixes tag.  
> > Also this fixes tag is incorrect, this points to the commit where ravb.c 
> > was renamed ravb_main.c. ravb_poll() is not touched by this commit.
> 
> Sergey identified these as bugfixes in the following emails:
>   https://lore.kernel.org/netdev/a364963f-4e4f-dba5-cb59-b2125c14e8fc@omp.ru/
>   https://lore.kernel.org/netdev/c58ab319-222b-5ab0-0924-7774a473e276@omp.ru/

I see, I missed that. I do not agree, this is not a bugfix, it changes a 
design decision and the behavior of the driver.

@Sergey: What do you think? If you feel strongly about this being a bug 
I will yield.

> 
> I got the wrong fixes tag though, it should be:
>   Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
> 
> Thanks,
> 
> -- 
> Paul Barker
Sergey Shtylyov March 27, 2024, 8:59 p.m. UTC | #4 
On 3/26/24 11:37 AM, Paul Barker wrote:

> The TX queue should be serviced each time the poll function is called,
> even if the full RX work budget has been consumed. This prevents
> starvation of the TX queue when RX bandwidth usage is high.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>

[...]

> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index d1be030c8848..4f98e4e2badb 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1324,12 +1324,12 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
>  	int quota = budget;
> +	bool rearm = true;

   I don't think we need an initializer, it gets reassigned below.
   And I'd rather call it unmask...

>  
>  	/* Processing RX Descriptor Ring */
>  	/* Clear RX interrupt */
>  	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> -	if (ravb_rx(ndev, &quota, q))
> -		goto out;
> +	rearm = !ravb_rx(ndev, &quota, q);
>  
>  	/* Processing TX Descriptor Ring */
>  	spin_lock_irqsave(&priv->lock, flags);
[...]

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index d1be030c8848..4f98e4e2badb 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1324,12 +1324,12 @@  static int ravb_poll(struct napi_struct *napi, int budget)
 	int q = napi - priv->napi;
 	int mask = BIT(q);
 	int quota = budget;
+	bool rearm = true;
 
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	if (ravb_rx(ndev, &quota, q))
-		goto out;
+	rearm = !ravb_rx(ndev, &quota, q);
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
@@ -1339,6 +1339,9 @@  static int ravb_poll(struct napi_struct *napi, int budget)
 	netif_wake_subqueue(ndev, q);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
+	if (!rearm)
+		goto out;
+
 	napi_complete(napi);
 
 	/* Re-enable RX/TX interrupts */