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

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

[Heiner Kallweit]

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
---
 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

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

[Vladimir Oltean]

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

> ---
>  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?

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

[Heiner Kallweit]

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.

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

[Vladimir Oltean]

On Mon, Nov 02, 2020 at 09:01:00AM +0100, Heiner Kallweit wrote:
> 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.

When you say that "it doesn't make sense", is there something that is
actually measurably worse when the hardirq handler gets force-threaded?
Rephrased, is it something that doesn't make sense in principle, or in
practice?

My understanding is that this is not where the bulk of the NAPI poll
processing is done anyway, so it should not have a severe negative
impact on performance in any case.

On the other hand, moving as much code as possible outside interrupt
context (be it hardirq or softirq) is beneficial to some use cases,
because the scheduler is not in control of that code's runtime unless it
is in a thread.

> The actual discussion would be how to make the NAPI processing a
> thread (instead softirq).

I don't get it, so you prefer the hardirq handler to consume CPU time
which is not accounted for by the scheduler, but for the NAPI poll, you
do want the scheduler to account for it? So why one but not the other?

> 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.

Yes, probably not, hence my question.

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

[Heiner Kallweit]

On 02.11.2020 13:41, Vladimir Oltean wrote:
> On Mon, Nov 02, 2020 at 09:01:00AM +0100, Heiner Kallweit wrote:
>> 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.
> 
> When you say that "it doesn't make sense", is there something that is
> actually measurably worse when the hardirq handler gets force-threaded?
> Rephrased, is it something that doesn't make sense in principle, or in
> practice?
> 
> My understanding is that this is not where the bulk of the NAPI poll
> processing is done anyway, so it should not have a severe negative
> impact on performance in any case.
> 
> On the other hand, moving as much code as possible outside interrupt
> context (be it hardirq or softirq) is beneficial to some use cases,
> because the scheduler is not in control of that code's runtime unless it
> is in a thread.
> 

According to my understanding the point is that executing the simple
hard irq handler for NAPI drivers doesn't cost significantly more than
executing the default hard irq handler (irq_default_primary_handler).
Therefore threadifying it means more or less just overhead.

forced threading:
1. irq_default_primary_handler, wakes irq thread
2. threadified driver hard irq handler (basically just calling napi_schedule)
3. NAPI processing

IRQF_NO_THREAD:
1. driver hard irq handler, scheduling NAPI
2. NAPI processing

>> The actual discussion would be how to make the NAPI processing a
>> thread (instead softirq).
> 
> I don't get it, so you prefer the hardirq handler to consume CPU time
> which is not accounted for by the scheduler, but for the NAPI poll, you
> do want the scheduler to account for it? So why one but not the other?
> 

The CPU time for scheduling NAPI is neglectable, but doing all the
rx and tx work in NAPI processing is significant effort.

>> 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.
> 
> Yes, probably not, hence my question.
> 

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

[Vladimir Oltean]

On Mon, Nov 02, 2020 at 04:18:07PM +0100, Heiner Kallweit wrote:
> According to my understanding the point is that executing the simple
> hard irq handler for NAPI drivers doesn't cost significantly more than
> executing the default hard irq handler (irq_default_primary_handler).
> Therefore threadifying it means more or less just overhead.

If that is really true, then sure. You could probably run a cyclictest
under a ping flood just to make sure though.

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

[Jakub Kicinski]

On Sun, 1 Nov 2020 23:30:44 +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

Applied, based on my understanding on RT. But I do agree with Vladimir
that it'd be good to see actual performance numbers for these sort of
optimizations.