On Mon, Nov 09, 2020 at 03:52:29PM +0200, Gal Pressman wrote: > 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@xxxxxxxxxx/ > >>>>> > >>>>> 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. I don't know. Thanks
