Re: [PATCH 2/3] igb: Link queues to NAPI instances

[Kurt Kanzenbach <kurt@linutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 4318 bytes --]

On Mon Feb 10 2025, Joe Damato wrote:
> On Mon, Feb 10, 2025 at 10:19:36AM +0100, Kurt Kanzenbach wrote:
>> Link queues to NAPI instances via netdev-genl API. This is required to use
>> XDP/ZC busy polling. See commit 5ef44b3cb43b ("xsk: Bring back busy polling
>> support") for details.
>> 
>> This also allows users to query the info with netlink:
>> 
>> |$ ./tools/net/ynl/pyynl/cli.py --spec Documentation/netlink/specs/netdev.yaml \
>> |                               --dump queue-get --json='{"ifindex": 2}'
>> |[{'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'rx'},
>> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'rx'},
>> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'rx'},
>> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'rx'},
>> | {'id': 0, 'ifindex': 2, 'napi-id': 8201, 'type': 'tx'},
>> | {'id': 1, 'ifindex': 2, 'napi-id': 8202, 'type': 'tx'},
>> | {'id': 2, 'ifindex': 2, 'napi-id': 8203, 'type': 'tx'},
>> | {'id': 3, 'ifindex': 2, 'napi-id': 8204, 'type': 'tx'}]
>> 
>> While at __igb_open() use RCT coding style.
>> 
>> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
>> ---
>>  drivers/net/ethernet/intel/igb/igb.h      |  2 ++
>>  drivers/net/ethernet/intel/igb/igb_main.c | 35 ++++++++++++++++++++++++++-----
>>  drivers/net/ethernet/intel/igb/igb_xsk.c  |  2 ++
>>  3 files changed, 34 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
>> index 02f340280d20a6f7e32bbd3dfcbb9c1c7b4c6662..79eca385a751bfdafdf384928b6cc1b350b22560 100644
>> --- a/drivers/net/ethernet/intel/igb/igb.h
>> +++ b/drivers/net/ethernet/intel/igb/igb.h
>> @@ -722,6 +722,8 @@ enum igb_boards {
>>  
>>  extern char igb_driver_name[];
>>  
>> +void igb_set_queue_napi(struct igb_adapter *adapter, int q_idx,
>> +			struct napi_struct *napi);
>>  int igb_xmit_xdp_ring(struct igb_adapter *adapter,
>>  		      struct igb_ring *ring,
>>  		      struct xdp_frame *xdpf);
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index d4128d19cc08f62f95682069bb5ed9b8bbbf10cb..8e964484f4c9854e4e3e0b4f3e8785fe93bd1207 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -2099,6 +2099,22 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
>>  	wr32(E1000_CTRL_EXT, ctrl_ext);
>>  }
>>  
>> +void igb_set_queue_napi(struct igb_adapter *adapter, int vector,
>> +			struct napi_struct *napi)
>> +{
>> +	struct igb_q_vector *q_vector = adapter->q_vector[vector];
>> +
>> +	if (q_vector->rx.ring)
>> +		netif_queue_set_napi(adapter->netdev,
>> +				     q_vector->rx.ring->queue_index,
>> +				     NETDEV_QUEUE_TYPE_RX, napi);
>> +
>> +	if (q_vector->tx.ring)
>> +		netif_queue_set_napi(adapter->netdev,
>> +				     q_vector->tx.ring->queue_index,
>> +				     NETDEV_QUEUE_TYPE_TX, napi);
>> +}
>> +
>>  /**
>>   *  igb_up - Open the interface and prepare it to handle traffic
>>   *  @adapter: board private structure
>> @@ -2106,6 +2122,7 @@ static void igb_check_swap_media(struct igb_adapter *adapter)
>>  int igb_up(struct igb_adapter *adapter)
>>  {
>>  	struct e1000_hw *hw = &adapter->hw;
>> +	struct napi_struct *napi;
>>  	int i;
>>  
>>  	/* hardware has been reset, we need to reload some things */
>> @@ -2113,8 +2130,11 @@ int igb_up(struct igb_adapter *adapter)
>>  
>>  	clear_bit(__IGB_DOWN, &adapter->state);
>>  
>> -	for (i = 0; i < adapter->num_q_vectors; i++)
>> -		napi_enable(&(adapter->q_vector[i]->napi));
>> +	for (i = 0; i < adapter->num_q_vectors; i++) {
>> +		napi = &adapter->q_vector[i]->napi;
>> +		napi_enable(napi);
>> +		igb_set_queue_napi(adapter, i, napi);
>> +	}
>
> It looks like igb_ub is called from igb_io_resume (struct
> pci_error_handlers). I don't know if RTNL is held in that path. If
> its not, this could trip the ASSERT_RTNL in netif_queue_set_napi.
>
> Can you check and see if this is an issue for that path?

AFAICS the PCI error handlers are called in drivers/pci/pcie/err.c
without RTNL lock held. These function take only the device_lock().

I'll add the missing rtnl_lock()/unlock() calls to igb_io_resume() and
igb_io_error_detected(). Thanks.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]