[02/12] pci-p2p: Add sysfs group to display p2pmem stats
diff mbox

Message ID 20180104190137.7654-3-logang@deltatee.com
State New, archived
Headers show

Commit Message

Logan Gunthorpe Jan. 4, 2018, 7:01 p.m. UTC
Attributes display the total amount of P2P memory, the ammount 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/p2p.c                       | 51 +++++++++++++++++++++++++++++++++
 2 files changed, 76 insertions(+)

Comments

Bjorn Helgaas Jan. 4, 2018, 9:50 p.m. UTC | #1
On Thu, Jan 04, 2018 at 12:01:27PM -0700, Logan Gunthorpe wrote:
> Attributes display the total amount of P2P memory, the ammount available
> and whether it is published or not.

s/ammount/amount/ (also below)

> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 25 ++++++++++++++++
>  drivers/pci/p2p.c                       | 51 +++++++++++++++++++++++++++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index 44d4b2be92fd..7b80ea77faca 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

I wonder if "p2pdma" would be a more suggestive term?  It's not really
the *memory* that is peer-to-peer; the peer-to-peer part is referring
to *access* to the memory.

> @@ -82,6 +130,9 @@ static int pci_p2pmem_setup(struct pci_dev *pdev)
>  	if (error)
>  		goto out_pool_destroy;
>  
> +	if (sysfs_create_group(&pdev->dev.kobj, &p2pmem_group))
> +		dev_warn(&pdev->dev, "failed to create p2p sysfs group\n");

Not sure the warning (by itself) is worthwhile.  If we were going to
disable the feature if sysfs_create_group() failed, that's one thing,
but we aren't doing anything except generating a warning, which the
user can't really do anything with.  If the user is looking for the
sysfs file, its absence will be obvious even without the message.

>  	pdev->p2p = p2p;
>  
>  	return 0;
> -- 
> 2.11.0
>
Jason Gunthorpe Jan. 4, 2018, 10:25 p.m. UTC | #2
On Thu, Jan 04, 2018 at 03:50:40PM -0600, Bjorn Helgaas wrote:
> >  		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
> 
> I wonder if "p2pdma" would be a more suggestive term?  It's not really
> the *memory* that is peer-to-peer; the peer-to-peer part is referring
> to *access* to the memory.

There have been out of tree patches using P2P DMA for a long time, and
some of the use cases have nothing to do with 'memory' - eg DMA to
'registers'

I notice that this series particularly focus on treating the target
BAR as 'memory' - ie it puts genalloc on top of the BAR, and I guess
treat all addresses as equal and interchangable.

If this series gets accepted I would expect proposals to extend this
infrastructure to allow for P2P for registers in some way as well.

So I think the 'p2pmem' name is a good choice only when it is in
places that talk about the genalloc part of this design.

We should reserve p2pdma to talk about the generic infrastructure
unrelated to the genalloc pool.

Since these sysfs's seem to report the genalloc pool status, p2pmem
seems like a good choice to me.

> > @@ -82,6 +130,9 @@ static int pci_p2pmem_setup(struct pci_dev *pdev)
> >  	if (error)
> >  		goto out_pool_destroy;
> >  
> > +	if (sysfs_create_group(&pdev->dev.kobj, &p2pmem_group))
> > +		dev_warn(&pdev->dev, "failed to create p2p sysfs group\n");
> 
> Not sure the warning (by itself) is worthwhile.  If we were going to
> disable the feature if sysfs_create_group() failed, that's one thing,
> but we aren't doing anything except generating a warning, which the
> user can't really do anything with.  If the user is looking for the
> sysfs file, its absence will be obvious even without the message.

Don't most of the failure paths inside sysfs_create_group cause prints
anyhow?

Jason
Logan Gunthorpe Jan. 4, 2018, 11:13 p.m. UTC | #3
On 04/01/18 02:50 PM, Bjorn Helgaas wrote:
> On Thu, Jan 04, 2018 at 12:01:27PM -0700, Logan Gunthorpe wrote:
>> Attributes display the total amount of P2P memory, the ammount available
>> and whether it is published or not.
> 
> s/ammount/amount/ (also below)

Will fix.

> I wonder if "p2pdma" would be a more suggestive term?  It's not really
> the *memory* that is peer-to-peer; the peer-to-peer part is referring
> to *access* to the memory.

I agree with Jason on this point. For now, it only describes the 
peer-to-peer memory attributes. If we change the directory to p2pdma 
then we'd have to add a mem prefix or something to each attribute.

>> @@ -82,6 +130,9 @@ static int pci_p2pmem_setup(struct pci_dev *pdev)
>>   	if (error)
>>   		goto out_pool_destroy;
>>   
>> +	if (sysfs_create_group(&pdev->dev.kobj, &p2pmem_group))
>> +		dev_warn(&pdev->dev, "failed to create p2p sysfs group\n");
> 
> Not sure the warning (by itself) is worthwhile.  If we were going to
> disable the feature if sysfs_create_group() failed, that's one thing,
> but we aren't doing anything except generating a warning, which the
> user can't really do anything with.  If the user is looking for the
> sysfs file, its absence will be obvious even without the message.

Unfortunately, sysfs_create_group() has the warn_unused_result flag set. 
So I need to do something with the result to squash the warning. I could 
just assign it to an unused variable but something seemed like it needed 
to be done and printing a warning was the easiest thing...

Logan

Patch
diff mbox

diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
index 44d4b2be92fd..7b80ea77faca 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 ammount 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/p2p.c b/drivers/pci/p2p.c
index aa465ac9273d..03330c7b5e6d 100644
--- a/drivers/pci/p2p.c
+++ b/drivers/pci/p2p.c
@@ -28,6 +28,53 @@  struct pci_p2p {
 	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->p2p->pool)
+		size = gen_pool_size(pdev->p2p->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->p2p->pool)
+		avail = gen_pool_avail(pdev->p2p->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->p2p->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_p2pmem_percpu_release(struct percpu_ref *ref)
 {
 	struct pci_p2p *p2p =
@@ -54,6 +101,7 @@  static void pci_p2pmem_release(void *data)
 	percpu_ref_exit(&pdev->p2p->devmap_ref);
 
 	gen_pool_destroy(pdev->p2p->pool);
+	sysfs_remove_group(&pdev->dev.kobj, &p2pmem_group);
 	pdev->p2p = NULL;
 }
 
@@ -82,6 +130,9 @@  static int pci_p2pmem_setup(struct pci_dev *pdev)
 	if (error)
 		goto out_pool_destroy;
 
+	if (sysfs_create_group(&pdev->dev.kobj, &p2pmem_group))
+		dev_warn(&pdev->dev, "failed to create p2p sysfs group\n");
+
 	pdev->p2p = p2p;
 
 	return 0;