Re: [PATCH net-next] r8169: set IRQF_NO_THREAD if MSI(X) is enabled

[Heiner Kallweit <hkallweit1@gmail.com>]

On 02.11.2020 01:06, Vladimir Oltean wrote:
> On Sun, Nov 01, 2020 at 11:30:44PM +0100, Heiner Kallweit wrote:
>> We had to remove flag IRQF_NO_THREAD because it conflicts with shared
>> interrupts in case legacy interrupts are used. Following up on the
>> linked discussion set IRQF_NO_THREAD if MSI or MSI-X is used, because
>> both guarantee that interrupt won't be shared.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> Link: https://www.spinics.net/lists/netdev/msg695341.html
> 
> I am not sure if this utilization of the Link: tag is valid. I think it
> has a well-defined meaning and maintainers use it to provide a link to
> the email where the patch was picked from:
> https://lkml.org/lkml/2011/4/6/421
> 

Thanks for the link. There have been discussions whether to have the
change log of patches as part of the commit message or not, and as part
of this discussion how the Link tag can help. IIRC outcome was:
- Link tag can be used to point to a discussion elaborating on the
  evolution of a patch series
- Link tag can be used to point to a discussion explaining the motivation
  for a change

Having said that it's my understanding that this tag isn't to be used by
the maintainers only. However maintainers may see this differently.

>> ---
>>  drivers/net/ethernet/realtek/r8169_main.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
>> index 319399a03..4d6afaf7c 100644
>> --- a/drivers/net/ethernet/realtek/r8169_main.c
>> +++ b/drivers/net/ethernet/realtek/r8169_main.c
>> @@ -4690,6 +4690,7 @@ static int rtl_open(struct net_device *dev)
>>  {
>>  	struct rtl8169_private *tp = netdev_priv(dev);
>>  	struct pci_dev *pdev = tp->pci_dev;
>> +	unsigned long irqflags;
>>  	int retval = -ENOMEM;
>>  
>>  	pm_runtime_get_sync(&pdev->dev);
>> @@ -4714,8 +4715,9 @@ static int rtl_open(struct net_device *dev)
>>  
>>  	rtl_request_firmware(tp);
>>  
>> +	irqflags = pci_dev_msi_enabled(pdev) ? IRQF_NO_THREAD : IRQF_SHARED;
>>  	retval = request_irq(pci_irq_vector(pdev, 0), rtl8169_interrupt,
>> -			     IRQF_SHARED, dev->name, tp);
>> +			     irqflags, dev->name, tp);
>>  	if (retval < 0)
>>  		goto err_release_fw_2;
>>  
>> -- 
>> 2.29.2
>>
> 
> So all things considered, what do you want to achieve with this change?
> Is there other benefit with disabling force threading of the
> rtl8169_interrupt, or are you still looking to add back the
> napi_schedule_irqoff call?
> 

As mentioned by Eric it doesn't make sense to make the minimal hard irq
handlers used with NAPI a thread. This more contributes to the problem
than to the solution. The change here reflects this. The actual discussion
would be how to make the NAPI processing a thread (instead softirq).

For using napi_schedule_irqoff we most likely need something like
if (pci_dev_msi_enabled(pdev))
	napi_schedule_irqoff(napi);
else
	napi_schedule(napi);
and I doubt that's worth it.