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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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
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/[email protected]/
https://lore.kernel.org/netdev/[email protected]/
I got the wrong fixes tag though, it should be:
Fixes: c156633f1353 ("Renesas Ethernet AVB driver proper")
Thanks,
--
Paul Barker
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/[email protected]/
> https://lore.kernel.org/netdev/[email protected]/
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
[...]
> 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