netdev - Re: [PATCH RFC net-next] e1000e: Fix real-time violations on link up

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f5515472-21d0-4a56-a6a1-8431c1d60f7e@engleder-embedded.com>
Date: Mon, 14 Oct 2024 19:59:07 +0200
From: Gerhard Engleder <gerhard@...leder-embedded.com>
To: Andrew Lunn <andrew@...n.ch>, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
 pabeni@...hat.com, intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
 Gerhard Engleder <eg@...a.com>
Subject: Re: [PATCH RFC net-next] e1000e: Fix real-time violations on link up

On 12.10.24 20:42, Andrew Lunn wrote:
> On Fri, Oct 11, 2024 at 09:54:12PM +0200, Gerhard Engleder wrote:
>> From: Gerhard Engleder <eg@...a.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. The result are timing violations on real-time
>> systems during link down and up of e1000e.
>>
>> Execute a flush after every single write. This prevents overloading the
>> interconnect with posted writes. As this also increases the time spent for
>> MTA table update considerable this change is limited to PREEMPT_RT.
>>
>> Signed-off-by: Gerhard Engleder <eg@...a.com>
>> ---
>>   drivers/net/ethernet/intel/e1000e/mac.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/e1000e/mac.c b/drivers/net/ethernet/intel/e1000e/mac.c
>> index d7df2a0ed629..f4693d355886 100644
>> --- a/drivers/net/ethernet/intel/e1000e/mac.c
>> +++ b/drivers/net/ethernet/intel/e1000e/mac.c
>> @@ -331,9 +331,15 @@ 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]);
>> +#ifdef CONFIG_PREEMPT_RT
>> +		e1e_flush();
>> +#endif
>> +	}
>> +#ifndef CONFIG_PREEMPT_RT
>>   	e1e_flush();
>> +#endif
> 
> #ifdef FOO is generally not liked because it reduces the effectiveness
> of build testing.
> 
> Two suggestions:
> 
> 	if (IS_ENABLED(CONFIG_PREEMPT_RT))
> 		e1e_flush();

I will do that.

> This will then end up as and if (0) or if (1), with the statement
> following it always being compiled, and then optimised out if not
> needed.
> 
> Alternatively, consider something like:
> 
> 	if (i % 8)
> 		e1e_flush()
> 
> if there is a reasonable compromise between RT and none RT
> performance. Given that RT is now fully merged, we might see some
> distros enable it, so a compromise would probably be better.

Yes, read/flush after every posted write is likely too much. I will
do some testing how often flush is required.

Thank you for your feedback Andrew!

Any comments from Intel driver maintainers?

Gerhard