Re: [PATCH for-next] RDMA/nldev: Add parent bdf to device information dump

Gal Pressman
2020-11-09T13:52:48Z

On 09/11/2020 15:02, Leon Romanovsky wrote:
> On Mon, Nov 09, 2020 at 02:47:07PM +0200, Gal Pressman wrote:
>> On 09/11/2020 14:32, Leon Romanovsky wrote:
>>> On Mon, Nov 09, 2020 at 02:27:16PM +0200, Gal Pressman wrote:
>>>> On 09/11/2020 13:55, Leon Romanovsky wrote:
>>>>> On Mon, Nov 09, 2020 at 11:03:25AM +0200, Gal Pressman wrote:
>>>>>>
>>>>>> On 09/11/2020 7:09, Leon Romanovsky wrote:
>>>>>>> On Sun, Nov 08, 2020 at 07:49:35PM -0400, Jason Gunthorpe wrote:
>>>>>>>> On Sun, Nov 08, 2020 at 03:03:45PM +0200, Gal Pressman wrote:
>>>>>>>>> On 05/11/2020 22:00, Jason Gunthorpe wrote:
>>>>>>>>>> On Tue, Nov 03, 2020 at 05:45:26PM +0200, Gal Pressman wrote:
>>>>>>>>>>> On 03/11/2020 16:22, Jason Gunthorpe wrote:
>>>>>>>>>>>> On Tue, Nov 03, 2020 at 04:11:19PM +0200, Gal Pressman wrote:
>>>>>>>>>>>>> On 03/11/2020 15:57, Leon Romanovsky wrote:
>>>>>>>>>>>>>> On Tue, Nov 03, 2020 at 09:45:22AM -0400, Jason Gunthorpe wrote:
>>>>>>>>>>>>>>> On Tue, Nov 03, 2020 at 03:26:27PM +0200, Gal Pressman wrote:
>>>>>>>>>>>>>>>> Add the ability to query the device's bdf through rdma tool netlink
>>>>>>>>>>>>>>>> command (in addition to the sysfs infra).
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> In case of virtual devices (rxe/siw), the netdev bdf will be shown.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Why? What is the use case?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Right, and why isn't netdev (RDMA_NLDEV_ATTR_NDEV_NAME) enough?
>>>>>>>>>>>>>
>>>>>>>>>>>>> When taking system topology into consideration you need some way to pair the
>>>>>>>>>>>>> ibdev and bdf, especially when working with multiple devices.
>>>>>>>>>>>>> The netdev name doesn't exist on devices with no netdevs (IB, EFA).
>>>>>>>>>>>>
>>>>>>>>>>>> You are supposed to use sysfs
>>>>>>>>>>>>
>>>>>>>>>>>> /sys/class/infiniband/ibp0s9/device
>>>>>>>>>>>>
>>>>>>>>>>>> Should always be the physical device
>>>>>>>>>>>>
>>>>>>>>>>>>> Why rdma tool? Because it's more intuitive than sysfs.
>>>>>>>>>>>>
>>>>>>>>>>>> But we generally don't put this information into netlink BDF is just
>>>>>>>>>>>> the start, you need all the other topology information to make sense
>>>>>>>>>>>> of it, and all that is in sysfs only already
>>>>>>>>>>>
>>>>>>>>>>> As the commit message says, it's in addition to the device sysfs.
>>>>>>>>>>>
>>>>>>>>>>> Many (if not most) of the existing rdma netlink commands are duplicates of some
>>>>>>>>>>> sysfs entries, but show it in a more "modern" way.
>>>>>>>>>>> I'm not convinced that bdf should be treated differently.
>>>>>>>>>>
>>>>>>>>>> Why did you call it BDF anyhow? it has nothing to do with PCI BDF
>>>>>>>>>> other than it happens to be the PDF for PCI devices. Netdev called
>>>>>>>>>> this bus_info
>>>>>>>>>
>>>>>>>>> Are there non pci devices in the subsystem?
>>>>>>>>
>>>>>>>> Yes, HNS uses non-pci devices
>>>>>>>>
>>>>>>>>> I can rename to a more fitting name, will change to bus_info unless
>>>>>>>>> someone has a better idea.
>>>>>>>>
>>>>>>>> The thing is, is is still useless. You have to consult sysfs to
>>>>>>>> understand what bus it is scoped on to do anything further with
>>>>>>>> it. Can't just assume it is PCI.
>>>>>>>
>>>>>>> Can anyone please remind me why are we doing it?
>>>>>>> What problem do you solve here by adding new nldev attributes?
>>>>>>
>>>>>> https://lore.kernel.org/linux-rdma/0825e1bf-f913-d2c1-ad3f-35ba3d6b75ef@amazon.com/
>>>>>
>>>>> Thanks, but IMHO it doesn't answer on the question about the problem.
>>>>
>>>> For example, in an instance with multiple NICs and GPUs, it's common to examine
>>>> the devices topology and distances, device bdfs are needed for that.
>>>>
>>>> Also, when analyzing dmesg logs the prints contain the ibdev name, which is not
>>>> always enough when trying to debug the corresponding physical device.
>>>
>>> Gal,
>>>
>>> I'm asking which problem will solve new nldev and not why BDF is important. :)
>>
>> This patch follows the implementation of other fields in fill_dev_info() such as
>> port index, fw version, node guid, sys image guid, node type, dev protocol, etc,
>> which also exist in sysfs.
>>
>> You added most of these new nldevs not long ago, so I find your question a bit
>> confusing.. Can you please explain your concerns and why you think bdf is different?
>
> Almost all fields that you mentioned were needed to implement rdma_rename
> utility that followed systemd internal implementation and/or were used in
> the rdma-core.
>
> The FW version is clearly an exemption to the above.
>
> So I'm trying to understand the rationale behind BDF and how it will
> work with different bus variants that we will have. Like Parav said,
> the IB is connected to auxiliary bus (no BDF) and will have parent
> with BDF too at the same time.

We can review the different cases and make sure it works as expected (and what's
expected), but let's first reach an agreement if I should continue with this
work or not.