[PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

Provider specific attributes which are necessary for the userspace
functionality should be part of the alloc ucontext response, not query
device. This way a userspace provider could work without issuing a query
device verb call. However, the fields will remain in the query device
ABI in order to maintain backwards compatibility.

Reviewed-by: Firas JahJah <firasj@amazon.com>
Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
PR was sent:
https://github.com/linux-rdma/rdma-core/pull/775
---
 drivers/infiniband/hw/efa/efa_verbs.c | 10 ++++++++++
 include/uapi/rdma/efa-abi.h           | 23 ++++++++++++++++++++++-
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index 08313f7c73bc..519cc959acfe 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -1520,11 +1520,21 @@ int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
 
 	ucontext->uarn = result.uarn;
 
+	resp.comp_mask |= EFA_ALLOC_UCONTEXT_RESP_DEV_ATTR;
 	resp.cmds_supp_udata_mask |= EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE;
 	resp.cmds_supp_udata_mask |= EFA_USER_CMDS_SUPP_UDATA_CREATE_AH;
 	resp.sub_cqs_per_cq = dev->dev_attr.sub_cqs_per_cq;
 	resp.inline_buf_size = dev->dev_attr.inline_buf_size;
 	resp.max_llq_size = dev->dev_attr.max_llq_size;
+	resp.max_sq_sge = dev->dev_attr.max_sq_sge;
+	resp.max_rq_sge = dev->dev_attr.max_rq_sge;
+	resp.max_sq_wr = dev->dev_attr.max_sq_depth;
+	resp.max_rq_wr = dev->dev_attr.max_rq_depth;
+	resp.max_rdma_size = dev->dev_attr.max_rdma_size;
+	resp.max_wr_rdma_sge = dev->dev_attr.max_wr_rdma_sge;
+
+	if (is_rdma_read_cap(dev))
+		resp.device_caps |= EFA_ALLOC_UCONTEXT_DEVICE_CAPS_RDMA_READ;
 
 	if (udata && udata->outlen) {
 		err = ib_copy_to_udata(udata, &resp,
diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h
index 53b6e2036a9b..12df5c1659b6 100644
--- a/include/uapi/rdma/efa-abi.h
+++ b/include/uapi/rdma/efa-abi.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
 /*
- * Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All rights reserved.
+ * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
  */
 
 #ifndef EFA_ABI_USER_H
@@ -20,6 +20,14 @@
  * hex bit offset of the field.
  */
 
+enum {
+	EFA_ALLOC_UCONTEXT_RESP_DEV_ATTR = 1 << 0,
+};
+
+enum {
+	EFA_ALLOC_UCONTEXT_DEVICE_CAPS_RDMA_READ = 1 << 0,
+};
+
 enum efa_ibv_user_cmds_supp_udata {
 	EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE = 1 << 0,
 	EFA_USER_CMDS_SUPP_UDATA_CREATE_AH    = 1 << 1,
@@ -31,6 +39,14 @@ struct efa_ibv_alloc_ucontext_resp {
 	__u16 sub_cqs_per_cq;
 	__u16 inline_buf_size;
 	__u32 max_llq_size; /* bytes */
+	__u32 max_sq_wr;
+	__u32 max_rq_wr;
+	__u16 max_sq_sge;
+	__u16 max_rq_sge;
+	__u32 max_rdma_size;
+	__u32 device_caps;
+	__u16 max_wr_rdma_sge;
+	__u8 reserved_130[2];
 };
 
 struct efa_ibv_alloc_pd_resp {
@@ -96,6 +112,11 @@ enum {
 
 struct efa_ibv_ex_query_device_resp {
 	__u32 comp_mask;
+	/*
+	 * Attributes which are required for userspace provider functionality
+	 * should be in alloc ucontext response, the following fields have been
+	 * moved.
+	 */
 	__u32 max_sq_wr;
 	__u32 max_rq_wr;
 	__u16 max_sq_sge;

base-commit: fba97dc7fc76b2c9a909fa0b3786d30a9899f5cf
-- 
2.27.0

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Leon Romanovsky]

On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
> Provider specific attributes which are necessary for the userspace
> functionality should be part of the alloc ucontext response, not query
> device. This way a userspace provider could work without issuing a query
> device verb call. However, the fields will remain in the query device
> ABI in order to maintain backwards compatibility.

I don't really understand why "should be ..."? Device properties exposed
here are per-device and will be equal to all ucontexts, so instead of
doing one very fast system call, you are "punishing" every ucontext
call.

What is wrong with calling one query_device before allocating any
ucontext? What are you trying to achieve and what will it give?

Thanks

>
> Reviewed-by: Firas JahJah <firasj@amazon.com>
> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
> PR was sent:
> https://github.com/linux-rdma/rdma-core/pull/775
> ---
>  drivers/infiniband/hw/efa/efa_verbs.c | 10 ++++++++++
>  include/uapi/rdma/efa-abi.h           | 23 ++++++++++++++++++++++-
>  2 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
> index 08313f7c73bc..519cc959acfe 100644
> --- a/drivers/infiniband/hw/efa/efa_verbs.c
> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
> @@ -1520,11 +1520,21 @@ int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
>
>  	ucontext->uarn = result.uarn;
>
> +	resp.comp_mask |= EFA_ALLOC_UCONTEXT_RESP_DEV_ATTR;
>  	resp.cmds_supp_udata_mask |= EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE;
>  	resp.cmds_supp_udata_mask |= EFA_USER_CMDS_SUPP_UDATA_CREATE_AH;
>  	resp.sub_cqs_per_cq = dev->dev_attr.sub_cqs_per_cq;
>  	resp.inline_buf_size = dev->dev_attr.inline_buf_size;
>  	resp.max_llq_size = dev->dev_attr.max_llq_size;
> +	resp.max_sq_sge = dev->dev_attr.max_sq_sge;
> +	resp.max_rq_sge = dev->dev_attr.max_rq_sge;
> +	resp.max_sq_wr = dev->dev_attr.max_sq_depth;
> +	resp.max_rq_wr = dev->dev_attr.max_rq_depth;
> +	resp.max_rdma_size = dev->dev_attr.max_rdma_size;
> +	resp.max_wr_rdma_sge = dev->dev_attr.max_wr_rdma_sge;
> +
> +	if (is_rdma_read_cap(dev))
> +		resp.device_caps |= EFA_ALLOC_UCONTEXT_DEVICE_CAPS_RDMA_READ;
>
>  	if (udata && udata->outlen) {
>  		err = ib_copy_to_udata(udata, &resp,
> diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h
> index 53b6e2036a9b..12df5c1659b6 100644
> --- a/include/uapi/rdma/efa-abi.h
> +++ b/include/uapi/rdma/efa-abi.h
> @@ -1,6 +1,6 @@
>  /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */
>  /*
> - * Copyright 2018-2019 Amazon.com, Inc. or its affiliates. All rights reserved.
> + * Copyright 2018-2020 Amazon.com, Inc. or its affiliates. All rights reserved.
>   */
>
>  #ifndef EFA_ABI_USER_H
> @@ -20,6 +20,14 @@
>   * hex bit offset of the field.
>   */
>
> +enum {
> +	EFA_ALLOC_UCONTEXT_RESP_DEV_ATTR = 1 << 0,
> +};
> +
> +enum {
> +	EFA_ALLOC_UCONTEXT_DEVICE_CAPS_RDMA_READ = 1 << 0,
> +};
> +
>  enum efa_ibv_user_cmds_supp_udata {
>  	EFA_USER_CMDS_SUPP_UDATA_QUERY_DEVICE = 1 << 0,
>  	EFA_USER_CMDS_SUPP_UDATA_CREATE_AH    = 1 << 1,
> @@ -31,6 +39,14 @@ struct efa_ibv_alloc_ucontext_resp {
>  	__u16 sub_cqs_per_cq;
>  	__u16 inline_buf_size;
>  	__u32 max_llq_size; /* bytes */
> +	__u32 max_sq_wr;
> +	__u32 max_rq_wr;
> +	__u16 max_sq_sge;
> +	__u16 max_rq_sge;
> +	__u32 max_rdma_size;
> +	__u32 device_caps;
> +	__u16 max_wr_rdma_sge;
> +	__u8 reserved_130[2];
>  };
>
>  struct efa_ibv_alloc_pd_resp {
> @@ -96,6 +112,11 @@ enum {
>
>  struct efa_ibv_ex_query_device_resp {
>  	__u32 comp_mask;
> +	/*
> +	 * Attributes which are required for userspace provider functionality
> +	 * should be in alloc ucontext response, the following fields have been
> +	 * moved.
> +	 */
>  	__u32 max_sq_wr;
>  	__u32 max_rq_wr;
>  	__u16 max_sq_sge;
>
> base-commit: fba97dc7fc76b2c9a909fa0b3786d30a9899f5cf
> --
> 2.27.0
>

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

On 16/06/2020 9:30, Leon Romanovsky wrote:
> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
>> Provider specific attributes which are necessary for the userspace
>> functionality should be part of the alloc ucontext response, not query
>> device. This way a userspace provider could work without issuing a query
>> device verb call. However, the fields will remain in the query device
>> ABI in order to maintain backwards compatibility.
> 
> I don't really understand why "should be ..."? Device properties exposed
> here are per-device and will be equal to all ucontexts, so instead of
> doing one very fast system call, you are "punishing" every ucontext
> call.

I talked about it with Jason in the past, the query device verb is intended to
follow the IBA verb, alloc ucontext should return driver specific data that's
required to operate the user space provider.
A query device call should not be mandatory to load the provider.

Whether it's done through query device/ucontext response, both happen for each
new context call. With this patch, we gather all needed data in one system call
instead of two.

> What is wrong with calling one query_device before allocating any
> ucontext? What are you trying to achieve and what will it give?

How can you call query device without allocating a context?

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Leon Romanovsky]

On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
> On 16/06/2020 9:30, Leon Romanovsky wrote:
> > On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
> >> Provider specific attributes which are necessary for the userspace
> >> functionality should be part of the alloc ucontext response, not query
> >> device. This way a userspace provider could work without issuing a query
> >> device verb call. However, the fields will remain in the query device
> >> ABI in order to maintain backwards compatibility.
> >
> > I don't really understand why "should be ..."? Device properties exposed
> > here are per-device and will be equal to all ucontexts, so instead of
> > doing one very fast system call, you are "punishing" every ucontext
> > call.
>
> I talked about it with Jason in the past, the query device verb is intended to
> follow the IBA verb, alloc ucontext should return driver specific data that's
> required to operate the user space provider.
> A query device call should not be mandatory to load the provider.

Why? query_device is declared as mandatory verb for any provider, so
anyway all in-the-tree RDMA drivers will have such verb.

>
> Whether it's done through query device/ucontext response, both happen for each
> new context call. With this patch, we gather all needed data in one system call
> instead of two.

Is it important in control path to have one call?

>
> > What is wrong with calling one query_device before allocating any
> > ucontext? What are you trying to achieve and what will it give?
>
> How can you call query device without allocating a context?

Forget about my comment above, it was my over-thinking.

I had in mind some scheme that first ucontext will cache the all device
related data and share it with other ucontexts.

Thanks

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

On 16/06/2020 12:38, Leon Romanovsky wrote:
> On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
>> On 16/06/2020 9:30, Leon Romanovsky wrote:
>>> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
>>>> Provider specific attributes which are necessary for the userspace
>>>> functionality should be part of the alloc ucontext response, not query
>>>> device. This way a userspace provider could work without issuing a query
>>>> device verb call. However, the fields will remain in the query device
>>>> ABI in order to maintain backwards compatibility.
>>>
>>> I don't really understand why "should be ..."? Device properties exposed
>>> here are per-device and will be equal to all ucontexts, so instead of
>>> doing one very fast system call, you are "punishing" every ucontext
>>> call.
>>
>> I talked about it with Jason in the past, the query device verb is intended to
>> follow the IBA verb, alloc ucontext should return driver specific data that's
>> required to operate the user space provider.
>> A query device call should not be mandatory to load the provider.
> 
> Why? query_device is declared as mandatory verb for any provider, so
> anyway all in-the-tree RDMA drivers will have such verb.

I don't think the concern here is if the verb exists or not, my understanding is
that query device should be used for IBA query device attributes, not other
provider specific stuff.
Jason, want to chime in with your thoughts?

>> Whether it's done through query device/ucontext response, both happen for each
>> new context call. With this patch, we gather all needed data in one system call
>> instead of two.
> 
> Is it important in control path to have one call?

Not a huge difference, better one than two though.

>>> What is wrong with calling one query_device before allocating any
>>> ucontext? What are you trying to achieve and what will it give?
>>
>> How can you call query device without allocating a context?
> 
> Forget about my comment above, it was my over-thinking.
> 
> I had in mind some scheme that first ucontext will cache the all device
> related data and share it with other ucontexts.
> 
> Thanks
> 

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Leon Romanovsky]

On Tue, Jun 16, 2020 at 08:44:37PM +0300, Gal Pressman wrote:
> On 16/06/2020 12:38, Leon Romanovsky wrote:
> > On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
> >> On 16/06/2020 9:30, Leon Romanovsky wrote:
> >>> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
> >>>> Provider specific attributes which are necessary for the userspace
> >>>> functionality should be part of the alloc ucontext response, not query
> >>>> device. This way a userspace provider could work without issuing a query
> >>>> device verb call. However, the fields will remain in the query device
> >>>> ABI in order to maintain backwards compatibility.
> >>>
> >>> I don't really understand why "should be ..."? Device properties exposed
> >>> here are per-device and will be equal to all ucontexts, so instead of
> >>> doing one very fast system call, you are "punishing" every ucontext
> >>> call.
> >>
> >> I talked about it with Jason in the past, the query device verb is intended to
> >> follow the IBA verb, alloc ucontext should return driver specific data that's
> >> required to operate the user space provider.
> >> A query device call should not be mandatory to load the provider.
> >
> > Why? query_device is declared as mandatory verb for any provider, so
> > anyway all in-the-tree RDMA drivers will have such verb.
>
> I don't think the concern here is if the verb exists or not, my understanding is
> that query device should be used for IBA query device attributes, not other
> provider specific stuff.
> Jason, want to chime in with your thoughts?

I would like to hear it too. Almost all (if not all) extended
verbs support providing vendor specific data in a response. Why
should query_device() be different here?

>
> >> Whether it's done through query device/ucontext response, both happen for each
> >> new context call. With this patch, we gather all needed data in one system call
> >> instead of two.
> >
> > Is it important in control path to have one call?
>
> Not a huge difference, better one than two though.

My problem here is duplication of functionality and data.

>
> >>> What is wrong with calling one query_device before allocating any
> >>> ucontext? What are you trying to achieve and what will it give?
> >>
> >> How can you call query device without allocating a context?
> >
> > Forget about my comment above, it was my over-thinking.
> >
> > I had in mind some scheme that first ucontext will cache the all device
> > related data and share it with other ucontexts.
> >
> > Thanks
> >

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Jason Gunthorpe]

On Tue, Jun 16, 2020 at 08:44:37PM +0300, Gal Pressman wrote:
> On 16/06/2020 12:38, Leon Romanovsky wrote:
> > On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
> >> On 16/06/2020 9:30, Leon Romanovsky wrote:
> >>> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
> >>>> Provider specific attributes which are necessary for the userspace
> >>>> functionality should be part of the alloc ucontext response, not query
> >>>> device. This way a userspace provider could work without issuing a query
> >>>> device verb call. However, the fields will remain in the query device
> >>>> ABI in order to maintain backwards compatibility.
> >>>
> >>> I don't really understand why "should be ..."? Device properties exposed
> >>> here are per-device and will be equal to all ucontexts, so instead of
> >>> doing one very fast system call, you are "punishing" every ucontext
> >>> call.
> >>
> >> I talked about it with Jason in the past, the query device verb is intended to
> >> follow the IBA verb, alloc ucontext should return driver specific data that's
> >> required to operate the user space provider.
> >> A query device call should not be mandatory to load the provider.
> > 
> > Why? query_device is declared as mandatory verb for any provider, so
> > anyway all in-the-tree RDMA drivers will have such verb.
> 
> I don't think the concern here is if the verb exists or not, my understanding is
> that query device should be used for IBA query device attributes, not other
> provider specific stuff.
> Jason, want to chime in with your thoughts?

query_device should be used to implement the ibverb query_device and
query_device_ex

It should only return rdma-core defined common stuff because that is
what that verb does - there is no reason to return driver specific
things as there is nothing the driver can do with it.

The only exception might be some provider specific query_device dv
that needs more information.

query_device should not be used as some two-part
create_context. Information related only to create_context that is not
already exposed to query_device should not be added to query_device
only for create_context's use.

Similarly, information in query_device should not be duplicated into
create_context just to save a system call.

Jason

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

On 17/06/2020 18:36, Jason Gunthorpe wrote:
> On Tue, Jun 16, 2020 at 08:44:37PM +0300, Gal Pressman wrote:
>> On 16/06/2020 12:38, Leon Romanovsky wrote:
>>> On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
>>>> On 16/06/2020 9:30, Leon Romanovsky wrote:
>>>>> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
>>>>>> Provider specific attributes which are necessary for the userspace
>>>>>> functionality should be part of the alloc ucontext response, not query
>>>>>> device. This way a userspace provider could work without issuing a query
>>>>>> device verb call. However, the fields will remain in the query device
>>>>>> ABI in order to maintain backwards compatibility.
>>>>>
>>>>> I don't really understand why "should be ..."? Device properties exposed
>>>>> here are per-device and will be equal to all ucontexts, so instead of
>>>>> doing one very fast system call, you are "punishing" every ucontext
>>>>> call.
>>>>
>>>> I talked about it with Jason in the past, the query device verb is intended to
>>>> follow the IBA verb, alloc ucontext should return driver specific data that's
>>>> required to operate the user space provider.
>>>> A query device call should not be mandatory to load the provider.
>>>
>>> Why? query_device is declared as mandatory verb for any provider, so
>>> anyway all in-the-tree RDMA drivers will have such verb.
>>
>> I don't think the concern here is if the verb exists or not, my understanding is
>> that query device should be used for IBA query device attributes, not other
>> provider specific stuff.
>> Jason, want to chime in with your thoughts?
> 
> query_device should be used to implement the ibverb query_device and
> query_device_ex
> 
> It should only return rdma-core defined common stuff because that is
> what that verb does - there is no reason to return driver specific
> things as there is nothing the driver can do with it.
> 
> The only exception might be some provider specific query_device dv
> that needs more information.
> 
> query_device should not be used as some two-part
> create_context. Information related only to create_context that is not
> already exposed to query_device should not be added to query_device
> only for create_context's use.
> 
> Similarly, information in query_device should not be duplicated into
> create_context just to save a system call.

That makes sense.
To clarify, the "duplicated" fields in this patch are moved to the ucontext
allocation, where they originally belong as all of them are necessary for the
provider's functionality.
Future fields such as these will only be added to alloc_ucontext, not both, so
there's no duplication.

Otherwise, future extensions will either have to be added to query_device, which
is the wrong place, just to be consistent with the existing code. Or we add them
to the ucontext response, where they belong, and end up with some hybrid
solution where different fields are gathered from different places (yuck :\).

We got it wrong the first place but it's a two way door, let's fix it.

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

On 17/06/2020 20:49, Gal Pressman wrote:
> On 17/06/2020 18:36, Jason Gunthorpe wrote:
>> On Tue, Jun 16, 2020 at 08:44:37PM +0300, Gal Pressman wrote:
>>> On 16/06/2020 12:38, Leon Romanovsky wrote:
>>>> On Tue, Jun 16, 2020 at 11:53:11AM +0300, Gal Pressman wrote:
>>>>> On 16/06/2020 9:30, Leon Romanovsky wrote:
>>>>>> On Mon, Jun 15, 2020 at 10:59:20AM +0300, Gal Pressman wrote:
>>>>>>> Provider specific attributes which are necessary for the userspace
>>>>>>> functionality should be part of the alloc ucontext response, not query
>>>>>>> device. This way a userspace provider could work without issuing a query
>>>>>>> device verb call. However, the fields will remain in the query device
>>>>>>> ABI in order to maintain backwards compatibility.
>>>>>>
>>>>>> I don't really understand why "should be ..."? Device properties exposed
>>>>>> here are per-device and will be equal to all ucontexts, so instead of
>>>>>> doing one very fast system call, you are "punishing" every ucontext
>>>>>> call.
>>>>>
>>>>> I talked about it with Jason in the past, the query device verb is intended to
>>>>> follow the IBA verb, alloc ucontext should return driver specific data that's
>>>>> required to operate the user space provider.
>>>>> A query device call should not be mandatory to load the provider.
>>>>
>>>> Why? query_device is declared as mandatory verb for any provider, so
>>>> anyway all in-the-tree RDMA drivers will have such verb.
>>>
>>> I don't think the concern here is if the verb exists or not, my understanding is
>>> that query device should be used for IBA query device attributes, not other
>>> provider specific stuff.
>>> Jason, want to chime in with your thoughts?
>>
>> query_device should be used to implement the ibverb query_device and
>> query_device_ex
>>
>> It should only return rdma-core defined common stuff because that is
>> what that verb does - there is no reason to return driver specific
>> things as there is nothing the driver can do with it.
>>
>> The only exception might be some provider specific query_device dv
>> that needs more information.
>>
>> query_device should not be used as some two-part
>> create_context. Information related only to create_context that is not
>> already exposed to query_device should not be added to query_device
>> only for create_context's use.
>>
>> Similarly, information in query_device should not be duplicated into
>> create_context just to save a system call.
> 
> That makes sense.
> To clarify, the "duplicated" fields in this patch are moved to the ucontext
> allocation, where they originally belong as all of them are necessary for the
> provider's functionality.
> Future fields such as these will only be added to alloc_ucontext, not both, so
> there's no duplication.
> 
> Otherwise, future extensions will either have to be added to query_device, which
> is the wrong place, just to be consistent with the existing code. Or we add them
> to the ucontext response, where they belong, and end up with some hybrid
> solution where different fields are gathered from different places (yuck :\).
> 
> We got it wrong the first place but it's a two way door, let's fix it.

Uhh.. We can't really get rid of the query device call as the provider needs the
max_qp attribute in order to allocate the QP table properly.

I still think we should move the fields to keep things clean, but I can drop
this change if you prefer to avoid the churn. The provider will always call
query device on context initialization and gather different fields from
different system calls.

Thoughts?

Re: [PATCH for-next] RDMA/efa: Move provider specific attributes to ucontext allocation response

[Gal Pressman]

On 15/06/2020 10:59, Gal Pressman wrote:
> Provider specific attributes which are necessary for the userspace
> functionality should be part of the alloc ucontext response, not query
> device. This way a userspace provider could work without issuing a query
> device verb call. However, the fields will remain in the query device
> ABI in order to maintain backwards compatibility.
> 
> Reviewed-by: Firas JahJah <firasj@amazon.com>
> Reviewed-by: Yossi Leybovich <sleybo@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
> PR was sent:
> https://github.com/linux-rdma/rdma-core/pull/775

This patch can be dropped following the discussion.