Re: [Intel-wired-lan] [PATCH net v3 2/4] iavf: Don't lock rtnl_lock twice in reset

[Jacob Keller <jacob.e.keller@intel.com>]

On 4/19/2023 4:50 AM, Kamil Maziarz wrote:
> From: Marcin Szycik <marcin.szycik@linux.intel.com>
> 
> Some ndo/ethtool callbacks are called under rtnl_lock. If such callback
> then triggers a reset, the reset task might try to take the rtnl_lock
> again, causing a deadlock.
> 
> Callbacks that are sensitive to rtnl_lock are scheduled when the drivers
> are unable to obtain the rtnl_lock in the reset flow. This ensures that
> the reset task does not attempt to double lock rtnl_lock and avoids
> the deadlock.
> 
> Before this patch, a deadlock could be caused by e.g.:
> 
> echo 1 > /sys/class/net/$PF1/device/sriov_numvfs
> while :; do
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 8
> ip l s $VF1 down
> ip l s $VF1 up
> ethtool --set-channels $VF1 combined 16
> ip l s $VF1 down
> done
> 
> Fixes: aa626da947e9 ("iavf: Detach device during reset task")
> Signed-off-by: Marcin Szycik <marcin.szycik@linux.intel.com>
> Co-developed-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Dawid Wesierski <dawidx.wesierski@intel.com>
> Signed-off-by: Kamil Maziarz <kamil.maziarz@intel.com>
> ---
> v2: no changes
> ---
> v3: Instead of the flag, we are using the rtnl_trylock in the rtnl_lock sensitive functions in the iavf_reset to avoid deadlock.
>     Adding the scheduling functionality so we can update the netdev later in that case.
>  out:
> -	netif_set_real_num_rx_queues(adapter->netdev, pairs);
> -	netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +	if (rtnl_trylock()) {
> +		netif_set_real_num_rx_queues(adapter->netdev, pairs);
> +		netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +		rtnl_unlock();
> +	} else {
> +		queue_work(adapter->wq, &adapter->set_interrupt_capability);
> +	}
> +

I guess there's some merit to doing it inline when possible, but I'd
argue for simplicity of just always scheduling the work item. The
trylock is at least safe. I'm ok with either approach.

>  	return err;
>  }
>  
> +/**
> + * iavf_delayed_set_interrupt_capability - schedule the update of the netdev
> + * @work: pointer to work_struct containing our data
> + **/
> +static void iavf_delayed_set_interrupt_capability(struct work_struct *work)
> +{
> +	struct iavf_adapter *adapter = container_of(work, struct iavf_adapter,
> +                                                   set_interrupt_capability);
> +	int pairs = adapter->num_active_queues;
> +
> +	if(rtnl_trylock()) {
> +		netif_set_real_num_rx_queues(adapter->netdev, pairs);
> +		netif_set_real_num_tx_queues(adapter->netdev, pairs);
> +		rtnl_unlock();
> +	} else {
> +		queue_work(adapter->wq, &adapter->set_interrupt_capability);
> +	}
> +}

The delayed function is now in a thread where its safe to take RTNL
lock, (since no one is waiting on a different lock/lock bit/etc) so this
should just be "rtnl_lock() / *set_real_num* / rtnl_unlock(). I don't
see the benefit of re-queueing here. The update won't take long so I
don't think we're going to be starving the adapter work queue by locking
on it..
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan