[PATCH for-next 0/2] Host information userspace version

[PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

The following two patches add the userspace version to the host
information struct reported to the device, used for debugging and
troubleshooting purposes.

PR was sent:
https://github.com/linux-rdma/rdma-core/pull/918

Thanks,
Gal

Gal Pressman (2):
  RDMA/efa: Move host info set to first ucontext allocation
  RDMA/efa: Report userspace version in host info

 drivers/infiniband/hw/efa/efa.h                 | 7 +++++++
 drivers/infiniband/hw/efa/efa_admin_cmds_defs.h | 3 +++
 drivers/infiniband/hw/efa/efa_main.c            | 6 +++---
 drivers/infiniband/hw/efa/efa_verbs.c           | 3 +++
 include/uapi/rdma/efa-abi.h                     | 3 ++-
 5 files changed, 18 insertions(+), 4 deletions(-)


base-commit: e246b7c035d74abfb3507fa10082d0c42cc016c3
-- 
2.30.0

[PATCH for-next 1/2] RDMA/efa: Move host info set to first ucontext allocation

[Gal Pressman]

Downstream patch will require the userspace version which is passed as
part of ucontext allocation. Move the host info set there and make sure
it's only called once (on the first allocation).

Reviewed-by: Firas JahJah <firasj@amazon.com>
Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/hw/efa/efa.h       | 7 +++++++
 drivers/infiniband/hw/efa/efa_main.c  | 4 +---
 drivers/infiniband/hw/efa/efa_verbs.c | 3 +++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
index e5d9712e98c4..9c9cd5867489 100644
--- a/drivers/infiniband/hw/efa/efa.h
+++ b/drivers/infiniband/hw/efa/efa.h
@@ -45,6 +45,11 @@ struct efa_stats {
 	atomic64_t keep_alive_rcvd;
 };
 
+enum {
+	EFA_FLAGS_HOST_INFO_SET_BIT,
+	EFA_FLAGS_NUM,
+};
+
 struct efa_dev {
 	struct ib_device ibdev;
 	struct efa_com_dev edev;
@@ -62,6 +67,7 @@ struct efa_dev {
 	struct efa_irq admin_irq;
 
 	struct efa_stats stats;
+	DECLARE_BITMAP(flags, EFA_FLAGS_NUM);
 };
 
 struct efa_ucontext {
@@ -117,6 +123,7 @@ struct efa_ah {
 	u8 id[EFA_GID_SIZE];
 };
 
+void efa_set_host_info(struct efa_dev *dev);
 int efa_query_device(struct ib_device *ibdev,
 		     struct ib_device_attr *props,
 		     struct ib_udata *udata);
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 0f578734bddb..90a033a6af6c 100644
--- a/drivers/infiniband/hw/efa/efa_main.c
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -191,7 +191,7 @@ static void efa_stats_init(struct efa_dev *dev)
 		atomic64_set(s, 0);
 }
 
-static void efa_set_host_info(struct efa_dev *dev)
+void efa_set_host_info(struct efa_dev *dev)
 {
 	struct efa_admin_set_feature_resp resp = {};
 	struct efa_admin_set_feature_cmd cmd = {};
@@ -301,8 +301,6 @@ static int efa_ib_device_add(struct efa_dev *dev)
 	if (err)
 		goto err_release_doorbell_bar;
 
-	efa_set_host_info(dev);
-
 	dev->ibdev.node_type = RDMA_NODE_UNSPECIFIED;
 	dev->ibdev.phys_port_cnt = 1;
 	dev->ibdev.num_comp_vectors = 1;
diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index 2fe5708b2d9d..5c12bdc28ef0 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -1695,6 +1695,9 @@ int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
 		goto err_out;
 	}
 
+	if (!test_and_set_bit(EFA_FLAGS_HOST_INFO_SET_BIT, dev->flags))
+		efa_set_host_info(dev);
+
 	err = efa_user_comp_handshake(ibucontext, &cmd);
 	if (err)
 		goto err_out;
-- 
2.30.0

[PATCH for-next 2/2] RDMA/efa: Report userspace version in host info

[Gal Pressman]

Report the userspace version in the host information set feature admin
command.

Reviewed-by: Firas JahJah <firasj@amazon.com>
Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
Signed-off-by: Gal Pressman <galpress@amazon.com>
---
 drivers/infiniband/hw/efa/efa.h                 | 2 +-
 drivers/infiniband/hw/efa/efa_admin_cmds_defs.h | 3 +++
 drivers/infiniband/hw/efa/efa_main.c            | 4 +++-
 drivers/infiniband/hw/efa/efa_verbs.c           | 2 +-
 include/uapi/rdma/efa-abi.h                     | 3 ++-
 5 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
index 9c9cd5867489..e554b53997c4 100644
--- a/drivers/infiniband/hw/efa/efa.h
+++ b/drivers/infiniband/hw/efa/efa.h
@@ -123,7 +123,7 @@ struct efa_ah {
 	u8 id[EFA_GID_SIZE];
 };
 
-void efa_set_host_info(struct efa_dev *dev);
+void efa_set_host_info(struct efa_dev *dev, u8 *userspace_ver);
 int efa_query_device(struct ib_device *ibdev,
 		     struct ib_device_attr *props,
 		     struct ib_udata *udata);
diff --git a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
index b199e4ac6cf9..0822a5d5dcb6 100644
--- a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
+++ b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
@@ -884,6 +884,9 @@ struct efa_admin_host_info {
 	 * 31:2 : reserved2
 	 */
 	u32 flags;
+
+	/* Userspace version */
+	u8 userspace_ver[32];
 };
 
 /* create_qp_cmd */
diff --git a/drivers/infiniband/hw/efa/efa_main.c b/drivers/infiniband/hw/efa/efa_main.c
index 90a033a6af6c..027f9d0ebf25 100644
--- a/drivers/infiniband/hw/efa/efa_main.c
+++ b/drivers/infiniband/hw/efa/efa_main.c
@@ -191,7 +191,7 @@ static void efa_stats_init(struct efa_dev *dev)
 		atomic64_set(s, 0);
 }
 
-void efa_set_host_info(struct efa_dev *dev)
+void efa_set_host_info(struct efa_dev *dev, u8 *userspace_ver)
 {
 	struct efa_admin_set_feature_resp resp = {};
 	struct efa_admin_set_feature_cmd cmd = {};
@@ -230,6 +230,8 @@ void efa_set_host_info(struct efa_dev *dev)
 		EFA_COMMON_SPEC_VERSION_MINOR);
 	EFA_SET(&hinf->flags, EFA_ADMIN_HOST_INFO_INTREE, 1);
 	EFA_SET(&hinf->flags, EFA_ADMIN_HOST_INFO_GDR, 0);
+	strlcpy(hinf->userspace_ver, userspace_ver,
+		sizeof(hinf->userspace_ver));
 
 	efa_com_set_feature_ex(&dev->edev, &resp, &cmd, EFA_ADMIN_HOST_INFO,
 			       hinf_dma, bufsz);
diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
index 5c12bdc28ef0..d3e3787a9e1e 100644
--- a/drivers/infiniband/hw/efa/efa_verbs.c
+++ b/drivers/infiniband/hw/efa/efa_verbs.c
@@ -1696,7 +1696,7 @@ int efa_alloc_ucontext(struct ib_ucontext *ibucontext, struct ib_udata *udata)
 	}
 
 	if (!test_and_set_bit(EFA_FLAGS_HOST_INFO_SET_BIT, dev->flags))
-		efa_set_host_info(dev);
+		efa_set_host_info(dev, cmd.userspace_ver);
 
 	err = efa_user_comp_handshake(ibucontext, &cmd);
 	if (err)
diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h
index f89fbb5b1e8d..28a8e3f982a0 100644
--- a/include/uapi/rdma/efa-abi.h
+++ b/include/uapi/rdma/efa-abi.h
@@ -27,7 +27,8 @@ enum {
 
 struct efa_ibv_alloc_ucontext_cmd {
 	__u32 comp_mask;
-	__u8 reserved_20[4];
+	__u8 userspace_ver[32];
+	__u8 reserved_120[4];
 };
 
 enum efa_ibv_user_cmds_supp_udata {
-- 
2.30.0

Re: [PATCH for-next 1/2] RDMA/efa: Move host info set to first ucontext allocation

[Leon Romanovsky]

On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote:
> Downstream patch will require the userspace version which is passed as
> part of ucontext allocation. Move the host info set there and make sure
> it's only called once (on the first allocation).
>
> Reviewed-by: Firas JahJah <firasj@amazon.com>
> Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
> Signed-off-by: Gal Pressman <galpress@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa.h       | 7 +++++++
>  drivers/infiniband/hw/efa/efa_main.c  | 4 +---
>  drivers/infiniband/hw/efa/efa_verbs.c | 3 +++
>  3 files changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
> index e5d9712e98c4..9c9cd5867489 100644
> --- a/drivers/infiniband/hw/efa/efa.h
> +++ b/drivers/infiniband/hw/efa/efa.h
> @@ -45,6 +45,11 @@ struct efa_stats {
>  	atomic64_t keep_alive_rcvd;
>  };
>
> +enum {
> +	EFA_FLAGS_HOST_INFO_SET_BIT,
> +	EFA_FLAGS_NUM,
> +};
> +
>  struct efa_dev {
>  	struct ib_device ibdev;
>  	struct efa_com_dev edev;
> @@ -62,6 +67,7 @@ struct efa_dev {
>  	struct efa_irq admin_irq;
>
>  	struct efa_stats stats;
> +	DECLARE_BITMAP(flags, EFA_FLAGS_NUM);
>  };

Why do you need such over-engineering?
What is wrong with old school "u8 flag"?

Thanks

Re: [PATCH for-next 1/2] RDMA/efa: Move host info set to first ucontext allocation

[Gal Pressman]

On 05/01/2021 13:21, Leon Romanovsky wrote:
> On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote:
>> Downstream patch will require the userspace version which is passed as
>> part of ucontext allocation. Move the host info set there and make sure
>> it's only called once (on the first allocation).
>>
>> Reviewed-by: Firas JahJah <firasj@amazon.com>
>> Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>> ---
>>  drivers/infiniband/hw/efa/efa.h       | 7 +++++++
>>  drivers/infiniband/hw/efa/efa_main.c  | 4 +---
>>  drivers/infiniband/hw/efa/efa_verbs.c | 3 +++
>>  3 files changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
>> index e5d9712e98c4..9c9cd5867489 100644
>> --- a/drivers/infiniband/hw/efa/efa.h
>> +++ b/drivers/infiniband/hw/efa/efa.h
>> @@ -45,6 +45,11 @@ struct efa_stats {
>>       atomic64_t keep_alive_rcvd;
>>  };
>>
>> +enum {
>> +     EFA_FLAGS_HOST_INFO_SET_BIT,
>> +     EFA_FLAGS_NUM,
>> +};
>> +
>>  struct efa_dev {
>>       struct ib_device ibdev;
>>       struct efa_com_dev edev;
>> @@ -62,6 +67,7 @@ struct efa_dev {
>>       struct efa_irq admin_irq;
>>
>>       struct efa_stats stats;
>> +     DECLARE_BITMAP(flags, EFA_FLAGS_NUM);
>>  };
> 
> Why do you need such over-engineering?
> What is wrong with old school "u8 flag"?

The main reason is for the atomic test_and_set_bit() usage, otherwise it would
be an atomic flag, not u8 flag.

Re: [PATCH for-next 1/2] RDMA/efa: Move host info set to first ucontext allocation

[Leon Romanovsky]

On Tue, Jan 05, 2021 at 02:22:23PM +0200, Gal Pressman wrote:
> On 05/01/2021 13:21, Leon Romanovsky wrote:
> > On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote:
> >> Downstream patch will require the userspace version which is passed as
> >> part of ucontext allocation. Move the host info set there and make sure
> >> it's only called once (on the first allocation).
> >>
> >> Reviewed-by: Firas JahJah <firasj@amazon.com>
> >> Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
> >> Signed-off-by: Gal Pressman <galpress@amazon.com>
> >> ---
> >>  drivers/infiniband/hw/efa/efa.h       | 7 +++++++
> >>  drivers/infiniband/hw/efa/efa_main.c  | 4 +---
> >>  drivers/infiniband/hw/efa/efa_verbs.c | 3 +++
> >>  3 files changed, 11 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
> >> index e5d9712e98c4..9c9cd5867489 100644
> >> --- a/drivers/infiniband/hw/efa/efa.h
> >> +++ b/drivers/infiniband/hw/efa/efa.h
> >> @@ -45,6 +45,11 @@ struct efa_stats {
> >>       atomic64_t keep_alive_rcvd;
> >>  };
> >>
> >> +enum {
> >> +     EFA_FLAGS_HOST_INFO_SET_BIT,
> >> +     EFA_FLAGS_NUM,
> >> +};
> >> +
> >>  struct efa_dev {
> >>       struct ib_device ibdev;
> >>       struct efa_com_dev edev;
> >> @@ -62,6 +67,7 @@ struct efa_dev {
> >>       struct efa_irq admin_irq;
> >>
> >>       struct efa_stats stats;
> >> +     DECLARE_BITMAP(flags, EFA_FLAGS_NUM);
> >>  };
> >
> > Why do you need such over-engineering?
> > What is wrong with old school "u8 flag"?
>
> The main reason is for the atomic test_and_set_bit() usage, otherwise it would
> be an atomic flag, not u8 flag.

But efa_dev can be opened with different applications and they can have
different user space versions, but you are setting this info for the
first caller only. How will it help for the debug?

Thanks

Re: [PATCH for-next 1/2] RDMA/efa: Move host info set to first ucontext allocation

[Gal Pressman]

On 05/01/2021 14:40, Leon Romanovsky wrote:
> On Tue, Jan 05, 2021 at 02:22:23PM +0200, Gal Pressman wrote:
>> On 05/01/2021 13:21, Leon Romanovsky wrote:
>>> On Tue, Jan 05, 2021 at 12:43:25PM +0200, Gal Pressman wrote:
>>>> Downstream patch will require the userspace version which is passed as
>>>> part of ucontext allocation. Move the host info set there and make sure
>>>> it's only called once (on the first allocation).
>>>>
>>>> Reviewed-by: Firas JahJah <firasj@amazon.com>
>>>> Reviewed-by: Leonid Feschuk <lfesch@amazon.com>
>>>> Signed-off-by: Gal Pressman <galpress@amazon.com>
>>>> ---
>>>>  drivers/infiniband/hw/efa/efa.h       | 7 +++++++
>>>>  drivers/infiniband/hw/efa/efa_main.c  | 4 +---
>>>>  drivers/infiniband/hw/efa/efa_verbs.c | 3 +++
>>>>  3 files changed, 11 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/efa/efa.h b/drivers/infiniband/hw/efa/efa.h
>>>> index e5d9712e98c4..9c9cd5867489 100644
>>>> --- a/drivers/infiniband/hw/efa/efa.h
>>>> +++ b/drivers/infiniband/hw/efa/efa.h
>>>> @@ -45,6 +45,11 @@ struct efa_stats {
>>>>       atomic64_t keep_alive_rcvd;
>>>>  };
>>>>
>>>> +enum {
>>>> +     EFA_FLAGS_HOST_INFO_SET_BIT,
>>>> +     EFA_FLAGS_NUM,
>>>> +};
>>>> +
>>>>  struct efa_dev {
>>>>       struct ib_device ibdev;
>>>>       struct efa_com_dev edev;
>>>> @@ -62,6 +67,7 @@ struct efa_dev {
>>>>       struct efa_irq admin_irq;
>>>>
>>>>       struct efa_stats stats;
>>>> +     DECLARE_BITMAP(flags, EFA_FLAGS_NUM);
>>>>  };
>>>
>>> Why do you need such over-engineering?
>>> What is wrong with old school "u8 flag"?
>>
>> The main reason is for the atomic test_and_set_bit() usage, otherwise it would
>> be an atomic flag, not u8 flag.
> 
> But efa_dev can be opened with different applications and they can have
> different user space versions, but you are setting this info for the
> first caller only. How will it help for the debug?

Right, it is possible for the userspace version to change between contexts, but
that's a tradeoff we're willing to take for the meantime so we don't have to
send a host info command for each ucontext allocation.

We can change that in the future if that won't prove to be reliable enough.

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 05/01/2021 12:43, Gal Pressman wrote:
> The following two patches add the userspace version to the host
> information struct reported to the device, used for debugging and
> troubleshooting purposes.
> 
> PR was sent:
> https://github.com/linux-rdma/rdma-core/pull/918
> 
> Thanks,
> Gal

Anything stopping this series from being merged?

Re: [PATCH for-next 0/2] Host information userspace version

[Leon Romanovsky]

On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> On 05/01/2021 12:43, Gal Pressman wrote:
> > The following two patches add the userspace version to the host
> > information struct reported to the device, used for debugging and
> > troubleshooting purposes.
> >
> > PR was sent:
> > https://github.com/linux-rdma/rdma-core/pull/918
> >
> > Thanks,
> > Gal
>
> Anything stopping this series from being merged?

It is unclear when this forwarding of non-verbs data to the FW will stop.

Thanks

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 19/01/2021 10:46, Leon Romanovsky wrote:
> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
>> On 05/01/2021 12:43, Gal Pressman wrote:
>>> The following two patches add the userspace version to the host
>>> information struct reported to the device, used for debugging and
>>> troubleshooting purposes.
>>>
>>> PR was sent:
>>> https://github.com/linux-rdma/rdma-core/pull/918
>>>
>>> Thanks,
>>> Gal
>>
>> Anything stopping this series from being merged?
> 
> It is unclear when this forwarding of non-verbs data to the FW will stop.

This was already discussed in the PR. Not everything should be passed through
this interface, there should be a limit and it should be examined per case.
rdma-core version is clearly related to an RDMA device.

BTW, if you have any concerns about a patch you can state them, you don't have
to ignore it and wait for the submitter to ask what's wrong..

Re: [PATCH for-next 0/2] Host information userspace version

[Leon Romanovsky]

On Tue, Jan 19, 2021 at 11:10:59AM +0200, Gal Pressman wrote:
> On 19/01/2021 10:46, Leon Romanovsky wrote:
> > On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> >> On 05/01/2021 12:43, Gal Pressman wrote:
> >>> The following two patches add the userspace version to the host
> >>> information struct reported to the device, used for debugging and
> >>> troubleshooting purposes.
> >>>
> >>> PR was sent:
> >>> https://github.com/linux-rdma/rdma-core/pull/918
> >>>
> >>> Thanks,
> >>> Gal
> >>
> >> Anything stopping this series from being merged?
> >
> > It is unclear when this forwarding of non-verbs data to the FW will stop.
>
> This was already discussed in the PR. Not everything should be passed through
> this interface, there should be a limit and it should be examined per case.
> rdma-core version is clearly related to an RDMA device.

"Clearly or not" - it depends on the observer.

>
> BTW, if you have any concerns about a patch you can state them, you don't have
> to ignore it and wait for the submitter to ask what's wrong..

Didn't you mistake me with anyone else?

I'm reviewer in the kernel exactly like you and it gives me nice thing - ignore patches.

Thanks

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 19/01/2021 13:58, Leon Romanovsky wrote:
> On Tue, Jan 19, 2021 at 11:10:59AM +0200, Gal Pressman wrote:
>> On 19/01/2021 10:46, Leon Romanovsky wrote:
>>> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
>>>> On 05/01/2021 12:43, Gal Pressman wrote:
>>>>> The following two patches add the userspace version to the host
>>>>> information struct reported to the device, used for debugging and
>>>>> troubleshooting purposes.
>>>>>
>>>>> PR was sent:
>>>>> https://github.com/linux-rdma/rdma-core/pull/918
>>>>>
>>>>> Thanks,
>>>>> Gal
>>>>
>>>> Anything stopping this series from being merged?
>>>
>>> It is unclear when this forwarding of non-verbs data to the FW will stop.
>>
>> This was already discussed in the PR. Not everything should be passed through
>> this interface, there should be a limit and it should be examined per case.
>> rdma-core version is clearly related to an RDMA device.
> 
> "Clearly or not" - it depends on the observer.
> 
>>
>> BTW, if you have any concerns about a patch you can state them, you don't have
>> to ignore it and wait for the submitter to ask what's wrong..
> 
> Didn't you mistake me with anyone else?

No, you decided to answer my original question :).

> I'm reviewer in the kernel exactly like you and it gives me nice thing - ignore patches.

Don't get me wrong, your review is very appreciated, but this series is 20 LOC
which you already reviewed two weeks ago, and I replied to all comments.
Ignoring patches is fine, but please don't review, ignore and wait for the last
minute to say they shouldn't be merged.

Re: [PATCH for-next 0/2] Host information userspace version

[Leon Romanovsky]

On Tue, Jan 19, 2021 at 03:19:15PM +0200, Gal Pressman wrote:
> On 19/01/2021 13:58, Leon Romanovsky wrote:
> > On Tue, Jan 19, 2021 at 11:10:59AM +0200, Gal Pressman wrote:
> >> On 19/01/2021 10:46, Leon Romanovsky wrote:
> >>> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> >>>> On 05/01/2021 12:43, Gal Pressman wrote:
> >>>>> The following two patches add the userspace version to the host
> >>>>> information struct reported to the device, used for debugging and
> >>>>> troubleshooting purposes.
> >>>>>
> >>>>> PR was sent:
> >>>>> https://github.com/linux-rdma/rdma-core/pull/918
> >>>>>
> >>>>> Thanks,
> >>>>> Gal
> >>>>
> >>>> Anything stopping this series from being merged?
> >>>
> >>> It is unclear when this forwarding of non-verbs data to the FW will stop.
> >>
> >> This was already discussed in the PR. Not everything should be passed through
> >> this interface, there should be a limit and it should be examined per case.
> >> rdma-core version is clearly related to an RDMA device.
> >
> > "Clearly or not" - it depends on the observer.
> >
> >>
> >> BTW, if you have any concerns about a patch you can state them, you don't have
> >> to ignore it and wait for the submitter to ask what's wrong..
> >
> > Didn't you mistake me with anyone else?
>
> No, you decided to answer my original question :).
>
> > I'm reviewer in the kernel exactly like you and it gives me nice thing - ignore patches.
>
> Don't get me wrong, your review is very appreciated, but this series is 20 LOC
> which you already reviewed two weeks ago, and I replied to all comments.
> Ignoring patches is fine, but please don't review, ignore and wait for the last
> minute to say they shouldn't be merged.

Sorry, but it is impossible to get it right.

I gave you hint WHY it takes so long, it is not review/ack/nack or
anything like this.

Thanks

Re: [PATCH for-next 0/2] Host information userspace version

[Jason Gunthorpe]

On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> On 05/01/2021 12:43, Gal Pressman wrote:
> > The following two patches add the userspace version to the host
> > information struct reported to the device, used for debugging and
> > troubleshooting purposes.
> > 
> > PR was sent:
> > https://github.com/linux-rdma/rdma-core/pull/918
> > 
> > Thanks,
> > Gal
> 
> Anything stopping this series from being merged?

Honestly, I'm not very keen on this

Why does this have to go through a kernel driver, can't you collect
OS telemetry some other way?

Jason

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 21/01/2021 20:35, Jason Gunthorpe wrote:
> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
>> On 05/01/2021 12:43, Gal Pressman wrote:
>>> The following two patches add the userspace version to the host
>>> information struct reported to the device, used for debugging and
>>> troubleshooting purposes.
>>>
>>> PR was sent:
>>> https://github.com/linux-rdma/rdma-core/pull/918
>>>
>>> Thanks,
>>> Gal
>>
>> Anything stopping this series from being merged?
> 
> Honestly, I'm not very keen on this
> 
> Why does this have to go through a kernel driver, can't you collect
> OS telemetry some other way?

Hmm, it has to go through rdma-core somehow, what sort of component can
rdma-core interact with to pass such data? The only one I could think of is the
RDMA driver :).

As I said, I get your concern, I was going on and off about this as well, but
the userspace version is a very useful piece of information in the context of a
kernel bypass device. It's just as important as the kernel version.
I agree that this is not the place to pass things like gcc version, but I don't
think that's the case here :).

Do you absolutely hate the idea of passing the userspace version, or are you
worried about what's next to come?
If it's the latter, we don't really have plans to push anything similar anytime
soon, and even if we did, I don't think it should block this series.

Re: [PATCH for-next 0/2] Host information userspace version

[Jason Gunthorpe]

On Thu, Jan 21, 2021 at 09:40:49PM +0200, Gal Pressman wrote:
> On 21/01/2021 20:35, Jason Gunthorpe wrote:
> > On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> >> On 05/01/2021 12:43, Gal Pressman wrote:
> >>> The following two patches add the userspace version to the host
> >>> information struct reported to the device, used for debugging and
> >>> troubleshooting purposes.
> >>>
> >>> PR was sent:
> >>> https://github.com/linux-rdma/rdma-core/pull/918
> >>>
> >>> Thanks,
> >>> Gal
> >>
> >> Anything stopping this series from being merged?
> > 
> > Honestly, I'm not very keen on this
> > 
> > Why does this have to go through a kernel driver, can't you collect
> > OS telemetry some other way?
> 
> Hmm, it has to go through rdma-core somehow, what sort of component can
> rdma-core interact with to pass such data? The only one I could think of is the
> RDMA driver :).
> 
> As I said, I get your concern, I was going on and off about this as well, but
> the userspace version is a very useful piece of information in the context of a
> kernel bypass device. It's just as important as the kernel version.
> I agree that this is not the place to pass things like gcc version, but I don't
> think that's the case here :).

Well, if we were to do this for mlx5 we'd want to pass UCX and maybe
other stuff, it seems like it gets quickly out of hand.

I think telemetry is better done as some telemetry subsystem, not
integrated all over the place

Jason

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 27/01/2021 18:57, Jason Gunthorpe wrote:
> On Thu, Jan 21, 2021 at 09:40:49PM +0200, Gal Pressman wrote:
>> On 21/01/2021 20:35, Jason Gunthorpe wrote:
>>> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
>>>> On 05/01/2021 12:43, Gal Pressman wrote:
>>>>> The following two patches add the userspace version to the host
>>>>> information struct reported to the device, used for debugging and
>>>>> troubleshooting purposes.
>>>>>
>>>>> PR was sent:
>>>>> https://github.com/linux-rdma/rdma-core/pull/918
>>>>>
>>>>> Thanks,
>>>>> Gal
>>>>
>>>> Anything stopping this series from being merged?
>>>
>>> Honestly, I'm not very keen on this
>>>
>>> Why does this have to go through a kernel driver, can't you collect
>>> OS telemetry some other way?
>>
>> Hmm, it has to go through rdma-core somehow, what sort of component can
>> rdma-core interact with to pass such data? The only one I could think of is the
>> RDMA driver :).
>>
>> As I said, I get your concern, I was going on and off about this as well, but
>> the userspace version is a very useful piece of information in the context of a
>> kernel bypass device. It's just as important as the kernel version.
>> I agree that this is not the place to pass things like gcc version, but I don't
>> think that's the case here :).
> 
> Well, if we were to do this for mlx5 we'd want to pass UCX and maybe
> other stuff, it seems like it gets quickly out of hand.

Agree, that's why I think this should be limited to things in rdma-core's reach,
sounds like a reasonable limit to me.

> I think telemetry is better done as some telemetry subsystem, not
> integrated all over the place

Interesting, but that would still be all over the place as each package would
have to report its version to that telemetry driver.

And since this currently doesn't exist, should we stay without a solution?
Specifically talking about rdma-core version, do you think it could be merged?

Re: [PATCH for-next 0/2] Host information userspace version

[Gal Pressman]

On 27/01/2021 19:53, Gal Pressman wrote:
> On 27/01/2021 18:57, Jason Gunthorpe wrote:
>> On Thu, Jan 21, 2021 at 09:40:49PM +0200, Gal Pressman wrote:
>>> On 21/01/2021 20:35, Jason Gunthorpe wrote:
>>>> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
>>>>> On 05/01/2021 12:43, Gal Pressman wrote:
>>>>>> The following two patches add the userspace version to the host
>>>>>> information struct reported to the device, used for debugging and
>>>>>> troubleshooting purposes.
>>>>>>
>>>>>> PR was sent:
>>>>>> https://github.com/linux-rdma/rdma-core/pull/918
>>>>>>
>>>>>> Thanks,
>>>>>> Gal
>>>>>
>>>>> Anything stopping this series from being merged?
>>>>
>>>> Honestly, I'm not very keen on this
>>>>
>>>> Why does this have to go through a kernel driver, can't you collect
>>>> OS telemetry some other way?
>>>
>>> Hmm, it has to go through rdma-core somehow, what sort of component can
>>> rdma-core interact with to pass such data? The only one I could think of is the
>>> RDMA driver :).
>>>
>>> As I said, I get your concern, I was going on and off about this as well, but
>>> the userspace version is a very useful piece of information in the context of a
>>> kernel bypass device. It's just as important as the kernel version.
>>> I agree that this is not the place to pass things like gcc version, but I don't
>>> think that's the case here :).
>>
>> Well, if we were to do this for mlx5 we'd want to pass UCX and maybe
>> other stuff, it seems like it gets quickly out of hand.
> 
> Agree, that's why I think this should be limited to things in rdma-core's reach,
> sounds like a reasonable limit to me.
> 
>> I think telemetry is better done as some telemetry subsystem, not
>> integrated all over the place
> 
> Interesting, but that would still be all over the place as each package would
> have to report its version to that telemetry driver.
> 
> And since this currently doesn't exist, should we stay without a solution?
> Specifically talking about rdma-core version, do you think it could be merged?
> 

Jason?

Re: [PATCH for-next 0/2] Host information userspace version

[Jason Gunthorpe]

On Sun, Feb 28, 2021 at 11:56:01AM +0200, Gal Pressman wrote:
> On 27/01/2021 19:53, Gal Pressman wrote:
> > On 27/01/2021 18:57, Jason Gunthorpe wrote:
> >> On Thu, Jan 21, 2021 at 09:40:49PM +0200, Gal Pressman wrote:
> >>> On 21/01/2021 20:35, Jason Gunthorpe wrote:
> >>>> On Tue, Jan 19, 2021 at 09:17:14AM +0200, Gal Pressman wrote:
> >>>>> On 05/01/2021 12:43, Gal Pressman wrote:
> >>>>>> The following two patches add the userspace version to the host
> >>>>>> information struct reported to the device, used for debugging and
> >>>>>> troubleshooting purposes.
> >>>>>>
> >>>>>> PR was sent:
> >>>>>> https://github.com/linux-rdma/rdma-core/pull/918
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Gal
> >>>>>
> >>>>> Anything stopping this series from being merged?
> >>>>
> >>>> Honestly, I'm not very keen on this
> >>>>
> >>>> Why does this have to go through a kernel driver, can't you collect
> >>>> OS telemetry some other way?
> >>>
> >>> Hmm, it has to go through rdma-core somehow, what sort of component can
> >>> rdma-core interact with to pass such data? The only one I could think of is the
> >>> RDMA driver :).
> >>>
> >>> As I said, I get your concern, I was going on and off about this as well, but
> >>> the userspace version is a very useful piece of information in the context of a
> >>> kernel bypass device. It's just as important as the kernel version.
> >>> I agree that this is not the place to pass things like gcc version, but I don't
> >>> think that's the case here :).
> >>
> >> Well, if we were to do this for mlx5 we'd want to pass UCX and maybe
> >> other stuff, it seems like it gets quickly out of hand.
> > 
> > Agree, that's why I think this should be limited to things in rdma-core's reach,
> > sounds like a reasonable limit to me.
> > 
> >> I think telemetry is better done as some telemetry subsystem, not
> >> integrated all over the place
> > 
> > Interesting, but that would still be all over the place as each package would
> > have to report its version to that telemetry driver.
> > 
> > And since this currently doesn't exist, should we stay without a solution?
> > Specifically talking about rdma-core version, do you think it could be merged?
> > 
> 
> Jason?

I'm not keen on it, it doesn't work well for other use-cases, it seems
too hacky.

Jason