Re: [Intel-wired-lan] [PATCH iwl-net v1] igc: Fix infinite initialization loop with early XDP redirect

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: Andre Guedes <andre.guedes@intel.com>,
	Ferenc Fejes <ferenc.fejes@ericsson.com>,
	netdev@vger.kernel.org,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	linux-kernel@vger.kernel.org,
	Jithu Joseph <jithu.joseph@intel.com>,
	Eric Dumazet <edumazet@google.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	Jakub Kicinski <kuba@kernel.org>,
	intel-wired-lan@lists.osuosl.org, Paolo Abeni <pabeni@redhat.com>,
	"David S. Miller" <davem@davemloft.net>,
	Vedang Patel <vedang.patel@intel.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-net v1] igc: Fix infinite initialization loop with early XDP redirect
Date: Wed, 06 Sep 2023 16:37:34 -0700	[thread overview]
Message-ID: <877cp2rhz5.fsf@intel.com> (raw)
In-Reply-To: <ZPkDaLo4ubFRpPg3@boxer>

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

> On Tue, Sep 05, 2023 at 02:37:52PM -0700, Vinicius Costa Gomes wrote:
>> When a XDP redirect happens before the link is ready, that
>
> When exactly link was 'ready' in your setup? You said it was enough to
> launch traffic towards igc iface before running xdp-bench. Was the iface
> down or up or?

In short, the interface was up and it was brought down "externally".

I should have explained my test better: A is the system under test, B is
the monitor; 1. initially the link between systems A and B is up; 2. I
setup the vlans and xdp-bench; 3. I start sending traffic; 4. on system
B, I brind the NIC connected to A down; 5. infinite initialization loop.

>
>> transmission will not finish and will timeout, causing an adapter
>> reset. If the redirects do not stop, the adapter will not stop
>> resetting.
>
> Please highlight that this driver shares tx resources with netstack. I
> believe the source of this bug is that the watchdog is responsible to call
> netif_carrier_on() from a workqueue which happens to be scheduled *after*
> clearing __IGC_DOWN in igc_up().
>

Sure, will add this information to the commit message and send a v2.

>> 
>> Wait for the driver to signal that there's a carrier before allowing
>> transmissions to proceed.
>> 
>> Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action")
>> Reported-by: Ferenc Fejes <ferenc.fejes@ericsson.com>
>> Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@ericsson.com/
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Tested-by: Ferenc Fejes <ferenc.fejes@ericsson.com>
>> ---
>>  drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> index 293b45717683..98de34d0ce07 100644
>> --- a/drivers/net/ethernet/intel/igc/igc_main.c
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -6491,7 +6491,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames,
>>  	struct igc_ring *ring;
>>  	int i, drops;
>>  
>> -	if (unlikely(test_bit(__IGC_DOWN, &adapter->state)))
>> +	if (unlikely(!netif_carrier_ok(dev)))
>>  		return -ENETDOWN;
>
> I thought about keeping the bit check as well but given what i wrote above
> it is probably redundant, so:
>
> Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
>
>>  
>>  	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>> -- 
>> 2.41.0
>> 

Cheers,
-- 
Vinicius
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan