| Message ID | 20190826204119.54386-4-parav@mellanox.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce variable length mdev alias | expand |
On Mon, 26 Aug 2019 15:41:18 -0500 Parav Pandit <parav@mellanox.com> wrote: > Expose mdev alias as string in a sysfs tree so that such attribute can > be used to generate netdevice name by systemd/udev or can be used to > match other kernel objects based on the alias of the mdev. > > Signed-off-by: Parav Pandit <parav@mellanox.com> > --- > drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index 43afe0e80b76..59f4e3cc5233 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr, > > static DEVICE_ATTR_WO(remove); > > +static ssize_t alias_show(struct device *device, > + struct device_attribute *attr, char *buf) > +{ > + struct mdev_device *dev = mdev_from_dev(device); > + > + if (!dev->alias) > + return -EOPNOTSUPP; Wouldn't it be better to not create the alias at all? Thanks, Alex > + > + return sprintf(buf, "%s\n", dev->alias); > +} > +static DEVICE_ATTR_RO(alias); > + > static const struct attribute *mdev_device_attrs[] = { > + &dev_attr_alias.attr, > &dev_attr_remove.attr, > NULL, > };
> -----Original Message----- > From: Alex Williamson <alex.williamson@redhat.com> > Sent: Tuesday, August 27, 2019 7:24 AM > To: Parav Pandit <parav@mellanox.com> > Cc: Jiri Pirko <jiri@mellanox.com>; kwankhede@nvidia.com; > cohuck@redhat.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > On Mon, 26 Aug 2019 15:41:18 -0500 > Parav Pandit <parav@mellanox.com> wrote: > > > Expose mdev alias as string in a sysfs tree so that such attribute can > > be used to generate netdevice name by systemd/udev or can be used to > > match other kernel objects based on the alias of the mdev. > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > > drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233 > > 100644 > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, > > struct device_attribute *attr, > > > > static DEVICE_ATTR_WO(remove); > > > > +static ssize_t alias_show(struct device *device, > > + struct device_attribute *attr, char *buf) { > > + struct mdev_device *dev = mdev_from_dev(device); > > + > > + if (!dev->alias) > > + return -EOPNOTSUPP; > > Wouldn't it be better to not create the alias at all? Thanks, > In other subsystem such as netdev sysfs files are always created that returns either returns EOPNOTSUPP or attribute value. I guess overhead of create multiple groups or creating individual sysfs files outweigh the simplify of single group. I think its ok to keep it simple this way. > Alex > > > + > > + return sprintf(buf, "%s\n", dev->alias); } static > > +DEVICE_ATTR_RO(alias); > > + > > static const struct attribute *mdev_device_attrs[] = { > > + &dev_attr_alias.attr, > > &dev_attr_remove.attr, > > NULL, > > };
On Mon, 26 Aug 2019 15:41:18 -0500 Parav Pandit <parav@mellanox.com> wrote: > Expose mdev alias as string in a sysfs tree so that such attribute can > be used to generate netdevice name by systemd/udev or can be used to > match other kernel objects based on the alias of the mdev. What about "Expose the optional alias for an mdev device as a sysfs attribute. This way, userspace tools such as udev may make use of the alias, for example to create a netdevice name for the mdev." > > Signed-off-by: Parav Pandit <parav@mellanox.com> > --- > drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++ I think the documentation should be updated as well. > 1 file changed, 13 insertions(+) > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c > index 43afe0e80b76..59f4e3cc5233 100644 > --- a/drivers/vfio/mdev/mdev_sysfs.c > +++ b/drivers/vfio/mdev/mdev_sysfs.c > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr, > > static DEVICE_ATTR_WO(remove); > > +static ssize_t alias_show(struct device *device, > + struct device_attribute *attr, char *buf) > +{ > + struct mdev_device *dev = mdev_from_dev(device); > + > + if (!dev->alias) > + return -EOPNOTSUPP; I'm wondering how to make this consumable by userspace in the easiest way. - As you do now (userspace gets an error when trying to read)? - Returning an empty value (nothing to see here, move along)? - Or not creating the attribute at all? That would match what userspace sees on older kernels, so it needs to be able to deal with that anyway. > + > + return sprintf(buf, "%s\n", dev->alias); > +} > +static DEVICE_ATTR_RO(alias); > + > static const struct attribute *mdev_device_attrs[] = { > + &dev_attr_alias.attr, > &dev_attr_remove.attr, > NULL, > };
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, August 27, 2019 4:17 PM > To: Parav Pandit <parav@mellanox.com> > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > On Mon, 26 Aug 2019 15:41:18 -0500 > Parav Pandit <parav@mellanox.com> wrote: > > > Expose mdev alias as string in a sysfs tree so that such attribute can > > be used to generate netdevice name by systemd/udev or can be used to > > match other kernel objects based on the alias of the mdev. > > What about > > "Expose the optional alias for an mdev device as a sysfs attribute. > This way, userspace tools such as udev may make use of the alias, for example > to create a netdevice name for the mdev." > Ok. I will change it. > > > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > --- > > drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++ > > I think the documentation should be updated as well. > > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/vfio/mdev/mdev_sysfs.c > > b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233 > > 100644 > > --- a/drivers/vfio/mdev/mdev_sysfs.c > > +++ b/drivers/vfio/mdev/mdev_sysfs.c > > @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, > > struct device_attribute *attr, > > > > static DEVICE_ATTR_WO(remove); > > > > +static ssize_t alias_show(struct device *device, > > + struct device_attribute *attr, char *buf) { > > + struct mdev_device *dev = mdev_from_dev(device); > > + > > + if (!dev->alias) > > + return -EOPNOTSUPP; > > I'm wondering how to make this consumable by userspace in the easiest way. > - As you do now (userspace gets an error when trying to read)? > - Returning an empty value (nothing to see here, move along)? No. This is confusing, to return empty value, because it says that there is an alias but it is some weird empty string. If there is alias, it shows exactly what it is. If no alias it returns an error code = unsupported -> inline with other widely used subsystem. > - Or not creating the attribute at all? That would match what userspace > sees on older kernels, so it needs to be able to deal with that New sysfs files can appear. Tool cannot say that I was not expecting this file here. User space is supposed to work with the file they are off interest. Mdev interface has option to specify vendor specific files, though in usual manner it's not recommended. So there is no old user space, new kernel issue here. > anyway. > > > + > > + return sprintf(buf, "%s\n", dev->alias); } static > > +DEVICE_ATTR_RO(alias); > > + > > static const struct attribute *mdev_device_attrs[] = { > > + &dev_attr_alias.attr, > > &dev_attr_remove.attr, > > NULL, > > };
On Tue, 27 Aug 2019 11:07:37 +0000 Parav Pandit <parav@mellanox.com> wrote: > > -----Original Message----- > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Tuesday, August 27, 2019 4:17 PM > > To: Parav Pandit <parav@mellanox.com> > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > > kernel@vger.kernel.org; netdev@vger.kernel.org > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > On Mon, 26 Aug 2019 15:41:18 -0500 > > Parav Pandit <parav@mellanox.com> wrote: > > > +static ssize_t alias_show(struct device *device, > > > + struct device_attribute *attr, char *buf) { > > > + struct mdev_device *dev = mdev_from_dev(device); > > > + > > > + if (!dev->alias) > > > + return -EOPNOTSUPP; > > > > I'm wondering how to make this consumable by userspace in the easiest way. > > - As you do now (userspace gets an error when trying to read)? > > - Returning an empty value (nothing to see here, move along)? > No. This is confusing, to return empty value, because it says that there is an alias but it is some weird empty string. > If there is alias, it shows exactly what it is. > If no alias it returns an error code = unsupported -> inline with other widely used subsystem. > > > - Or not creating the attribute at all? That would match what userspace > > sees on older kernels, so it needs to be able to deal with that > New sysfs files can appear. Tool cannot say that I was not expecting this file here. > User space is supposed to work with the file they are off interest. > Mdev interface has option to specify vendor specific files, though in usual manner it's not recommended. > So there is no old user space, new kernel issue here. I'm not talking about old userspace/new kernel, but new userspace/old kernel. Code that wants to consume this attribute needs to be able to cope with its absence anyway. > > > anyway. > > > > > + > > > + return sprintf(buf, "%s\n", dev->alias); } static > > > +DEVICE_ATTR_RO(alias); > > > + > > > static const struct attribute *mdev_device_attrs[] = { > > > + &dev_attr_alias.attr, > > > &dev_attr_remove.attr, > > > NULL, > > > }; >
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, August 27, 2019 5:05 PM > To: Parav Pandit <parav@mellanox.com> > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > On Tue, 27 Aug 2019 11:07:37 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Cornelia Huck <cohuck@redhat.com> > > > Sent: Tuesday, August 27, 2019 4:17 PM > > > To: Parav Pandit <parav@mellanox.com> > > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; > > > linux- kernel@vger.kernel.org; netdev@vger.kernel.org > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > > > On Mon, 26 Aug 2019 15:41:18 -0500 > > > Parav Pandit <parav@mellanox.com> wrote: > > > > > +static ssize_t alias_show(struct device *device, > > > > + struct device_attribute *attr, char *buf) { > > > > + struct mdev_device *dev = mdev_from_dev(device); > > > > + > > > > + if (!dev->alias) > > > > + return -EOPNOTSUPP; > > > > > > I'm wondering how to make this consumable by userspace in the easiest > way. > > > - As you do now (userspace gets an error when trying to read)? > > > - Returning an empty value (nothing to see here, move along)? > > No. This is confusing, to return empty value, because it says that there is an > alias but it is some weird empty string. > > If there is alias, it shows exactly what it is. > > If no alias it returns an error code = unsupported -> inline with other widely > used subsystem. > > > > > - Or not creating the attribute at all? That would match what userspace > > > sees on older kernels, so it needs to be able to deal with that > > New sysfs files can appear. Tool cannot say that I was not expecting this file > here. > > User space is supposed to work with the file they are off interest. > > Mdev interface has option to specify vendor specific files, though in usual > manner it's not recommended. > > So there is no old user space, new kernel issue here. > > I'm not talking about old userspace/new kernel, but new userspace/old kernel. > Code that wants to consume this attribute needs to be able to cope with its > absence anyway. > Old kernel doesn't have alias file. If some tool tries to read this file it will fail to open non existing file; open() system call is already taking care of it.
On Tue, 27 Aug 2019 11:52:21 +0000 Parav Pandit <parav@mellanox.com> wrote: > > -----Original Message----- > > From: Cornelia Huck <cohuck@redhat.com> > > Sent: Tuesday, August 27, 2019 5:05 PM > > To: Parav Pandit <parav@mellanox.com> > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > > kernel@vger.kernel.org; netdev@vger.kernel.org > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > On Tue, 27 Aug 2019 11:07:37 +0000 > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > -----Original Message----- > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > Sent: Tuesday, August 27, 2019 4:17 PM > > > > To: Parav Pandit <parav@mellanox.com> > > > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; > > > > linux- kernel@vger.kernel.org; netdev@vger.kernel.org > > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > > > > > On Mon, 26 Aug 2019 15:41:18 -0500 > > > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > > +static ssize_t alias_show(struct device *device, > > > > > + struct device_attribute *attr, char *buf) { > > > > > + struct mdev_device *dev = mdev_from_dev(device); > > > > > + > > > > > + if (!dev->alias) > > > > > + return -EOPNOTSUPP; > > > > > > > > I'm wondering how to make this consumable by userspace in the easiest > > way. > > > > - As you do now (userspace gets an error when trying to read)? > > > > - Returning an empty value (nothing to see here, move along)? > > > No. This is confusing, to return empty value, because it says that there is an > > alias but it is some weird empty string. > > > If there is alias, it shows exactly what it is. > > > If no alias it returns an error code = unsupported -> inline with other widely > > used subsystem. > > > > > > > - Or not creating the attribute at all? That would match what userspace > > > > sees on older kernels, so it needs to be able to deal with that > > > New sysfs files can appear. Tool cannot say that I was not expecting this file > > here. > > > User space is supposed to work with the file they are off interest. > > > Mdev interface has option to specify vendor specific files, though in usual > > manner it's not recommended. > > > So there is no old user space, new kernel issue here. > > > > I'm not talking about old userspace/new kernel, but new userspace/old kernel. > > Code that wants to consume this attribute needs to be able to cope with its > > absence anyway. > > > Old kernel doesn't have alias file. > If some tool tries to read this file it will fail to open non existing file; open() system call is already taking care of it. Yes, that was exactly my argument?
> -----Original Message----- > From: Cornelia Huck <cohuck@redhat.com> > Sent: Tuesday, August 27, 2019 5:25 PM > To: Parav Pandit <parav@mellanox.com> > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; linux- > kernel@vger.kernel.org; netdev@vger.kernel.org > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > On Tue, 27 Aug 2019 11:52:21 +0000 > Parav Pandit <parav@mellanox.com> wrote: > > > > -----Original Message----- > > > From: Cornelia Huck <cohuck@redhat.com> > > > Sent: Tuesday, August 27, 2019 5:05 PM > > > To: Parav Pandit <parav@mellanox.com> > > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; > > > linux- kernel@vger.kernel.org; netdev@vger.kernel.org > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > > > On Tue, 27 Aug 2019 11:07:37 +0000 > > > Parav Pandit <parav@mellanox.com> wrote: > > > > > > > > -----Original Message----- > > > > > From: Cornelia Huck <cohuck@redhat.com> > > > > > Sent: Tuesday, August 27, 2019 4:17 PM > > > > > To: Parav Pandit <parav@mellanox.com> > > > > > Cc: alex.williamson@redhat.com; Jiri Pirko <jiri@mellanox.com>; > > > > > kwankhede@nvidia.com; davem@davemloft.net; kvm@vger.kernel.org; > > > > > linux- kernel@vger.kernel.org; netdev@vger.kernel.org > > > > > Subject: Re: [PATCH 3/4] mdev: Expose mdev alias in sysfs tree > > > > > > > > > > On Mon, 26 Aug 2019 15:41:18 -0500 Parav Pandit > > > > > <parav@mellanox.com> wrote: > > > > > > > > > +static ssize_t alias_show(struct device *device, > > > > > > + struct device_attribute *attr, char *buf) { > > > > > > + struct mdev_device *dev = mdev_from_dev(device); > > > > > > + > > > > > > + if (!dev->alias) > > > > > > + return -EOPNOTSUPP; > > > > > > > > > > I'm wondering how to make this consumable by userspace in the > > > > > easiest > > > way. > > > > > - As you do now (userspace gets an error when trying to read)? > > > > > - Returning an empty value (nothing to see here, move along)? > > > > No. This is confusing, to return empty value, because it says that > > > > there is an > > > alias but it is some weird empty string. > > > > If there is alias, it shows exactly what it is. > > > > If no alias it returns an error code = unsupported -> inline with > > > > other widely > > > used subsystem. > > > > > > > > > - Or not creating the attribute at all? That would match what userspace > > > > > sees on older kernels, so it needs to be able to deal with > > > > > that > > > > New sysfs files can appear. Tool cannot say that I was not > > > > expecting this file > > > here. > > > > User space is supposed to work with the file they are off interest. > > > > Mdev interface has option to specify vendor specific files, though > > > > in usual > > > manner it's not recommended. > > > > So there is no old user space, new kernel issue here. > > > > > > I'm not talking about old userspace/new kernel, but new userspace/old > kernel. > > > Code that wants to consume this attribute needs to be able to cope > > > with its absence anyway. > > > > > Old kernel doesn't have alias file. > > If some tool tries to read this file it will fail to open non existing file; open() > system call is already taking care of it. > > Yes, that was exactly my argument? I misunderstood probably. I re-read all 3 options you posted. I do not see any issue in reporting error code similar to other widely used netdev subsystem, hence propose what is posted in the patch.
diff --git a/drivers/vfio/mdev/mdev_sysfs.c b/drivers/vfio/mdev/mdev_sysfs.c index 43afe0e80b76..59f4e3cc5233 100644 --- a/drivers/vfio/mdev/mdev_sysfs.c +++ b/drivers/vfio/mdev/mdev_sysfs.c @@ -246,7 +246,20 @@ static ssize_t remove_store(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR_WO(remove); +static ssize_t alias_show(struct device *device, + struct device_attribute *attr, char *buf) +{ + struct mdev_device *dev = mdev_from_dev(device); + + if (!dev->alias) + return -EOPNOTSUPP; + + return sprintf(buf, "%s\n", dev->alias); +} +static DEVICE_ATTR_RO(alias); + static const struct attribute *mdev_device_attrs[] = { + &dev_attr_alias.attr, &dev_attr_remove.attr, NULL, };
Expose mdev alias as string in a sysfs tree so that such attribute can be used to generate netdevice name by systemd/udev or can be used to match other kernel objects based on the alias of the mdev. Signed-off-by: Parav Pandit <parav@mellanox.com> --- drivers/vfio/mdev/mdev_sysfs.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)