| Message ID | 20180228234006.21093-3-logang@deltatee.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Copy Offload in NVMe Fabrics with P2P PCI Memory | expand |
On Wed, Feb 28, 2018 at 04:39:58PM -0700, Logan Gunthorpe wrote: > Attributes display the total amount of P2P memory, the amount available > and whether it is published or not. Can you add enough text here to make the body of the changelog complete in itself? That might mean just repeating the subject, which is fine. > Signed-off-by: Logan Gunthorpe <logang@deltatee.com> > --- > Documentation/ABI/testing/sysfs-bus-pci | 25 +++++++++++++++++ > drivers/pci/p2pdma.c | 50 +++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+) > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > index 44d4b2be92fd..f5656dae21be 100644 > --- a/Documentation/ABI/testing/sysfs-bus-pci > +++ b/Documentation/ABI/testing/sysfs-bus-pci > @@ -323,3 +323,28 @@ Description: > > This is similar to /sys/bus/pci/drivers_autoprobe, but > affects only the VFs associated with a specific PF. > + > +What: /sys/bus/pci/devices/.../p2pmem/available > +Date: November 2017 > +Contact: Logan Gunthorpe <logang@deltatee.com> > +Description: > + If the device has any Peer-to-Peer memory registered, this > + file contains the amount of memory that has not been > + allocated (in decimal). > + > +What: /sys/bus/pci/devices/.../p2pmem/size > +Date: November 2017 > +Contact: Logan Gunthorpe <logang@deltatee.com> > +Description: > + If the device has any Peer-to-Peer memory registered, this > + file contains the total ammount of memory that the device s/ammount/amount/ > + provides (in decimal). > + > +What: /sys/bus/pci/devices/.../p2pmem/published > +Date: November 2017 > +Contact: Logan Gunthorpe <logang@deltatee.com> > +Description: > + If the device has any Peer-to-Peer memory registered, this > + file contains a '1' if the memory has been published for > + use inside the kernel or a '0' if it is only intended > + for use within the driver that published it. > diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c > index ec0a6cb9e500..a57df78f6a32 100644 > --- a/drivers/pci/p2pdma.c > +++ b/drivers/pci/p2pdma.c > @@ -29,6 +29,53 @@ struct pci_p2pdma { > bool published; > }; > > +static ssize_t size_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct pci_dev *pdev = to_pci_dev(dev); > + size_t size = 0; > + > + if (pdev->p2pdma->pool) > + size = gen_pool_size(pdev->p2pdma->pool); > + > + return snprintf(buf, PAGE_SIZE, "%zd\n", size); > +} > +static DEVICE_ATTR_RO(size); > + > +static ssize_t available_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct pci_dev *pdev = to_pci_dev(dev); > + size_t avail = 0; > + > + if (pdev->p2pdma->pool) > + avail = gen_pool_avail(pdev->p2pdma->pool); > + > + return snprintf(buf, PAGE_SIZE, "%zd\n", avail); > +} > +static DEVICE_ATTR_RO(available); > + > +static ssize_t published_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct pci_dev *pdev = to_pci_dev(dev); > + > + return snprintf(buf, PAGE_SIZE, "%d\n", pdev->p2pdma->published); > +} > +static DEVICE_ATTR_RO(published); > + > +static struct attribute *p2pmem_attrs[] = { > + &dev_attr_size.attr, > + &dev_attr_available.attr, > + &dev_attr_published.attr, > + NULL, > +}; > + > +static const struct attribute_group p2pmem_group = { > + .attrs = p2pmem_attrs, > + .name = "p2pmem", > +}; > + > static void pci_p2pdma_percpu_release(struct percpu_ref *ref) > { > struct pci_p2pdma *p2p = > @@ -55,6 +102,7 @@ static void pci_p2pdma_release(void *data) > percpu_ref_exit(&pdev->p2pdma->devmap_ref); > > gen_pool_destroy(pdev->p2pdma->pool); > + sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group); > pdev->p2pdma = NULL; > } > > @@ -83,6 +131,8 @@ static int pci_p2pdma_setup(struct pci_dev *pdev) > if (error) > goto out_pool_destroy; > > + error = sysfs_create_group(&pdev->dev.kobj, &p2pmem_group); > + > pdev->p2pdma = p2p; I think these two statements are out of order, since the attributes dereference pdev->p2pdma. And it looks like you set "error" unnecessarily, since you return immediately looking at it. > return 0; > -- > 2.11.0 >
On 01/03/18 10:44 AM, Bjorn Helgaas wrote: > I think these two statements are out of order, since the attributes > dereference pdev->p2pdma. And it looks like you set "error" > unnecessarily, since you return immediately looking at it. Per the previous series, sysfs_create_group is must_check for some reason. I had a printk there but you didn't think it was necessary. So assigning it to error is the only way to squash the warning. Logan
On Thu, Mar 1, 2018 at 4:15 PM, Logan Gunthorpe <logang@deltatee.com> wrote: > > > On 01/03/18 10:44 AM, Bjorn Helgaas wrote: >> >> I think these two statements are out of order, since the attributes >> dereference pdev->p2pdma. And it looks like you set "error" >> unnecessarily, since you return immediately looking at it. > > > Per the previous series, sysfs_create_group is must_check for some reason. I > had a printk there but you didn't think it was necessary. So assigning it to > error is the only way to squash the warning. Why not fail the setup if the sysfs_create_group() fails? Sure, it may not be strictly required for userspace to have access to these attributes, but it seems hostile that userspace can't make assumptions about the presence of the "p2pmem" directory relative to the capability being setup.
On 01/03/18 05:36 PM, Dan Williams wrote: > On Thu, Mar 1, 2018 at 4:15 PM, Logan Gunthorpe <logang@deltatee.com> wrote: >> >> >> On 01/03/18 10:44 AM, Bjorn Helgaas wrote: >>> >>> I think these two statements are out of order, since the attributes >>> dereference pdev->p2pdma. And it looks like you set "error" >>> unnecessarily, since you return immediately looking at it. >> >> >> Per the previous series, sysfs_create_group is must_check for some reason. I >> had a printk there but you didn't think it was necessary. So assigning it to >> error is the only way to squash the warning. > > Why not fail the setup if the sysfs_create_group() fails? Sure, it may > not be strictly required for userspace to have access to these > attributes, but it seems hostile that userspace can't make assumptions > about the presence of the "p2pmem" directory relative to the > capability being setup. Works for me. I'll do that. Thanks, Logan
diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci index 44d4b2be92fd..f5656dae21be 100644 --- a/Documentation/ABI/testing/sysfs-bus-pci +++ b/Documentation/ABI/testing/sysfs-bus-pci @@ -323,3 +323,28 @@ Description: This is similar to /sys/bus/pci/drivers_autoprobe, but affects only the VFs associated with a specific PF. + +What: /sys/bus/pci/devices/.../p2pmem/available +Date: November 2017 +Contact: Logan Gunthorpe <logang@deltatee.com> +Description: + If the device has any Peer-to-Peer memory registered, this + file contains the amount of memory that has not been + allocated (in decimal). + +What: /sys/bus/pci/devices/.../p2pmem/size +Date: November 2017 +Contact: Logan Gunthorpe <logang@deltatee.com> +Description: + If the device has any Peer-to-Peer memory registered, this + file contains the total ammount of memory that the device + provides (in decimal). + +What: /sys/bus/pci/devices/.../p2pmem/published +Date: November 2017 +Contact: Logan Gunthorpe <logang@deltatee.com> +Description: + If the device has any Peer-to-Peer memory registered, this + file contains a '1' if the memory has been published for + use inside the kernel or a '0' if it is only intended + for use within the driver that published it. diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c index ec0a6cb9e500..a57df78f6a32 100644 --- a/drivers/pci/p2pdma.c +++ b/drivers/pci/p2pdma.c @@ -29,6 +29,53 @@ struct pci_p2pdma { bool published; }; +static ssize_t size_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct pci_dev *pdev = to_pci_dev(dev); + size_t size = 0; + + if (pdev->p2pdma->pool) + size = gen_pool_size(pdev->p2pdma->pool); + + return snprintf(buf, PAGE_SIZE, "%zd\n", size); +} +static DEVICE_ATTR_RO(size); + +static ssize_t available_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct pci_dev *pdev = to_pci_dev(dev); + size_t avail = 0; + + if (pdev->p2pdma->pool) + avail = gen_pool_avail(pdev->p2pdma->pool); + + return snprintf(buf, PAGE_SIZE, "%zd\n", avail); +} +static DEVICE_ATTR_RO(available); + +static ssize_t published_show(struct device *dev, struct device_attribute *attr, + char *buf) +{ + struct pci_dev *pdev = to_pci_dev(dev); + + return snprintf(buf, PAGE_SIZE, "%d\n", pdev->p2pdma->published); +} +static DEVICE_ATTR_RO(published); + +static struct attribute *p2pmem_attrs[] = { + &dev_attr_size.attr, + &dev_attr_available.attr, + &dev_attr_published.attr, + NULL, +}; + +static const struct attribute_group p2pmem_group = { + .attrs = p2pmem_attrs, + .name = "p2pmem", +}; + static void pci_p2pdma_percpu_release(struct percpu_ref *ref) { struct pci_p2pdma *p2p = @@ -55,6 +102,7 @@ static void pci_p2pdma_release(void *data) percpu_ref_exit(&pdev->p2pdma->devmap_ref); gen_pool_destroy(pdev->p2pdma->pool); + sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group); pdev->p2pdma = NULL; } @@ -83,6 +131,8 @@ static int pci_p2pdma_setup(struct pci_dev *pdev) if (error) goto out_pool_destroy; + error = sysfs_create_group(&pdev->dev.kobj, &p2pmem_group); + pdev->p2pdma = p2p; return 0;
Attributes display the total amount of P2P memory, the amount available and whether it is published or not. Signed-off-by: Logan Gunthorpe <logang@deltatee.com> --- Documentation/ABI/testing/sysfs-bus-pci | 25 +++++++++++++++++ drivers/pci/p2pdma.c | 50 +++++++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+)