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: Netdev Maintainers
Headers show
Series [1/2] net: ravb: Always process TX descriptor ring | expand

Checks

Context Check Description
netdev/series_format warning Single patches do not need cover letters; Target tree name not specified in the subject
netdev/tree_selection success Guessed tree name to be net-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 944 this patch: 944
netdev/build_tools success No tools touched, skip
netdev/cc_maintainers warning 1 maintainers not CCed: claudiu.beznea.uj@bp.renesas.com
netdev/build_clang success Errors and warnings before: 955 this patch: 955
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 955 this patch: 955
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 23 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-03-27--00-00 (tests: 948)

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 */