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

[Gal Pressman <galpress@amazon.com>]

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?