* [PATCH 1/2] net: ravb: Always process TX descriptor ring
@ 2024-03-26 8:37 Paul Barker
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Paul Barker @ 2024-03-26 8:37 UTC (permalink / raw
To: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Paul Barker, Niklas Söderlund, netdev, linux-renesas-soc,
linux-kernel
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, "a, q))
- goto out;
+ rearm = !ravb_rx(ndev, "a, 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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] net: ravb: Always update error counters
2024-03-26 8:37 [PATCH 1/2] net: ravb: Always process TX descriptor ring Paul Barker
@ 2024-03-26 8:37 ` Paul Barker
2024-03-26 9:42 ` Niklas Söderlund
2024-03-26 20:45 ` Sergey Shtylyov
2024-03-26 9:38 ` [PATCH 1/2] net: ravb: Always process TX descriptor ring Niklas Söderlund
2024-03-27 20:59 ` Sergey Shtylyov
2 siblings, 2 replies; 8+ messages in thread
From: Paul Barker @ 2024-03-26 8:37 UTC (permalink / raw
To: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Paul Barker, Niklas Söderlund, netdev, linux-renesas-soc,
linux-kernel
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] net: ravb: Always process TX descriptor ring
2024-03-26 8:37 [PATCH 1/2] net: ravb: Always process TX descriptor ring Paul Barker
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
@ 2024-03-26 9:38 ` Niklas Söderlund
2024-03-26 9:54 ` Paul Barker
2024-03-27 20:59 ` Sergey Shtylyov
2 siblings, 1 reply; 8+ messages in thread
From: Niklas Söderlund @ 2024-03-26 9:38 UTC (permalink / raw
To: Paul Barker
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-renesas-soc, linux-kernel
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, "a, q))
> - goto out;
> + rearm = !ravb_rx(ndev, "a, 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] net: ravb: Always update error counters
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
@ 2024-03-26 9:42 ` Niklas Söderlund
2024-03-26 20:45 ` Sergey Shtylyov
1 sibling, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2024-03-26 9:42 UTC (permalink / raw
To: Paul Barker
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] net: ravb: Always process TX descriptor ring
2024-03-26 9:38 ` [PATCH 1/2] net: ravb: Always process TX descriptor ring Niklas Söderlund
@ 2024-03-26 9:54 ` Paul Barker
2024-03-26 13:09 ` Niklas Söderlund
0 siblings, 1 reply; 8+ messages in thread
From: Paul Barker @ 2024-03-26 9:54 UTC (permalink / raw
To: Niklas Söderlund
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-renesas-soc, linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] net: ravb: Always process TX descriptor ring
2024-03-26 9:54 ` Paul Barker
@ 2024-03-26 13:09 ` Niklas Söderlund
0 siblings, 0 replies; 8+ messages in thread
From: Niklas Söderlund @ 2024-03-26 13:09 UTC (permalink / raw
To: Paul Barker
Cc: Sergey Shtylyov, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, netdev, linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] net: ravb: Always update error counters
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
2024-03-26 9:42 ` Niklas Söderlund
@ 2024-03-26 20:45 ` Sergey Shtylyov
1 sibling, 0 replies; 8+ messages in thread
From: Sergey Shtylyov @ 2024-03-26 20:45 UTC (permalink / raw
To: Paul Barker, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Niklas Söderlund, netdev, linux-renesas-soc, linux-kernel
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] net: ravb: Always process TX descriptor ring
2024-03-26 8:37 [PATCH 1/2] net: ravb: Always process TX descriptor ring Paul Barker
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
2024-03-26 9:38 ` [PATCH 1/2] net: ravb: Always process TX descriptor ring Niklas Söderlund
@ 2024-03-27 20:59 ` Sergey Shtylyov
2 siblings, 0 replies; 8+ messages in thread
From: Sergey Shtylyov @ 2024-03-27 20:59 UTC (permalink / raw
To: Paul Barker, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Niklas Söderlund, netdev, linux-renesas-soc, linux-kernel
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, "a, q))
> - goto out;
> + rearm = !ravb_rx(ndev, "a, q);
>
> /* Processing TX Descriptor Ring */
> spin_lock_irqsave(&priv->lock, flags);
[...]
MBR, Sergey
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-03-27 21:00 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-26 8:37 [PATCH 1/2] net: ravb: Always process TX descriptor ring Paul Barker
2024-03-26 8:37 ` [PATCH 2/2] net: ravb: Always update error counters Paul Barker
2024-03-26 9:42 ` Niklas Söderlund
2024-03-26 20:45 ` Sergey Shtylyov
2024-03-26 9:38 ` [PATCH 1/2] net: ravb: Always process TX descriptor ring Niklas Söderlund
2024-03-26 9:54 ` Paul Barker
2024-03-26 13:09 ` Niklas Söderlund
2024-03-27 20:59 ` Sergey Shtylyov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).