[PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[Gerhard Engleder]

From: Gerhard Engleder <eg@keba.com>

From: Gerhard Engleder <eg@keba.com>

Link down and up triggers update of MTA table. This update executes many
PCIe writes and a final flush. Thus, PCIe will be blocked until all writes
are flushed. As a result, DMA transfers of other targets suffer from delay
in the range of 50us. This results in timing violations on real-time
systems during link down and up of e1000e.

A flush after a low enough number of PCIe writes eliminates the delay
but also increases the time needed for MTA table update. The following
measurements were done on i3-2310E with e1000e for 128 MTA table entries:

Single flush after all writes: 106us
Flush after every write:       429us
Flush after every 2nd write:   266us
Flush after every 4th write:   180us
Flush after every 8th write:   141us
Flush after every 16th write:  121us

A flush after every 8th write delays the link up by 35us and the
negative impact to DMA transfers of other targets is still tolerable.

Execute a flush after every 8th write. This prevents overloading the
interconnect with posted writes.

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
CC: Vitaly Lifshits <vitaly.lifshits@intel.com>
Link: https://lore.kernel.org/netdev/f8fe665a-5e6c-4f95-b47a-2f3281aa0e6c@lunn.ch/T/
Signed-off-by: Gerhard Engleder <eg@keba.com>
---
v2:
- remove PREEMPT_RT dependency (Andrew Lunn, Przemek Kitszel)
---
 drivers/net/ethernet/intel/e1000e/mac.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c
index d7df2a0ed629..7d1482a9effd 100644
--- a/drivers/net/ethernet/intel/e1000e/mac.c
+++ b/drivers/net/ethernet/intel/e1000e/mac.c
@@ -331,8 +331,13 @@ void e1000e_update_mc_addr_list_generic(struct e1000_hw *hw,
 	}
 
 	/* replace the entire MTA table */
-	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
+	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
 		E1000_WRITE_REG_ARRAY(hw, E1000_MTA, i, hw->mac.mta_shadow[i]);
+
+		/* do not queue up too many writes */
+		if ((i % 8) == 0 && i != 0)
+			e1e_flush();
+	}
 	e1e_flush();
 }
 
-- 
2.39.2

Re: [Intel-wired-lan] [PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[Paul Menzel]

[Cc: +PCI folks]

Dear Gerhard,


Thank you for your patch.


Am 08.12.24 um 19:49 schrieb Gerhard Engleder:
> From: Gerhard Engleder <eg@keba.com>
> 
> From: Gerhard Engleder <eg@keba.com>

The from line is present twice. No idea, if git is going to remove both.

> Link down and up triggers update of MTA table. This update executes many
> PCIe writes and a final flush. Thus, PCIe will be blocked until all writes
> are flushed. As a result, DMA transfers of other targets suffer from delay
> in the range of 50us. This results in timing violations on real-time
> systems during link down and up of e1000e.
> 
> A flush after a low enough number of PCIe writes eliminates the delay
> but also increases the time needed for MTA table update. The following
> measurements were done on i3-2310E with e1000e for 128 MTA table entries:
> 
> Single flush after all writes: 106us
> Flush after every write:       429us
> Flush after every 2nd write:   266us
> Flush after every 4th write:   180us
> Flush after every 8th write:   141us
> Flush after every 16th write:  121us
> 
> A flush after every 8th write delays the link up by 35us and the
> negative impact to DMA transfers of other targets is still tolerable.
> 
> Execute a flush after every 8th write. This prevents overloading the
> interconnect with posted writes.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> CC: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Link: https://lore.kernel.org/netdev/f8fe665a-5e6c-4f95-b47a-2f3281aa0e6c@lunn.ch/T/
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> ---
> v2:
> - remove PREEMPT_RT dependency (Andrew Lunn, Przemek Kitszel)
> ---
>   drivers/net/ethernet/intel/e1000e/mac.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c
> index d7df2a0ed629..7d1482a9effd 100644
> --- a/drivers/net/ethernet/intel/e1000e/mac.c
> +++ b/drivers/net/ethernet/intel/e1000e/mac.c
> @@ -331,8 +331,13 @@ void e1000e_update_mc_addr_list_generic(struct e1000_hw *hw,
>   	}
>   
>   	/* replace the entire MTA table */
> -	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
> +	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
>   		E1000_WRITE_REG_ARRAY(hw, E1000_MTA, i, hw->mac.mta_shadow[i]);
> +
> +		/* do not queue up too many writes */

Maybe make the comment more elaborate?

> +		if ((i % 8) == 0 && i != 0)
> +			e1e_flush();
> +	}
>   	e1e_flush();
>   }


Kind regards,

Paul

Re: [Intel-wired-lan] [PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[Bjorn Helgaas]

On Sun, Dec 08, 2024 at 07:49:50PM +0100, Gerhard Engleder wrote:
> Link down and up triggers update of MTA table. This update executes many
> PCIe writes and a final flush. Thus, PCIe will be blocked until all writes
> are flushed. As a result, DMA transfers of other targets suffer from delay
> in the range of 50us. This results in timing violations on real-time
> systems during link down and up of e1000e.

These look like PCIe memory writes (not config or I/O writes), which
are posted and do not require Completions.  Generally devices should
not delay acceptance of posted requests for more than 10us (PCIe r6.0,
sec 2.3.1).

Since you mention DMA to/from other targets, maybe there's some kind
of fairness issue in the interconnect, which would suggest a
platform-specific issue that could happen with devices other than
e1000e.

I think it would be useful to get to the root cause of this, or at
least mention the interconnect design where you saw the problem in
case somebody trips over this issue with other devices.

The PCIe spec does have an implementation note that says drivers might
need to restrict the programming model as you do here for designs that
can't process posted requests fast enough.  If that's the case for
e1000e, I would ask Intel whether other related devices might also be
affected.

> A flush after a low enough number of PCIe writes eliminates the delay
> but also increases the time needed for MTA table update. The following
> measurements were done on i3-2310E with e1000e for 128 MTA table entries:
> 
> Single flush after all writes: 106us
> Flush after every write:       429us
> Flush after every 2nd write:   266us
> Flush after every 4th write:   180us
> Flush after every 8th write:   141us
> Flush after every 16th write:  121us
> 
> A flush after every 8th write delays the link up by 35us and the
> negative impact to DMA transfers of other targets is still tolerable.
> 
> Execute a flush after every 8th write. This prevents overloading the
> interconnect with posted writes.
> 
> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
> CC: Vitaly Lifshits <vitaly.lifshits@intel.com>
> Link: https://lore.kernel.org/netdev/f8fe665a-5e6c-4f95-b47a-2f3281aa0e6c@lunn.ch/T/
> Signed-off-by: Gerhard Engleder <eg@keba.com>
> ---
> v2:
> - remove PREEMPT_RT dependency (Andrew Lunn, Przemek Kitszel)
> ---
>  drivers/net/ethernet/intel/e1000e/mac.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c
> index d7df2a0ed629..7d1482a9effd 100644
> --- a/drivers/net/ethernet/intel/e1000e/mac.c
> +++ b/drivers/net/ethernet/intel/e1000e/mac.c
> @@ -331,8 +331,13 @@ void e1000e_update_mc_addr_list_generic(struct e1000_hw *hw,
>  	}
>  
>  	/* replace the entire MTA table */
> -	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
> +	for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
>  		E1000_WRITE_REG_ARRAY(hw, E1000_MTA, i, hw->mac.mta_shadow[i]);
> +
> +		/* do not queue up too many writes */
> +		if ((i % 8) == 0 && i != 0)
> +			e1e_flush();
> +	}
>  	e1e_flush();
>  }
>  
> -- 
> 2.39.2
> 

Re: [Intel-wired-lan] [PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[Gerhard Engleder]

On 09.12.24 12:34, Paul Menzel wrote:
> [Cc: +PCI folks]
> 
> Dear Gerhard,
> 
> 
> Thank you for your patch.
> 
> 
> Am 08.12.24 um 19:49 schrieb Gerhard Engleder:
>> From: Gerhard Engleder <eg@keba.com>
>>
>> From: Gerhard Engleder <eg@keba.com>
> 
> The from line is present twice. No idea, if git is going to remove both.

It seems git send-email is adding this line again automatically.
Will be fixed next time.

>> Link down and up triggers update of MTA table. This update executes many
>> PCIe writes and a final flush. Thus, PCIe will be blocked until all 
>> writes
>> are flushed. As a result, DMA transfers of other targets suffer from 
>> delay
>> in the range of 50us. This results in timing violations on real-time
>> systems during link down and up of e1000e.
>>
>> A flush after a low enough number of PCIe writes eliminates the delay
>> but also increases the time needed for MTA table update. The following
>> measurements were done on i3-2310E with e1000e for 128 MTA table entries:
>>
>> Single flush after all writes: 106us
>> Flush after every write:       429us
>> Flush after every 2nd write:   266us
>> Flush after every 4th write:   180us
>> Flush after every 8th write:   141us
>> Flush after every 16th write:  121us
>>
>> A flush after every 8th write delays the link up by 35us and the
>> negative impact to DMA transfers of other targets is still tolerable.
>>
>> Execute a flush after every 8th write. This prevents overloading the
>> interconnect with posted writes.
>>
>> Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>
>> CC: Vitaly Lifshits <vitaly.lifshits@intel.com>
>> Link: 
>> https://lore.kernel.org/netdev/f8fe665a-5e6c-4f95-b47a-2f3281aa0e6c@lunn.ch/T/
>> Signed-off-by: Gerhard Engleder <eg@keba.com>
>> ---
>> v2:
>> - remove PREEMPT_RT dependency (Andrew Lunn, Przemek Kitszel)
>> ---
>>   drivers/net/ethernet/intel/e1000e/mac.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/mac.c 
>> b/drivers/net/ethernet/intel/e1000e/mac.c
>> index d7df2a0ed629..7d1482a9effd 100644
>> --- a/drivers/net/ethernet/intel/e1000e/mac.c
>> +++ b/drivers/net/ethernet/intel/e1000e/mac.c
>> @@ -331,8 +331,13 @@ void e1000e_update_mc_addr_list_generic(struct 
>> e1000_hw *hw,
>>       }
>>       /* replace the entire MTA table */
>> -    for (i = hw->mac.mta_reg_count - 1; i >= 0; i--)
>> +    for (i = hw->mac.mta_reg_count - 1; i >= 0; i--) {
>>           E1000_WRITE_REG_ARRAY(hw, E1000_MTA, i, hw->mac.mta_shadow[i]);
>> +
>> +        /* do not queue up too many writes */
> 
> Maybe make the comment more elaborate?

I will try to extend based on comments from the other thread.

>> +        if ((i % 8) == 0 && i != 0)
>> +            e1e_flush();
>> +    }
>>       e1e_flush();
>>   }
> 
> 
> Kind regards,
> 
> Paul

Thank you for the review!

Gerhard

Re: [Intel-wired-lan] [PATCH iwl-next v2] e1000e: Fix real-time violations on link up

[Gerhard Engleder]

On 10.12.24 16:27, Bjorn Helgaas wrote:
> On Sun, Dec 08, 2024 at 07:49:50PM +0100, Gerhard Engleder wrote:
>> Link down and up triggers update of MTA table. This update executes many
>> PCIe writes and a final flush. Thus, PCIe will be blocked until all writes
>> are flushed. As a result, DMA transfers of other targets suffer from delay
>> in the range of 50us. This results in timing violations on real-time
>> systems during link down and up of e1000e.
> 
> These look like PCIe memory writes (not config or I/O writes), which
> are posted and do not require Completions.  Generally devices should
> not delay acceptance of posted requests for more than 10us (PCIe r6.0,
> sec 2.3.1).
> 
> Since you mention DMA to/from other targets, maybe there's some kind
> of fairness issue in the interconnect, which would suggest a
> platform-specific issue that could happen with devices other than
> e1000e.
> 
> I think it would be useful to get to the root cause of this, or at
> least mention the interconnect design where you saw the problem in
> case somebody trips over this issue with other devices.

Getting the root cause would be interesting, but this problem happens on
a rather ancient platform: an Intel i3-2310E Ivy Bridge CPU launched in
2011 (which still does its job as robot controller). Intel support does
not answer questions for such old platforms. Even for other timing
issues on the interconnect the Intel support was limited. I will mention
the CPU more explicitly as the platform with this issue.

> The PCIe spec does have an implementation note that says drivers might
> need to restrict the programming model as you do here for designs that
> can't process posted requests fast enough.  If that's the case for
> e1000e, I would ask Intel whether other related devices might also be
> affected.

Even for newer CPUs the Intel support has already ended and this CPU is
not sold by Intel anymore. So I won't get an answer. But our experience
is that limiting the number of posted writes always make sense at least
for real-time. Even for our own FPGA based PCIe target, which is able to
consume posted writes at full speed, we limit the number of posted
writes to reduce negative effects on real-time. This experience was made
with multiple Intel platforms.

Gerhard