diff mbox

[rdma-next,1/6] IB/core: Save the device attributes on the device structure

Message ID 1450358340-19361-2-git-send-email-ogerlitz@mellanox.com (mailing list archive)
State Superseded
Headers show

Commit Message

Or Gerlitz Dec. 17, 2015, 1:18 p.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

This way both the IB core and upper level drivers can access these cached
device attributes rather than querying or caching them on their own.

Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
 drivers/infiniband/core/device.c | 6 ++++++
 include/rdma/ib_verbs.h          | 1 +
 2 files changed, 7 insertions(+)

Comments

Sagi Grimberg Dec. 17, 2015, 1:44 p.m. UTC | #1 
> +	ret = ib_query_device(device, &device->attrs);
> +	if (ret) {
> +		printk(KERN_WARNING "Couldn't query the device attributes\n");
> +		goto out;
> +	}
> +

I thought we're all for removing the call altogether aren't we?

I'd say just call device->query_device() instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Dec. 17, 2015, 5:41 p.m. UTC | #2 
On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
> 
> >+	ret = ib_query_device(device, &device->attrs);
> >+	if (ret) {
> >+		printk(KERN_WARNING "Couldn't query the device attributes\n");
> >+		goto out;
> >+	}
> >+
> 
> I thought we're all for removing the call altogether aren't we?
> 
> I'd say just call device->query_device() instead.

Christoph's patch even got rid of device->query_device(), which, IHMO,
I prefer to see over this. It re-enforces that these values are
constants and drivers cannot change them on the fly.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Dec. 17, 2015, 6:31 p.m. UTC | #3 
On 12/17/2015 06:41 PM, Jason Gunthorpe wrote:
> On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
>>
>>> +	ret = ib_query_device(device, &device->attrs);
>>> +	if (ret) {
>>> +		printk(KERN_WARNING "Couldn't query the device attributes\n");
>>> +		goto out;
>>> +	}
>>> +
>>
>> I thought we're all for removing the call altogether aren't we?
>>
>> I'd say just call device->query_device() instead.
>
> Christoph's patch even got rid of device->query_device(), which, IHMO,
> I prefer to see over this. It re-enforces that these values are
> constants and drivers cannot change them on the fly.

I also would like to see the query_device() implementations to be 
removed from the hw drivers.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Dec. 18, 2015, 5:16 a.m. UTC | #4 
On Thu, Dec 17, 2015 at 07:31:20PM +0100, Bart Van Assche wrote:
> On 12/17/2015 06:41 PM, Jason Gunthorpe wrote:
> >On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
> >>
> >>>+	ret = ib_query_device(device, &device->attrs);
> >>>+	if (ret) {
> >>>+		printk(KERN_WARNING "Couldn't query the device 
> >>>attributes\n");
> >>>+		goto out;
> >>>+	}
> >>>+
> >>
> >>I thought we're all for removing the call altogether aren't we?
> >>
> >>I'd say just call device->query_device() instead.
> >
> >Christoph's patch even got rid of device->query_device(), which, IHMO,
> >I prefer to see over this. It re-enforces that these values are
> >constants and drivers cannot change them on the fly.
> 
> I also would like to see the query_device() implementations to be 
> removed from the hw drivers.

As do I.  More than anything what I hate is all the places that allocate struct
ib_device_attr just to free it after the query call.

We discussed this patch ages ago and decided against it and FWIW It does not
hurt my feelings at all to drop it.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz Dec. 18, 2015, 8:08 a.m. UTC | #5 
On 12/17/2015 7:41 PM, Jason Gunthorpe wrote:
> On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
>>> +	ret = ib_query_device(device, &device->attrs);
>>> +	if (ret) {
>>> +		printk(KERN_WARNING "Couldn't query the device attributes\n");
>>> +		goto out;
>>> +	}
>>> +
>> I thought we're all for removing the call altogether aren't we?
>>
>> I'd say just call device->query_device() instead.
> Christoph's patch even got rid of device->query_device(),

Wrong.

UDATA is a mechanism added by the founders of the RDMA stack to allow 
for LL HW
driver pass data to LL HW user space library piggy backed on uverbs 
commands.

For example, mlx4 uses that to path details of the HW clock which are 
needed for libmlx4.

Christoph noted that and didn't remove the query_device entry from mlx4, he
also madethis somehow inelegant change to uverbs_cmd.c:

@@ -3624,26 +3607,29 @@ int ib_uverbs_ex_query_device(struct 
ib_uverbs_file *file,
         if (ucore->outlen < resp.response_length)
                 return -ENOSPC;

-       memset(&attr, 0, sizeof(attr));
-
-       err = ib_dev->query_device(ib_dev, &attr, uhw);
-       if (err)
-               return err;
+       if (ib_dev->query_device) {
+               int err = ib_dev->query_device(ib_dev, uhw);
+               if (err)
+                       return err;
+       } else {
+               if (uhw->inlen || uhw->outlen)
+                       return -EINVAL;
+       }


> which, IHMO, I prefer to see over this.

It would be wrong to disallow LL drivers to pass data back and forth to/from
their user space sister libraries, this is part of a 10Y old kernel UAPI.

> It re-enforces that these values are constants and drivers cannot change them on the fly.

yes, we should be looking on how to constify things, but incrementally.
I don't see why block this cleanup now as of this point.

Or.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz Dec. 18, 2015, 9:01 a.m. UTC | #6 
On Fri, Dec 18, 2015 at 7:16 AM, ira.weiny <ira.weiny@intel.com> wrote:

> More than anything what I hate is all the places that allocate struct
> ib_device_attr just to free it after the query call.

did you care to look on patches 2-6, this is exactly what they are doing.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Gunthorpe Dec. 18, 2015, 4:49 p.m. UTC | #7 
On Fri, Dec 18, 2015 at 10:08:41AM +0200, Or Gerlitz wrote:
> On 12/17/2015 7:41 PM, Jason Gunthorpe wrote:
> >On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
> >>>+	ret = ib_query_device(device, &device->attrs);
> >>>+	if (ret) {
> >>>+		printk(KERN_WARNING "Couldn't query the device attributes\n");
> >>>+		goto out;
> >>>+	}
> >>>+
> >>I thought we're all for removing the call altogether aren't we?
> >>
> >>I'd say just call device->query_device() instead.
> >Christoph's patch even got rid of device->query_device(),
> 
> Wrong.

Not really, lots of hunks in Christoph's patch are removing
query_device ie:

@@ -1305,7 +1299,6 @@  int mthca_register_device(struct mthca_dev *dev)
 	dev->ib_dev.phys_port_cnt        = dev->limits.num_ports;
 	dev->ib_dev.num_comp_vectors     = 1;
 	dev->ib_dev.dma_device           = &dev->pdev->dev;
-	dev->ib_dev.query_device         = mthca_query_device;
 	dev->ib_dev.query_port           = mthca_query_port;

Sure, it sticks around in a couple places but it isn't 'query_device'
anymore, it is 'query_device_udata' which is reasonable.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ira Weiny Dec. 19, 2015, 12:50 a.m. UTC | #8 
On Fri, Dec 18, 2015 at 11:01:24AM +0200, Or Gerlitz wrote:
> On Fri, Dec 18, 2015 at 7:16 AM, ira.weiny <ira.weiny@intel.com> wrote:
> 
> > More than anything what I hate is all the places that allocate struct
> > ib_device_attr just to free it after the query call.
> 
> did you care to look on patches 2-6, this is exactly what they are doing.

Yes I did.  And I don't see how this statement is a problem as that is what
both your's and Christophs patches do.

If anything this was a statement in support of your series.  I'm sorry you
don't see it that way.

However, if we are getting rid of query_device and the last remaining call is
in this first patch why not go all the way and remove that call site as well?

Which is why I also said:

"We discussed this patch ages ago and decided against it and FWIW It does not
hurt my feelings at all to drop it."

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Or Gerlitz Dec. 20, 2015, 5:05 p.m. UTC | #9 
On 12/18/2015 6:49 PM, Jason Gunthorpe wrote:
> On Fri, Dec 18, 2015 at 10:08:41AM +0200, Or Gerlitz wrote:
>> On 12/17/2015 7:41 PM, Jason Gunthorpe wrote:
>>> On Thu, Dec 17, 2015 at 03:44:19PM +0200, Sagi Grimberg wrote:
>>>>> +	ret = ib_query_device(device, &device->attrs);
>>>>> +	if (ret) {
>>>>> +		printk(KERN_WARNING "Couldn't query the device attributes\n");
>>>>> +		goto out;
>>>>> +	}
>>>>> +
>>>> I thought we're all for removing the call altogether aren't we?
>>>>
>>>> I'd say just call device->query_device() instead.
>>> Christoph's patch even got rid of device->query_device(),
>> Wrong.
> Not really, lots of hunks in Christoph's patch are removing query_device ie:
>
> @@ -1305,7 +1299,6 @@  int mthca_register_device(struct mthca_dev *dev)
>   	dev->ib_dev.phys_port_cnt        = dev->limits.num_ports;
>   	dev->ib_dev.num_comp_vectors     = 1;
>   	dev->ib_dev.dma_device           = &dev->pdev->dev;
> -	dev->ib_dev.query_device         = mthca_query_device;
>   	dev->ib_dev.query_port           = mthca_query_port;

I know, I wanted to stress the point that under the udata architecture 
the query_device callbacked can't be just deleted since drivers use that 
to get/provide data to/from their user-space libraries.

> Sure, it sticks around in a couple places but it isn't 'query_device'
> anymore, it is 'query_device_udata' which is reasonable.
>

I see what you mean. But this involved somehow deeper patching to mlx4 
which should be applied later on to any ll driver that would like to 
expose udata for their query device entry (and return back that entry as 
well after we delete it now in Christoph's patch).

Or.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
index 179e813..398353b 100644
--- a/drivers/infiniband/core/device.c
+++ b/drivers/infiniband/core/device.c
@@ -352,6 +352,12 @@  int ib_register_device(struct ib_device *device,
 		goto out;
 	}
 
+	ret = ib_query_device(device, &device->attrs);
+	if (ret) {
+		printk(KERN_WARNING "Couldn't query the device attributes\n");
+		goto out;
+	}
+
 	ret = ib_device_register_sysfs(device, port_callback);
 	if (ret) {
 		printk(KERN_WARNING "Couldn't register device %s with driver model\n",
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index d56b39a..75a22533 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1828,6 +1828,7 @@  struct ib_device {
 	u16                          is_switch:1;
 	u8                           node_type;
 	u8                           phys_port_cnt;
+	struct ib_device_attr        attrs;
 
 	/**
 	 * The following mandatory functions are used only at device