Message-ID: <cac09781-e0b3-4646-974d-519184f63a81@nvidia.com>
Date: Thu, 30 May 2024 17:13:45 +0300
From: Shay Drori <shayd@...dia.com>
To: Greg KH <gregkh@...uxfoundation.org>
CC: <netdev@...r.kernel.org>, <pabeni@...hat.com>, <davem@...emloft.net>,
	<kuba@...nel.org>, <edumazet@...gle.com>, <david.m.ertman@...el.com>,
	<rafael@...nel.org>, <ira.weiny@...el.com>, <linux-rdma@...r.kernel.org>,
	<leon@...nel.org>, <tariqt@...dia.com>
Subject: Re: [PATCH net-next v5 0/2] Introduce auxiliary bus IRQs sysfs

Hi Greg,

Did you get a chance to see my reply? Is it ok with you? I would like to
submit the fixes that Parav/Przemek suggested if no further inputs.

Thanks

On 29/05/2024 9:19, Shay Drori wrote:
> 
> 
> On 28/05/2024 20:57, Greg KH wrote:
>> External email: Use caution opening links or attachments
>>
>>
>> On Tue, May 28, 2024 at 12:11:42PM +0300, Shay Drory wrote:
>>> Today, PCI PFs and VFs, which are anchored on the PCI bus, display their
>>> IRQ information in the <pci_device>/msi_irqs/<irq_num> sysfs files. PCI
>>> subfunctions (SFs) are similar to PFs and VFs and these SFs are anchored
>>> on the auxiliary bus. However, these PCI SFs lack such IRQ information
>>> on the auxiliary bus, leaving users without visibility into which IRQs
>>> are used by the SFs. This absence makes it impossible to debug
>>> situations and to understand the source of interrupts/SFs for
>>> performance tuning and debug.
>>
>> Wait, again, this feels wrong.  You should be able to walk back up the
>> tree see the irq for the device, and vf, right?  Why would the value be
>> different down in the aux device?
> 
> 
> you are correct, the IRQs of the aux device are subset of the IRQs of
> the parent device. more detailed answer bellow.
> 
> 
>> Does the msi irq somehow not actually show anywhere for the real pci 
>> device in sysfs at all today?
>>
>> What does sysfs look like today exactly for this information?
> 
> 
> The IRQ information of all the children SFs of a PF is found in the PF
> device as one single list.
> There is no sane way to distinguish which IRQ is used by which SFs.
> For example, in a machine with a single child SF of the PF 00:0b.0 we
> currently have the bellow:
> $ ls /sys/bus/pci/devices/0000:00:0b.0/msi_irqs/
> 41  42  43  44  45  46  47  48  49  50  51  52  53  54  55  56  57  58
> 
> the above are IRQs of both the PF and its child SF. from here we cannot
> tell which IRQ is used by the child SF.
> 
> 
>>
>> And what about /proc/irq/ and /proc/interrupts/ doesn't that show you
>> the needed information?  Why are aux devices somehow "special" here?
> 
> 
> /proc/interrupts interrupt name is some random driver choice. There is
> no sane naming convention and some small few bytes of upper limit of
> what the IRQ name.
> 
>>
>>> Additionally, the SFs are multifunctional devices supporting RDMA,
>>> network devices, clocks, and more, similar to their peer PCI PFs and
>>> VFs. Therefore, it is desirable to have SFs' IRQ information available
>>> at the bus/device level.
>>
>> But it should be as part of the pci device, as that's where that
>> information lives and is "bound" to, not the aux device on its own.
> 
> 
> Auxiliary bus level SF device is using that IRQ too. So it is
> additionally shown at auxiliary device level too.
> 
> 
>>
>>> To overcome the above limitations, this short series extends the
>>> auxiliary bus to display IRQ information in sysfs, similar to that of
>>> PFs and VFs.
>>
>> Again, examples of what it looks like today, and what it will look like
>> with this patch set is needed in order to justify why this really is
>> needed as it seems that the information should already be there for you.
> 
> 
> full example:
> in a machine with a single child SF of the PF 00:0b.0 we currently have
> the bellow:
> $ ls /sys/bus/pci/devices/0000:00:0b.0/msi_irqs/
> 41  42  43  44  45  46  47  48  49  50  51  52  53  54  55  56  57  58
> 
> with this patch-set we will also have:
> $ ls /sys/bus/pci/devices/0000\:00\:0b.0/mlx5_core.sf.1/irqs/
> 50  51  52  53  54  55  56  57  58
> 
> which means that IRQs 50-58, which are shown on the PF "msi_irqs" are
> used by the child SF.
> 
> 
>>
>>> It adds an 'irqs' directory under the auxiliary device and includes an
>>> <irq_num> sysfs file within it. Sometimes, the PCI SF auxiliary devices
>>> share the IRQ with other SFs, a detail that is also not available to the
>>> users. Consequently, this <irq_num> file indicates whether the IRQ is
>>> 'exclusive' or 'shared'.  This 'irqs' directory extenstion is optional,
>>> i.e. only for PCI SFs the sysfs irq information is optionally exposed.
>>
>> Why does userspace care about "shared" or not?  What can they do with
>> that, and why?
> 
> 
> If IRQ is shared, userspace needs to take it into account when looking
> at IRQ data and counters such as interrupts/sec.
> Also, If IRQ is shared, user better not mess with the irq affinity of
> such irq it as it can affect multiple SFs.
> 
> 
>>
>>> For example:
>>> $ ls /sys/bus/auxiliary/devices/mlx5_core.sf.1/irqs/
>>> 50  51  52  53  54  55  56  57  58
>>> $ cat /sys/bus/auxiliary/devices/mlx5_core.sf.1/irqs/52
>>> exclusive
>>
>> "exclusive" for now, but again, why?  Who cares?  These are msi irqs it
>> shouldn't matter if they are shared or not.
> 
> hope I explained the current limitation and the proposed solution above
> 
>>
>> thanks,
>>
>> greg k-h
> 

Powered by blists - more mailing lists