On Tue, Dec 18, 2018 at 05:40:37PM +0000, Parav Pandit wrote: > > > > -----Original Message----- > > From: Jason Gunthorpe > > Sent: Tuesday, December 18, 2018 10:56 AM > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; RDMA > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman > > <gregkh@xxxxxxx> > > Subject: Re: [PATCH rdma-next] IB/cm: Properly create sysfs layout of RDMA > > devices with InfiniBand link layer > > > > On Tue, Dec 18, 2018 at 09:50:44AM -0700, Parav Pandit wrote: > > > > > > > > > > From: Jason Gunthorpe > > > > Sent: Tuesday, December 18, 2018 10:35 AM > > > > To: Parav Pandit <parav@xxxxxxxxxxxx> > > > > Cc: Leon Romanovsky <leon@xxxxxxxxxx>; Doug Ledford > > > > <dledford@xxxxxxxxxx>; Leon Romanovsky <leonro@xxxxxxxxxxxx>; > > RDMA > > > > mailing list <linux-rdma@xxxxxxxxxxxxxxx>; Greg Kroah-Hartman > > > > <gregkh@xxxxxxx> > > > > Subject: Re: [PATCH rdma-next] IB/cm: Properly create sysfs layout > > > > of RDMA devices with InfiniBand link layer > > > > > > > > On Tue, Dec 18, 2018 at 09:32:16AM -0700, Parav Pandit wrote: > > > > > > > > > > > @@ -4368,13 +4371,12 @@ static void cm_add_one(struct > > > > > > > ib_device > > > > > > *ib_device) > > > > > > > cm_dev->ib_device = ib_device; > > > > > > > cm_dev->ack_delay = ib_device->attrs.local_ca_ack_delay; > > > > > > > cm_dev->going_down = 0; > > > > > > > - cm_dev->device = device_create(&cm_class, &ib_device- > > >dev, > > > > > > > + cm_dev->devnum = ida_alloc(&cm_ida, GFP_KERNEL); > > > > > > > + cm_dev->device = device_create(&cm_class, > > > > > > > +ib_device->dev.parent, > > > > > > > MKDEV(0, 0), NULL, > > > > > > > > > > > > Yikes, this is horrifying, why is it creating a struct device at all like > > this?? > > > > > > > > > > > > Probably the CM counter group debug sysfs should just be added > > > > > > directly to the actual ib_dev's sysfs?? > > > > > > > > > > Yes. anchoring to ibdev's sysfs looks right way but that breaks > > > > > the ABI/scripts who are accustomed to use it from infiniband_cm. > > > > > > > > But doesn't this patch rename those paths already? > > > > > > Yes, it breaks. I did raise this to Leon yesterday. But this patch > > > solves some other problem... > > > > Well, if we decide to break it then lets break it and end up in a sane spot.. > > > Yes, I also think so. If we anchor at ibdev's sysfs, it handles rename also cleanly. > > > > > What paths do you think people are using? > > > > > > > cat /sys/class/infiniband_cm/mlx5_0/1/cm_tx_retries/req > > > > Could we put a symlink here perhaps - one that tracks rename maybe? > If we put under, > cat /sys/class/infiniband/mlx5_0/1/cm_tx_retries/req > compare to previously, > cat /sys/class/infiniband_cm/mlx5_0/1/cm_tx_retries/req > > It is not that bad. > In current patch as end user, I need to figure out, which pci dev cm0 maps to. > Then, I need to go through all ibdevices and find out matching pci dev, and map cm0 to that device. > Of course, a script can be written... > But /sys/class/infiniband/mlx5_0/1/cm_tx_retries/req seems a cleaner approach that doesn't require special scripts, and works with renaming nicely. It is pretty easy to see to which cm your ibdev is connected: [leonro@server rdma-core]$ ls -al /sys/class/infiniband_cm/cm0/device/infiniband total 0 drwxr-xr-x 3 root root 0 Dec 16 12:38 . drwxr-xr-x 9 root root 0 Dec 16 17:17 .. drwxr-xr-x 4 root root 0 Dec 16 13:01 hfi1 Thanks
Attachment:
signature.asc
Description: PGP signature
