| Message ID | 446cf5b8-dddd-197f-cb96-66783141ade4@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] r8169: set IRQF_NO_THREAD if MSI(X) is enabled | expand |
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?
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.
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.
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. >
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.
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.
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;
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(-)