[v1] net: stmmac: Improve Tx timer arm logic further

[PATCH] net: stmmac: Improve Tx timer arm logic further

Posted by muhammad.nazim.amirul.nazle.asmade@altera.com 2 weeks ago

From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>

Currently hrtimer_start is called even if hrtimer is
active. This is unnecessary and expensive in some targets.
This patch avoids calling hrtimer_start unnecessarily.

Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3591755ea30b..35da51c26248 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3341,12 +3341,14 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
 	 * Try to cancel any timer if napi is scheduled, timer will be armed
 	 * again in the next scheduled napi.
 	 */
-	if (unlikely(!napi_is_scheduled(napi)))
-		hrtimer_start(&tx_q->txtimer,
-			      STMMAC_COAL_TIMER(tx_coal_timer),
-			      HRTIMER_MODE_REL);
-	else
+	if (unlikely(!napi_is_scheduled(napi))) {
+		if (unlikely(!(hrtimer_active(&tx_q->txtimer))))
+			hrtimer_start(&tx_q->txtimer,
+				      STMMAC_COAL_TIMER(tx_coal_timer),
+				      HRTIMER_MODE_REL);
+	} else {
 		hrtimer_try_to_cancel(&tx_q->txtimer);
+	}
 }
 
 /**
-- 
2.43.7

Re: [PATCH] net: stmmac: Improve Tx timer arm logic further

Posted by Andrew Lunn 2 weeks ago

On Sun, May 24, 2026 at 11:16:53PM -0700, muhammad.nazim.amirul.nazle.asmade@altera.com wrote:
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> 
> Currently hrtimer_start is called even if hrtimer is
> active. This is unnecessary and expensive in some targets.
> This patch avoids calling hrtimer_start unnecessarily.
> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3591755ea30b..35da51c26248 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3341,12 +3341,14 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
>  	 * Try to cancel any timer if napi is scheduled, timer will be armed
>  	 * again in the next scheduled napi.
>  	 */
> -	if (unlikely(!napi_is_scheduled(napi)))
> -		hrtimer_start(&tx_q->txtimer,
> -			      STMMAC_COAL_TIMER(tx_coal_timer),
> -			      HRTIMER_MODE_REL);
> -	else
> +	if (unlikely(!napi_is_scheduled(napi))) {
> +		if (unlikely(!(hrtimer_active(&tx_q->txtimer))))
> +			hrtimer_start(&tx_q->txtimer,
> +				      STMMAC_COAL_TIMER(tx_coal_timer),
> +				      HRTIMER_MODE_REL);

Is there a race condition here? I've not thought about it enough to
know if there is/is not.

Also, does this change the meaning of tx_coal_timer?

I think the commit message needs expanding, to cover these issues. For
performance changes, it is also normal to include some performance
numbers, before/after, in the commit message.

    Andrew

---
pw-bot: cr

Re: [PATCH] net: stmmac: Improve Tx timer arm logic further

Posted by Nazle Asmade, Muhammad Nazim Amirul 1 week, 6 days ago

On 25/5/2026 9:46 pm, Andrew Lunn wrote:
> On Sun, May 24, 2026 at 11:16:53PM -0700, muhammad.nazim.amirul.nazle.asmade@altera.com wrote:
>> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
>>
>> Currently hrtimer_start is called even if hrtimer is
>> active. This is unnecessary and expensive in some targets.
>> This patch avoids calling hrtimer_start unnecessarily.
>>
>> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
>> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
>> ---
>>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index 3591755ea30b..35da51c26248 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -3341,12 +3341,14 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
>>   	 * Try to cancel any timer if napi is scheduled, timer will be armed
>>   	 * again in the next scheduled napi.
>>   	 */
>> -	if (unlikely(!napi_is_scheduled(napi)))
>> -		hrtimer_start(&tx_q->txtimer,
>> -			      STMMAC_COAL_TIMER(tx_coal_timer),
>> -			      HRTIMER_MODE_REL);
>> -	else
>> +	if (unlikely(!napi_is_scheduled(napi))) {
>> +		if (unlikely(!(hrtimer_active(&tx_q->txtimer))))
>> +			hrtimer_start(&tx_q->txtimer,
>> +				      STMMAC_COAL_TIMER(tx_coal_timer),
>> +				      HRTIMER_MODE_REL);
> 
> Is there a race condition here? I've not thought about it enough to
> know if there is/is not.
> 
> Also, does this change the meaning of tx_coal_timer?
> 
> I think the commit message needs expanding, to cover these issues. For
> performance changes, it is also normal to include some performance
> numbers, before/after, in the commit message.
> 
>      Andrew
> 
> ---
> pw-bot: cr
Hi Maxime, Andrew

Thanks for the quick review! I already send out v2 which applied all the 
comments.
https://lore.kernel.org/all/20260526051921.14540-1-muhammad.nazim.amirul.nazle.asmade@altera.com/

Re: [PATCH] net: stmmac: Improve Tx timer arm logic further

Posted by Maxime Chevallier 2 weeks ago

Hi,

On 5/25/26 08:16, muhammad.nazim.amirul.nazle.asmade@altera.com wrote:
> From: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> 
> Currently hrtimer_start is called even if hrtimer is
> active. This is unnecessary and expensive in some targets.
> This patch avoids calling hrtimer_start unnecessarily.

This description is a bit lacking on details wrt. the expensiveness
and perf impacts, but I've tested with and without this patch on a
cyclone V with dwmac-socfpga, and I cansee a diff when sending
small UDP packets with :

iperf3 -c <srv> -u -b 0 -l 64

Before this patch, around 45200 pps sent,
after this patch, around 52300 pps sent !

Nice improvement :)

So from my perspective,

Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime


> 
> Signed-off-by: Rohan G Thomas <rohan.g.thomas@intel.com>
> Signed-off-by: Nazim Amirul <muhammad.nazim.amirul.nazle.asmade@altera.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3591755ea30b..35da51c26248 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3341,12 +3341,14 @@ static void stmmac_tx_timer_arm(struct stmmac_priv *priv, u32 queue)
>   	 * Try to cancel any timer if napi is scheduled, timer will be armed
>   	 * again in the next scheduled napi.
>   	 */
> -	if (unlikely(!napi_is_scheduled(napi)))
> -		hrtimer_start(&tx_q->txtimer,
> -			      STMMAC_COAL_TIMER(tx_coal_timer),
> -			      HRTIMER_MODE_REL);
> -	else
> +	if (unlikely(!napi_is_scheduled(napi))) {
> +		if (unlikely(!(hrtimer_active(&tx_q->txtimer))))
> +			hrtimer_start(&tx_q->txtimer,
> +				      STMMAC_COAL_TIMER(tx_coal_timer),
> +				      HRTIMER_MODE_REL);
> +	} else {
>   		hrtimer_try_to_cancel(&tx_q->txtimer);
> +	}
>   }
>   
>   /**