[PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Yishai Hadas]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The returned UAR pointer is actually void ** and points to whole
UAR database.

Because user is not supposed to access it and expected to use
the CQ doorbell, we will return uar[0] (CQ doorbell) directly
and eliminate the following logic in the applications:

uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
                     MLX5_CQ_DOORBELL;

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Yishai Hadas <yishaih-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---

Pull request was sent:
https://github.com/linux-rdma/rdma-core/pull/184

 providers/mlx5/mlx5.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index ede91cb..c7a6efb 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -678,7 +678,7 @@ static int mlx5dv_get_cq(struct ibv_cq *cq_in,
 	cq_out->cqe_size  = mcq->cqe_sz;
 	cq_out->buf       = mcq->active_buf->buf;
 	cq_out->dbrec     = mcq->dbrec;
-	cq_out->uar	  = mctx->uar;
+	cq_out->uar	  = mctx->uar[0];
 
 	mcq->flags	 |= MLX5_CQ_FLAGS_DV_OWNED;
 
-- 
1.8.3.1

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The returned UAR pointer is actually void ** and points to whole
> UAR database.
> 
> Because user is not supposed to access it and expected to use
> the CQ doorbell, we will return uar[0] (CQ doorbell) directly
> and eliminate the following logic in the applications:
> 
> uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
>                      MLX5_CQ_DOORBELL;

NAK, at least, as is..

This changes the ABI and mlx5dv is a public interface now, so you have
to do it properly.

Looking at it, my advice is to rev the symbol version of
mlx5dv_init_obj and consider providing a compat symbol.

Also, you need to change the name of the 'uar' field:

> -	cq_out->uar	  = mctx->uar;
> +	cq_out->uar	  = mctx->uar[0];

To something else, to make it clear, which of the two ABIs the
application code is coded to - for a change like this, you *MUST*
force old apps to have a compile failure.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]

On Thu, Aug 17, 2017 at 11:33:20AM -0600, Jason Gunthorpe wrote:
> On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > The returned UAR pointer is actually void ** and points to whole
> > UAR database.
> >
> > Because user is not supposed to access it and expected to use
> > the CQ doorbell, we will return uar[0] (CQ doorbell) directly
> > and eliminate the following logic in the applications:
> >
> > uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
> >                      MLX5_CQ_DOORBELL;
>
> NAK, at least, as is..
>
> This changes the ABI and mlx5dv is a public interface now, so you have
> to do it properly.
>
> Looking at it, my advice is to rev the symbol version of
> mlx5dv_init_obj and consider providing a compat symbol.
>
> Also, you need to change the name of the 'uar' field:
>
> > -	cq_out->uar	  = mctx->uar;
> > +	cq_out->uar	  = mctx->uar[0];
>
> To something else, to make it clear, which of the two ABIs the
> application code is coded to - for a change like this, you *MUST*
> force old apps to have a compile failure.

I know exactly who coded on top of this interface. They are the ones who
asked for this change and they are fully understand the implications,
inability to work with this interface on "old" libraries.

Thanks

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Thu, Aug 17, 2017 at 08:39:32PM +0300, Leon Romanovsky wrote:
> On Thu, Aug 17, 2017 at 11:33:20AM -0600, Jason Gunthorpe wrote:
> > On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
> > > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >
> > > The returned UAR pointer is actually void ** and points to whole
> > > UAR database.
> > >
> > > Because user is not supposed to access it and expected to use
> > > the CQ doorbell, we will return uar[0] (CQ doorbell) directly
> > > and eliminate the following logic in the applications:
> > >
> > > uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
> > >                      MLX5_CQ_DOORBELL;
> >
> > NAK, at least, as is..
> >
> > This changes the ABI and mlx5dv is a public interface now, so you have
> > to do it properly.
> >
> > Looking at it, my advice is to rev the symbol version of
> > mlx5dv_init_obj and consider providing a compat symbol.
> >
> > Also, you need to change the name of the 'uar' field:
> >
> > > -	cq_out->uar	  = mctx->uar;
> > > +	cq_out->uar	  = mctx->uar[0];
> >
> > To something else, to make it clear, which of the two ABIs the
> > application code is coded to - for a change like this, you *MUST*
> > force old apps to have a compile failure.
> 
> I know exactly who coded on top of this interface. They are the ones who
> asked for this change and they are fully understand the implications,
> inability to work with this interface on "old" libraries.

I don't care who they are - you need to follow sane ABI rules if you
are pushing public shared libraries into distros. This is not negotiable.

You can maybe avoid providing compat, but not the rest of it.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Yishai Hadas]

On 8/17/2017 9:03 PM, Jason Gunthorpe wrote:
> On Thu, Aug 17, 2017 at 08:39:32PM +0300, Leon Romanovsky wrote:
>> On Thu, Aug 17, 2017 at 11:33:20AM -0600, Jason Gunthorpe wrote:
>>> On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
>>>> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>>
>>>> The returned UAR pointer is actually void ** and points to whole
>>>> UAR database.
>>>>
>>>> Because user is not supposed to access it and expected to use
>>>> the CQ doorbell, we will return uar[0] (CQ doorbell) directly
>>>> and eliminate the following logic in the applications:
>>>>
>>>> uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
>>>>                      MLX5_CQ_DOORBELL;
>>>
>>> NAK, at least, as is..
>>>
>>> This changes the ABI and mlx5dv is a public interface now, so you have
>>> to do it properly.
>>>
>>> Looking at it, my advice is to rev the symbol version of
>>> mlx5dv_init_obj and consider providing a compat symbol.
>>>
>>> Also, you need to change the name of the 'uar' field:
>>>
>>>> -	cq_out->uar	  = mctx->uar;
>>>> +	cq_out->uar	  = mctx->uar[0];
>>>
>>> To something else, to make it clear, which of the two ABIs the
>>> application code is coded to - for a change like this, you *MUST*
>>> force old apps to have a compile failure.
>>
>> I know exactly who coded on top of this interface. They are the ones who
>> asked for this change and they are fully understand the implications,
>> inability to work with this interface on "old" libraries.
>
> I don't care who they are - you need to follow sane ABI rules if you
> are pushing public shared libraries into distros. This is not negotiable.
>

There is no ABI/API change but just a bug fix in mlx5 driver.
The API talked about void* but by mistake the mlx5 code returned void**, 
this patch fixes that.

We don't care about potential break for direct mlx5 applications in this 
arm_cq flow as code was buggy and couldn't be used properly.

Patch was merged.




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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Mon, Aug 21, 2017 at 12:06:57PM +0300, Yishai Hadas wrote:
> On 8/17/2017 9:03 PM, Jason Gunthorpe wrote:
> >On Thu, Aug 17, 2017 at 08:39:32PM +0300, Leon Romanovsky wrote:
> >>On Thu, Aug 17, 2017 at 11:33:20AM -0600, Jason Gunthorpe wrote:
> >>>On Thu, Aug 17, 2017 at 04:36:38PM +0300, Yishai Hadas wrote:
> >>>>From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >>>>
> >>>>The returned UAR pointer is actually void ** and points to whole
> >>>>UAR database.
> >>>>
> >>>>Because user is not supposed to access it and expected to use
> >>>>the CQ doorbell, we will return uar[0] (CQ doorbell) directly
> >>>>and eliminate the following logic in the applications:
> >>>>
> >>>>uint64_t cq_db_reg = (uint64_t *)(((uint64_t)(uint64_t *)rxq->cq_uar) +
> >>>>                     MLX5_CQ_DOORBELL;
> >>>
> >>>NAK, at least, as is..
> >>>
> >>>This changes the ABI and mlx5dv is a public interface now, so you have
> >>>to do it properly.
> >>>
> >>>Looking at it, my advice is to rev the symbol version of
> >>>mlx5dv_init_obj and consider providing a compat symbol.
> >>>
> >>>Also, you need to change the name of the 'uar' field:
> >>>
> >>>>-	cq_out->uar	  = mctx->uar;
> >>>>+	cq_out->uar	  = mctx->uar[0];
> >>>
> >>>To something else, to make it clear, which of the two ABIs the
> >>>application code is coded to - for a change like this, you *MUST*
> >>>force old apps to have a compile failure.
> >>
> >>I know exactly who coded on top of this interface. They are the ones who
> >>asked for this change and they are fully understand the implications,
> >>inability to work with this interface on "old" libraries.
> >
> >I don't care who they are - you need to follow sane ABI rules if you
> >are pushing public shared libraries into distros. This is not negotiable.
> >
> 
> There is no ABI/API change but just a bug fix in mlx5 driver.
> The API talked about void* but by mistake the mlx5 code returned void**,
> this patch fixes that.

The commit message is garbage then.

Is there existing code out there that uses cq_out->uar and works
properly today? Yes or No?

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Yishai Hadas]

On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
> Is there existing code out there that uses cq_out->uar and works
> properly today? Yes or No?

No, only this fix enables that to work properly.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Tue, Aug 22, 2017 at 11:36:42AM +0300, Yishai Hadas wrote:
> On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
> >Is there existing code out there that uses cq_out->uar and works
> >properly today? Yes or No?
> 
> No, only this fix enables that to work properly.

This is still bad reasoning.

rdma-core 14 does not support the 'uar' field, it may as well have
been marked reserved.

rdma-core 15 supports 'uar'.

This is an ABI change, and need to be handled properly to protect the
users against mixing incompatible libraries.

The fact that rdma-core 14 intended to support uar but screwed it up
doesn't really matter from the user perspective. All they care about
is that an app using using uar needs to be build against >= 15 or it
silently explodes at runtime.

This sorts of issues are exactly what we are supposed to be using
symbol versions to protect against, and largely why they exist at
all. If you do it properly then tools like RPM and the dynanmic linker
take care of everything for the end user.

You don't get to ignore sane shared library versioning rules just
because it is your company's library - part of the point of rdma-core
is to have a much stronger stance on ABI issues (our community has
done a terrible job over the years) and I do not like this weakening.

Particularly since a patch to do this with proper compatibility
exists, and there is no reason at all to take a shortcut.

Doug? This needs to be decided before you mark rdma-core 15 final.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Yishai Hadas]

On 8/22/2017 7:30 PM, Jason Gunthorpe wrote:
> On Tue, Aug 22, 2017 at 11:36:42AM +0300, Yishai Hadas wrote:
>> On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
>>> Is there existing code out there that uses cq_out->uar and works
>>> properly today? Yes or No?
>>
>> No, only this fix enables that to work properly.

> Particularly since a patch to do this with proper compatibility
> exists, and there is no reason at all to take a shortcut.
>

We are not looking for a shortcut but for a solution that will match 
majority users if not all.

Changing the symbol version for this API will cause that applications 
that will be linked against this version won't be able to run at all on 
previous RH packages (e.g RH 7.3/7.4) even if this flow wasn't used at 
all as we expect. As pointed, from our knowledge there are no customers 
for that specific *rare* flow in mlx5 DV and we prefer to stay with 
current bug fix and same API version.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Wed, Aug 23, 2017 at 03:13:38PM +0300, Yishai Hadas wrote:
> On 8/22/2017 7:30 PM, Jason Gunthorpe wrote:
> >On Tue, Aug 22, 2017 at 11:36:42AM +0300, Yishai Hadas wrote:
> >>On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
> >>>Is there existing code out there that uses cq_out->uar and works
> >>>properly today? Yes or No?
> >>
> >>No, only this fix enables that to work properly.
> 
> >Particularly since a patch to do this with proper compatibility
> >exists, and there is no reason at all to take a shortcut.
> 
> We are not looking for a shortcut but for a solution that will match
> majority users if not all.

It is a short cut.

> Changing the symbol version for this API will cause that applications that
> will be linked against this version won't be able to run at all on
> previous

Then provide the compat symbol too, as I said in the first place, easy
to do.

There is no excuse here.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Wed, Aug 23, 2017 at 03:13:38PM +0300, Yishai Hadas wrote:
> On 8/22/2017 7:30 PM, Jason Gunthorpe wrote:
> >On Tue, Aug 22, 2017 at 11:36:42AM +0300, Yishai Hadas wrote:
> >>On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
> >>>Is there existing code out there that uses cq_out->uar and works
> >>>properly today? Yes or No?
> >>
> >>No, only this fix enables that to work properly.
> 
> >Particularly since a patch to do this with proper compatibility
> >exists, and there is no reason at all to take a shortcut.
> >
> 
> We are not looking for a shortcut but for a solution that will match
> majority users if not all.

Here is the patch that takes care of everything properly. It is PR
188.

Please be more careful with the ABI requirements in the future.

>From 32ef444e45006112ff759106dcdee2b99999501a Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Wed, 23 Aug 2017 10:05:24 -0600
Subject: [PATCH] mlx5: Fix ABI break from revising the UAR pointer

Provide two implementations of mlx5dv_init_obj, one that has the
historical behaviour that has existed until now of returning the
void **uar and a new version that returns the 'void *' version
renamed to arb_db.

Apps that use this feature must refer to it as arb_db, they will not
compile on pre-rdma-core 15 releases, and they will not dynamically
link to old versions either. This provides a sane level of safety for
the end users of this library.

Fixes: c6e3439aaa93 ("mlx5: Return pointer to CQ doorbell")
Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 debian/ibverbs-providers.symbols |  2 ++
 providers/mlx5/CMakeLists.txt    |  2 +-
 providers/mlx5/libmlx5.map       |  5 +++++
 providers/mlx5/mlx5.c            | 22 ++++++++++++++++++++--
 providers/mlx5/mlx5dv.h          |  2 +-
 5 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/debian/ibverbs-providers.symbols b/debian/ibverbs-providers.symbols
index 5aa3b877b55b59..b03c33a2ba9fce 100644
--- a/debian/ibverbs-providers.symbols
+++ b/debian/ibverbs-providers.symbols
@@ -4,6 +4,8 @@ libmlx4.so.1 ibverbs-providers #MINVER#
 libmlx5.so.1 ibverbs-providers #MINVER#
  MLX5_1.0@MLX5_1.0 13
  MLX5_1.1@MLX5_1.1 14
+ MLX5_1.2@MLX5_1.2 15
  mlx5dv_init_obj@MLX5_1.0 13
+ mlx5dv_init_obj@MLX5_1.2 15
  mlx5dv_query_device@MLX5_1.0 13
  mlx5dv_create_cq@MLX5_1.1 14
diff --git a/providers/mlx5/CMakeLists.txt b/providers/mlx5/CMakeLists.txt
index 0fb9907bf258d1..ab6a42d8915a2a 100644
--- a/providers/mlx5/CMakeLists.txt
+++ b/providers/mlx5/CMakeLists.txt
@@ -11,7 +11,7 @@ if (MLX5_MW_DEBUG)
 endif()
 
 rdma_shared_provider(mlx5 libmlx5.map
-  1 1.1.${PACKAGE_VERSION}
+  1 1.2.${PACKAGE_VERSION}
   buf.c
   cq.c
   dbrec.c
diff --git a/providers/mlx5/libmlx5.map b/providers/mlx5/libmlx5.map
index 6d9fb25f00a3db..e7fe9f41697009 100644
--- a/providers/mlx5/libmlx5.map
+++ b/providers/mlx5/libmlx5.map
@@ -11,3 +11,8 @@ MLX5_1.1 {
 	global:
 		mlx5dv_create_cq;
 } MLX5_1.0;
+
+MLX5_1.2 {
+	global:
+		mlx5dv_init_obj;
+} MLX5_1.1;
diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index dd825561ec740f..7ec5951b6ff1f8 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -678,7 +678,7 @@ static int mlx5dv_get_cq(struct ibv_cq *cq_in,
 	cq_out->cqe_size  = mcq->cqe_sz;
 	cq_out->buf       = mcq->active_buf->buf;
 	cq_out->dbrec     = mcq->dbrec;
-	cq_out->uar	  = mctx->uar[0];
+	cq_out->arm_db	  = mctx->uar[0];
 
 	mcq->flags	 |= MLX5_CQ_FLAGS_DV_OWNED;
 
@@ -716,7 +716,8 @@ static int mlx5dv_get_srq(struct ibv_srq *srq_in,
 	return 0;
 }
 
-int mlx5dv_init_obj(struct mlx5dv_obj *obj, uint64_t obj_type)
+int __mlx5dv_init_obj_1_2(struct mlx5dv_obj *obj, uint64_t obj_type);
+int __mlx5dv_init_obj_1_2(struct mlx5dv_obj *obj, uint64_t obj_type)
 {
 	int ret = 0;
 
@@ -731,6 +732,23 @@ int mlx5dv_init_obj(struct mlx5dv_obj *obj, uint64_t obj_type)
 
 	return ret;
 }
+asm(".symver __mlx5dv_init_obj_1_2, mlx5dv_init_obj@@MLX5_1.2");
+
+int __mlx5dv_init_obj_1_0(struct mlx5dv_obj *obj, uint64_t obj_type);
+int __mlx5dv_init_obj_1_0(struct mlx5dv_obj *obj, uint64_t obj_type)
+{
+	int ret = 0;
+
+	ret = __mlx5dv_init_obj_1_2(obj, obj_type);
+	if (!ret && (obj_type & MLX5DV_OBJ_CQ)) {
+		/* ABI version 1.0 returns the void ** in this memory
+		 * location
+		 */
+		obj->cq.out->arm_db = to_mctx(obj->cq.in->context)->uar;
+	}
+	return ret;
+}
+asm(".symver __mlx5dv_init_obj_1_0, mlx5dv_init_obj@MLX5_1.0");
 
 static void adjust_uar_info(struct mlx5_device *mdev,
 			    struct mlx5_context *context,
diff --git a/providers/mlx5/mlx5dv.h b/providers/mlx5/mlx5dv.h
index cff3a10457300f..1a2e257c4bcc6f 100644
--- a/providers/mlx5/mlx5dv.h
+++ b/providers/mlx5/mlx5dv.h
@@ -129,7 +129,7 @@ struct mlx5dv_cq {
 	__be32			*dbrec;
 	uint32_t		cqe_cnt;
 	uint32_t		cqe_size;
-	void			*uar;
+	void			*arm_db;
 	uint32_t		cqn;
 	uint64_t		comp_mask;
 };
-- 
2.7.4


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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2085 bytes --]

On Wed, Aug 23, 2017 at 10:08:16AM -0600, Jason Gunthorpe wrote:
> On Wed, Aug 23, 2017 at 03:13:38PM +0300, Yishai Hadas wrote:
> > On 8/22/2017 7:30 PM, Jason Gunthorpe wrote:
> > >On Tue, Aug 22, 2017 at 11:36:42AM +0300, Yishai Hadas wrote:
> > >>On 8/21/2017 6:22 PM, Jason Gunthorpe wrote:
> > >>>Is there existing code out there that uses cq_out->uar and works
> > >>>properly today? Yes or No?
> > >>
> > >>No, only this fix enables that to work properly.
> >
> > >Particularly since a patch to do this with proper compatibility
> > >exists, and there is no reason at all to take a shortcut.
> > >
> >
> > We are not looking for a shortcut but for a solution that will match
> > majority users if not all.
>
> Here is the patch that takes care of everything properly. It is PR
> 188.
>
> Please be more careful with the ABI requirements in the future.
>
> From 32ef444e45006112ff759106dcdee2b99999501a Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Date: Wed, 23 Aug 2017 10:05:24 -0600
> Subject: [PATCH] mlx5: Fix ABI break from revising the UAR pointer
>
> Provide two implementations of mlx5dv_init_obj, one that has the
> historical behaviour that has existed until now of returning the
> void **uar and a new version that returns the 'void *' version
> renamed to arb_db.

arb_db -> arm_db

>
> Apps that use this feature must refer to it as arb_db, they will not
> compile on pre-rdma-core 15 releases, and they will not dynamically
> link to old versions either. This provides a sane level of safety for
> the end users of this library.
>
> Fixes: c6e3439aaa93 ("mlx5: Return pointer to CQ doorbell")

Strange, in github it has 7 digits (the same was with rsocket fixes)
Fixes: c6e3439 ("mlx5: Return pointer to CQ doorbell")

> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>

The overall looks good, but I need to run checks with before applying it.

Thanks for doing it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Wed, Aug 23, 2017 at 07:45:55PM +0300, Leon Romanovsky wrote:

> > Provide two implementations of mlx5dv_init_obj, one that has the
> > historical behaviour that has existed until now of returning the
> > void **uar and a new version that returns the 'void *' version
> > renamed to arb_db.
> 
> arb_db -> arm_db

Oops, I fixed that, thanks.

> Strange, in github it has 7 digits (the same was with rsocket fixes)
> Fixes: c6e3439 ("mlx5: Return pointer to CQ doorbell")

Yeah, side effect of github rendering it into a web link, I think.

> The overall looks good, but I need to run checks with before
> applying it.

Sure, I didn't test it obviously, but the readelf is what I expect:

   101: 000000000000e4d0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
   168: 000000000000e4d0    55 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_0
   102: 000000000000e300   451 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@@MLX5_1.2
   169: 000000000000e300   451 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_2

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3232 bytes --]

On Wed, Aug 23, 2017 at 10:52:21AM -0600, Jason Gunthorpe wrote:
> On Wed, Aug 23, 2017 at 07:45:55PM +0300, Leon Romanovsky wrote:
>
> > > Provide two implementations of mlx5dv_init_obj, one that has the
> > > historical behaviour that has existed until now of returning the
> > > void **uar and a new version that returns the 'void *' version
> > > renamed to arb_db.
> >
> > arb_db -> arm_db
>
> Oops, I fixed that, thanks.
>
> > Strange, in github it has 7 digits (the same was with rsocket fixes)
> > Fixes: c6e3439 ("mlx5: Return pointer to CQ doorbell")
>
> Yeah, side effect of github rendering it into a web link, I think.
>
> > The overall looks good, but I need to run checks with before
> > applying it.
>
> Sure, I didn't test it obviously, but the readelf is what I expect:
>
>    101: 000000000000e4d0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
>    168: 000000000000e4d0    55 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_0
>    102: 000000000000e300   451 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@@MLX5_1.2
>    169: 000000000000e300   451 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_2

It is not complete output as I would expect to see. It doesn't have
mlx5dv_init_obj (without explicit @MLX5_1.2). There is a need to declare default
function name for the static linking has no knowledge of versions.

The simplest approach which is used in whole rdma-core is just retain
the original function name and the patch below on top of your patch
achieves it.

diff --git a/providers/mlx5/mlx5.c b/providers/mlx5/mlx5.c
index 7ec5951b..49dd4f12 100644
--- a/providers/mlx5/mlx5.c
+++ b/providers/mlx5/mlx5.c
@@ -716,8 +716,7 @@ static int mlx5dv_get_srq(struct ibv_srq *srq_in,
 	return 0;
 }

-int __mlx5dv_init_obj_1_2(struct mlx5dv_obj *obj, uint64_t obj_type);
-int __mlx5dv_init_obj_1_2(struct mlx5dv_obj *obj, uint64_t obj_type)
+int mlx5dv_init_obj(struct mlx5dv_obj *obj, uint64_t obj_type)
 {
 	int ret = 0;

@@ -732,14 +731,13 @@ int __mlx5dv_init_obj_1_2(struct mlx5dv_obj *obj, uint64_t obj_type)

 	return ret;
 }
-asm(".symver __mlx5dv_init_obj_1_2, mlx5dv_init_obj@@MLX5_1.2");

 int __mlx5dv_init_obj_1_0(struct mlx5dv_obj *obj, uint64_t obj_type);
 int __mlx5dv_init_obj_1_0(struct mlx5dv_obj *obj, uint64_t obj_type)
 {
 	int ret = 0;

-	ret = __mlx5dv_init_obj_1_2(obj, obj_type);
+	ret = mlx5dv_init_obj(obj, obj_type);
 	if (!ret && (obj_type & MLX5DV_OBJ_CQ)) {
 		/* ABI version 1.0 returns the void ** in this memory
 		 * location

------

➜  rdma-core git:(tst-uar) ✗ readelf -s build/lib/libmlx5.so |grep -i mlx5dv_init
    99: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
   165: 000000000000dab0    55 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_0
   194: 000000000000d8f0   443 FUNC    LOCAL  DEFAULT   13 mlx5dv_init_obj
   268: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Thu, Aug 24, 2017 at 08:27:16AM +0300, Leon Romanovsky wrote:

> It is not complete output as I would expect to see. It doesn't have
> mlx5dv_init_obj (without explicit @MLX5_1.2). There is a need to declare default
> function name for the static linking has no knowledge of versions.

symver support and static linking in verbs has never worked properly,
we need to do more work overall to fix that..

Some common macros a bit of use of .alias will do the job, but nobody
has cared enough about static linking to do it. We shouldn't worry
about that now...

> The simplest approach which is used in whole rdma-core is just retain
> the original function name and the patch below on top of your patch
> achieves it.

Well, if this is the result you got:

>     99: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
>    165: 000000000000dab0    55 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_0
>    194: 000000000000d8f0   443 FUNC    LOCAL  DEFAULT   13 mlx5dv_init_obj
>    268: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0

Then changing the name totally broke the dynamic symbols.

Which is what I remember when I last tried to work on this (to avoid
having to specify a prototype for the __).

I concluded the linkers broke if there was a symbol in the link with
the same name as the versioned symbol when using _symver.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]

On Thu, Aug 24, 2017 at 09:00:45AM -0600, Jason Gunthorpe wrote:
> On Thu, Aug 24, 2017 at 08:27:16AM +0300, Leon Romanovsky wrote:
>
> > It is not complete output as I would expect to see. It doesn't have
> > mlx5dv_init_obj (without explicit @MLX5_1.2). There is a need to declare default
> > function name for the static linking has no knowledge of versions.
>
> symver support and static linking in verbs has never worked properly,
> we need to do more work overall to fix that..

Fix yes, but push something that works to be broken is a different story.

>
> Some common macros a bit of use of .alias will do the job, but nobody
> has cared enough about static linking to do it. We shouldn't worry
> about that now...

Maybe it is true for libibverbs, but it is not for libmlx4/5 which can
be statically linked.

>
> > The simplest approach which is used in whole rdma-core is just retain
> > the original function name and the patch below on top of your patch
> > achieves it.
>
> Well, if this is the result you got:
>
> >     99: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
> >    165: 000000000000dab0    55 FUNC    LOCAL  DEFAULT   13 __mlx5dv_init_obj_1_0
> >    194: 000000000000d8f0   443 FUNC    LOCAL  DEFAULT   13 mlx5dv_init_obj
> >    268: 000000000000dab0    55 FUNC    GLOBAL DEFAULT   13 mlx5dv_init_obj@MLX5_1.0
>
> Then changing the name totally broke the dynamic symbols.

Yes, I see, mlx5dv_init_obj should be GLOBAL.

>
> Which is what I remember when I last tried to work on this (to avoid
> having to specify a prototype for the __).
>
> I concluded the linkers broke if there was a symbol in the link with
> the same name as the versioned symbol when using _symver.
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Thu, Aug 24, 2017 at 06:50:36PM +0300, Leon Romanovsky wrote:

> > Some common macros a bit of use of .alias will do the job, but nobody
> > has cared enough about static linking to do it. We shouldn't worry
> > about that now...
> 
> Maybe it is true for libibverbs, but it is not for libmlx4/5 which can
> be statically linked.

Wel.. that is dangerous, there is quite a bit of stuff that the user
has to do exactly right to make that work safely. Including
guarenteeing the exact right version of libibverbs.so is present
when doing the compile.

If people are doing static linking then we should do a lot more to try
and make that work properly.

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

Re: [PATCH rdma-core] mlx5: Return pointer to CQ doorbell

[Jason Gunthorpe]

On Thu, Aug 24, 2017 at 06:50:36PM +0300, Leon Romanovsky wrote:

> > symver support and static linking in verbs has never worked properly,
> > we need to do more work overall to fix that..
> 
> Fix yes, but push something that works to be broken is a different story.

I'm not sure it matters to any users, since mlx5.a was broken until I
fixed it two months ago.. https://github.com/linux-rdma/rdma-core/pull/142

But, I fixed all the issues with symbol versioning here:

https://github.com/linux-rdma/rdma-core/pull/191

I revised the mlx5dv fix to use the new macros so static builds will
work for it as well.

So we should be good to go to fix this before rdma core 15

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