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

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

[Paul Barker]

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

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

[PATCH 2/2] net: ravb: Always update error counters

[Paul Barker]

The error statistics should be updated each time the poll function is
called, even if the full RX work budget has been consumed. This prevents
the counts from becoming stuck when RX bandwidth usage is high.

This also ensures that error counters are not updated after we've
re-enabled interrupts as that could result in a race condition.

Also drop an unnecessary space.

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 | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index 4f98e4e2badb..a95703948a36 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1339,6 +1339,15 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	netif_wake_subqueue(ndev, q);
 	spin_unlock_irqrestore(&priv->lock, flags);
 
+	/* Receive error message handling */
+	priv->rx_over_errors =  priv->stats[RAVB_BE].rx_over_errors;
+	if (info->nc_queues)
+		priv->rx_over_errors += priv->stats[RAVB_NC].rx_over_errors;
+	if (priv->rx_over_errors != ndev->stats.rx_over_errors)
+		ndev->stats.rx_over_errors = priv->rx_over_errors;
+	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
+		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
+
 	if (!rearm)
 		goto out;
 
@@ -1355,14 +1364,6 @@ static int ravb_poll(struct napi_struct *napi, int budget)
 	}
 	spin_unlock_irqrestore(&priv->lock, flags);
 
-	/* Receive error message handling */
-	priv->rx_over_errors =  priv->stats[RAVB_BE].rx_over_errors;
-	if (info->nc_queues)
-		priv->rx_over_errors += priv->stats[RAVB_NC].rx_over_errors;
-	if (priv->rx_over_errors != ndev->stats.rx_over_errors)
-		ndev->stats.rx_over_errors = priv->rx_over_errors;
-	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
-		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
 out:
 	return budget - quota;
 }
-- 
2.44.0

Re: [PATCH 2/2] net: ravb: Always update error counters

[Niklas Söderlund]

Hi Paul,

Thanks for your work.

On 2024-03-26 08:37:40 +0000, Paul Barker wrote:
> The error statistics should be updated each time the poll function is
> called, even if the full RX work budget has been consumed. This prevents
> the counts from becoming stuck when RX bandwidth usage is high.
> 
> This also ensures that error counters are not updated after we've
> re-enabled interrupts as that could result in a race condition.
> 
> Also drop an unnecessary space.
> 
> Fixes: a0d2f20650e8 ("Renesas Ethernet AVB PTP clock driver")

Same comment about fixes tag is in patch 1/2.

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4f98e4e2badb..a95703948a36 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1339,6 +1339,15 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	netif_wake_subqueue(ndev, q);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> +	/* Receive error message handling */
> +	priv->rx_over_errors =  priv->stats[RAVB_BE].rx_over_errors;

While you are dropping spaces s/=  priv/= priv/ here.

> +	if (info->nc_queues)
> +		priv->rx_over_errors += priv->stats[RAVB_NC].rx_over_errors;
> +	if (priv->rx_over_errors != ndev->stats.rx_over_errors)
> +		ndev->stats.rx_over_errors = priv->rx_over_errors;
> +	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
> +		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
> +
>  	if (!rearm)
>  		goto out;
>  
> @@ -1355,14 +1364,6 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	}
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> -	/* Receive error message handling */
> -	priv->rx_over_errors =  priv->stats[RAVB_BE].rx_over_errors;
> -	if (info->nc_queues)
> -		priv->rx_over_errors += priv->stats[RAVB_NC].rx_over_errors;
> -	if (priv->rx_over_errors != ndev->stats.rx_over_errors)
> -		ndev->stats.rx_over_errors = priv->rx_over_errors;
> -	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
> -		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
>  out:
>  	return budget - quota;
>  }
> -- 
> 2.44.0
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH 2/2] net: ravb: Always update error counters

[Sergey Shtylyov]

On 3/26/24 11:37 AM, Paul Barker wrote:

> The error statistics should be updated each time the poll function is
> called, even if the full RX work budget has been consumed. This prevents
> the counts from becoming stuck when RX bandwidth usage is high.
> 
> This also ensures that error counters are not updated after we've
> re-enabled interrupts as that could result in a race condition.
> 
> Also drop an unnecessary space.

   Which one? I'm seeing one intact... :-)

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

   As have been already said, it's wrong... :-/

> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 4f98e4e2badb..a95703948a36 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1339,6 +1339,15 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	netif_wake_subqueue(ndev, q);
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
> +	/* Receive error message handling */
> +	priv->rx_over_errors =  priv->stats[RAVB_BE].rx_over_errors;

   So you missed this extra space...

> +	if (info->nc_queues)
> +		priv->rx_over_errors += priv->stats[RAVB_NC].rx_over_errors;
> +	if (priv->rx_over_errors != ndev->stats.rx_over_errors)
> +		ndev->stats.rx_over_errors = priv->rx_over_errors;
> +	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
> +		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
> +
>  	if (!rearm)
>  		goto out;
>  
[...]

MBR, Sergey

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

[Niklas Söderlund]

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
> 

-- 
Kind Regards,
Niklas Söderlund

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

[Paul Barker]

[-- Attachment #1.1.1: Type: text/plain, Size: 1210 bytes --]

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,

-- 
Paul Barker

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3577 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

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

[Niklas Söderlund]

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






-- 
Kind Regards,
Niklas Söderlund

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

[Sergey Shtylyov]

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