Re: [rdma-next v3 02/24] IB/uverbs: Introduce and use helper functions to copy ah attributes

On Mon, Aug 28, 2017 at 08:20:26PM +0300, Leon Romanovsky wrote:

On Mon, Aug 28, 2017 at 12:32:04PM -0400, Doug Ledford wrote:

quoted

On Mon, 2017-08-28 at 18:55 +0300, Leon Romanovsky wrote:

quoted

On Mon, Aug 28, 2017 at 11:15:14AM -0400, Doug Ledford wrote:

quoted

On Sun, 2017-08-27 at 14:10 +0300, Leon Romanovsky wrote:

quoted

On Thu, Aug 24, 2017 at 04:40:42PM -0400, Doug Ledford wrote:

quoted

On Thu, 2017-08-17 at 15:50 +0300, Leon Romanovsky wrote:

quoted

From: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

This patch introduces two helper functions to copy ah
attributes
from uverbs to internal ib_ah_attr structure and the other
way
during modify qp and query qp respectively.

Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <redacted>
Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

quoted

---
 drivers/infiniband/core/uverbs_cmd.c | 124 ++++++++++++++---
----
----
----------
 1 file changed, 49 insertions(+), 75 deletions(-)

I had to fix this patch up considerably...

quoted

diff --git a/drivers/infiniband/core/uverbs_cmd.c

b/drivers/infiniband/core/uverbs_cmd.c
index 670176b670a0..515425a50059 100644

--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c

@@ -1936,6 +1926,28 @@ static int modify_qp_mask(enum

ib_qp_type
qp_type, int mask)
 	}
 }

+static void copy_ah_attr_from_uverbs(struct ib_device *dev,

                                             ^^^^^^^^^^^^^^^^^^^^
From your patch...

quoted

quoted

quoted

+				     struct rdma_ah_attr
*rdma_attr,
+				     struct
ib_uverbs_qp_dest

And we had a fix here where we only copy the ah attributes if
the
user
set the flag for it, so what I ended up with looks like this:

        if (cmd->base.attr_mask & IB_QP_AV)
                copy_ah_attr_from_uverbs(qp->device, &attr-

quoted

ah_attr,

                                         &cmd->base.dest);

        if (cmd->base.attr_mask & IB_QP_ALT_PATH)
                copy_ah_attr_from_uverbs(qp->device, &attr-

quoted

alt_ah_attr,

                                         &cmd->base.alt_dest);

Doug,

It should be "qp" and not "qp->device" in
copy_ah_attr_from_uverbs().

1998         if (cmd->base.attr_mask & IB_QP_AV)
1999                 copy_ah_attr_from_uverbs(qp, &attr->ah_attr,
&cmd->base.dest);
2000         if (cmd->base.attr_mask & IB_QP_ALT_PATH)
2001                 copy_ah_attr_from_uverbs(qp, &attr-

quoted

alt_ah_attr,

2002                                          &cmd-

quoted

base.alt_dest);

2003         ret = ib_modify_qp_with_udata(qp, attr,
2004                                       modify_qp_mask(qp-

quoted

qp_type,

2005                                                      cmd-

quoted

base.attr_mask),

2006                                       udata);

I'm pretty sure you need to re-read your own patch Leon ;-).  The
helper you added uses struct ib_device, so a pointer to the qp
would
not work.  And it wouldn't have even compiled if I had that wrong.

OK, I reread it and it helped me to find the differences.

In rdma-rc, there is the patch from Noa "IB/core: Avoid accessing
non-allocated memory when inferring port type", which plays exactly
at
the same place as this one.

Because, I'm creating my rdma-next for testing, I'm merging -rcX with
-next and my merge conflict resolution was to move
+               attr->ah_attr.type = rdma_ah_find_type(qp->device,
+                                                      cmd-

quoted

base.dest.port_num);

to the copy_ah_attr_from_uverbs and to change function signature to
be
"static void copy_ah_attr_from_uverbs(struct ib_qp *qp,"

It turns out this is kind of a moot point.  In uverbs_marshall.c there
already exists void ib_copy_ah_attr_to_user().  We should have used
that instead of creating new versions of this in this file.  So, since
I've already taken this patch and pushed it to k.o, please do a new
incremental patch that:

1) Merges the differences between the static versions of this routine
and the one in uverbs_marshall.c into the one in uverbs_marshall.c
2) Adds a copy from uverbs in uverbs_marshall.c that is coded in the
same style as the one already there
3) Converts uverbs_cmd.c routines to use the helpers in
uverbs_marshall.c and removes the duplicate helpers in uverbs_cmd.c.

And, if at all possible, I need it quick because I'm making my pull
request to Linus for the next merge window tomorrow.

On which branch do you want me to do it?
The k.o/for-next doesn't have if(..) copy_.. parts.

Ahh, I found it in k.o/for-next.

Thanks

quoted

--
Doug Ledford [off-list ref]
    GPG KeyID: B826A3330E572FDD
    Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD