RE: [net-next,v12,1/7] r8169: add support for multi irqs

[Javen]

>
>> +static int rtl8169_request_irq(struct rtl8169_private *tp) {
>> +     struct net_device *dev = tp->dev;
>> +     struct napi_struct *napi;
>> +     int i, rc;
>> +
>> +     for (i = 0; i < tp->irq_nvecs; i++) {
>> +             napi = &tp->rtl8169_napi[i];
>> +             rc = pci_request_irq(tp->pci_dev, i, rtl8169_interrupt,
>> +                                  NULL, napi, "%s-%d", dev->name, i);
>> +             if (rc)
>> +                     goto free_irq;
>> +     }
>> +     return 0;
>
>[ ... ]
>
>> @@ -5107,8 +5162,7 @@ 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(tp->irq, rtl8169_interrupt, irqflags, dev->name, tp);
>> +     retval = rtl8169_request_irq(tp);
>>       if (retval < 0)
>>               goto err_release_fw_2;
>
>[Severity: Medium]
>Does this drop the IRQF_NO_THREAD that commit 870f531e178e ("r8169: set
>IRQF_NO_THREAD if MSI(X) is enabled") added?
>
>pci_request_irq() hardcodes the flags and has no way to express
>IRQF_NO_THREAD:
>
>drivers/pci/irq.c:pci_request_irq() {
>        unsigned long irqflags = IRQF_SHARED;
>
>        if (!handler)
>                irqflags |= IRQF_ONESHOT;
>        ...
>        ret = request_threaded_irq(pci_irq_vector(dev, nr), handler, thread_fn,
>                                   irqflags, devname, dev_id); }
>
>With handler != NULL the flags are exactly IRQF_SHARED, so on a kernel
>booted with threadirqs, or with CONFIG_PREEMPT_RT, __setup_irq() reaches:

Hi Heiner,

In previous revisions, I was suggested to use the new api pci_request_irq(). However, since it hardcodes IRQF_SHARED and does not accept custom flags, it drops IRQF_NO_THREAD for MSI/MSI-X. 

To avoid this performance regression detected by ai, should I revert to using request_irq() + pci_irq_vector() in v13? Could you please advise on the best approach for v13?

Thanks,
Javen

>
>kernel/irq/manage.c:irq_setup_forced_threading() {
>        if (new->flags & (IRQF_NO_THREAD | IRQF_PERCPU | IRQF_ONESHOT))
>                return 0;
>        ...
>        new->flags |= IRQF_ONESHOT;
>        ...
>        new->thread_fn = new->handler;
>        new->handler = irq_default_primary_handler; }
>
>so rtl8169_interrupt() becomes force-threaded and the vector is masked until
>the thread runs, which is what 870f531e178e set out to avoid.
>
>Two related observations on the same change: MSI and MSI-X vectors are now
>requested as IRQF_SHARED even though they are not shareable, and the name
>registered in /proc/interrupts changes from "<ifname>" to "<ifname>-<N>".
>Should the changelog mention the name change?
>
>> @@ -5074,7 +5127,10 @@ static void rtl8169_netpoll(struct net_device
>> *dev)  {
>>       struct rtl8169_private *tp = netdev_priv(dev);
>>
>> -     rtl8169_interrupt(tp->irq, tp);
>> +     for (int i = 0; i < tp->irq_nvecs; i++) {
>> +             rtl8169_interrupt(pci_irq_vector(tp->pci_dev, i),
>> +                               &tp->rtl8169_napi[i]);
>> +     }
>>  }
>>  #endif