shrink struct ib_send_wr V3

shrink struct ib_send_wr V3

[Christoph Hellwig]

This series shrinks the WR size by splitting out the different WR
types.

Patch number two is too large for the mailinglist, so if you didn't
get it grab it here:

	http://git.infradead.org/users/hch/rdma.git/commitdiff/2b63f958de7bd630aba85caf65986831d4372869

or the full git tree at:

	git://git.infradead.org/users/hch/rdma.git wr-cleanup

Changes since V2:

 - fixed patch one to accept SEND - note that this was alredy fixed by
   patch 2
 - added a CC: stable for patch 1
 - added additional review tags

Changes since the version sent around a couple of times:

 - new patch 1 which rejects invalid opcodes.  Patch 2 was doing
   this implicitly except for UD QPs, but I think we should a)
   do this explicitly and b) ensure this goes into 4.2 and -stable
   as I can see quite a lot of harm from submitting such malformed
   operations
 - patch 2 now covers all drivers including those in staging to
   side step any sort of discussions on the staging tree.
 - patch 2 now explicitly replaces the weird overloading in the mlx5
   driver with an explicit embedding of struct ib_send_wr, similar
   to what we do for all other MRs.
 - new patch to drop another unused send_wr field.

--
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

[PATCH 1/3] IB/uverbs: reject invalid or unknown opcodes

[Christoph Hellwig]

We have many WR opcodes that are only supported in kernel space
and/or require optional information to be copied into the WR
structure.  Reject all those not explicitly handled so that we
can't pass invalid information to drivers.

Cc: stable@vger.kernel.org
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Reviewed-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index a15318a..be4cb9f 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2372,6 +2372,12 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 		next->send_flags = user_wr->send_flags;
 
 		if (is_ud) {
+			if (next->opcode != IB_WR_SEND &&
+			    next->opcode != IB_WR_SEND_WITH_IMM) {
+				ret = -EINVAL;
+				goto out_put;
+			}
+
 			next->wr.ud.ah = idr_read_ah(user_wr->wr.ud.ah,
 						     file->ucontext);
 			if (!next->wr.ud.ah) {
@@ -2411,9 +2417,11 @@ ssize_t ib_uverbs_post_send(struct ib_uverbs_file *file,
 					user_wr->wr.atomic.compare_add;
 				next->wr.atomic.swap = user_wr->wr.atomic.swap;
 				next->wr.atomic.rkey = user_wr->wr.atomic.rkey;
+			case IB_WR_SEND:
 				break;
 			default:
-				break;
+				ret = -EINVAL;
+				goto out_put;
 			}
 		}
 
-- 
1.9.1

[PATCH 3/3] IB: remove xrc_remote_srq_num from struct ib_send_wr

[Christoph Hellwig]

The field is only initialized in mlx, but never used.

If we want to add proper XRC support it should be done with a new
struct ib_xrc_wr.

This shrinks the various WR structures by another 4 bytes.

Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
 drivers/infiniband/hw/mlx5/qp.c | 1 -
 include/rdma/ib_verbs.h         | 1 -
 2 files changed, 2 deletions(-)

diff --git a/drivers/infiniband/hw/mlx5/qp.c b/drivers/infiniband/hw/mlx5/qp.c
index 04df156..83a290f 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -2634,7 +2634,6 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct ib_send_wr *wr,
 		switch (ibqp->qp_type) {
 		case IB_QPT_XRC_INI:
 			xrc = seg;
-			xrc->xrc_srqn = htonl(wr->xrc_remote_srq_num);
 			seg += sizeof(*xrc);
 			size += sizeof(*xrc) / 16;
 			/* fall through */
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 9b29c78..b855189 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1100,7 +1100,6 @@ struct ib_send_wr {
 		__be32		imm_data;
 		u32		invalidate_rkey;
 	} ex;
-	u32			xrc_remote_srq_num;	/* XRC TGT QPs only */
 };
 
 struct ib_rdma_wr {
-- 
1.9.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: shrink struct ib_send_wr V3

[Sagi Grimberg]

>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>     driver with an explicit embedding of struct ib_send_wr, similar
>     to what we do for all other MRs.

That's nice,

There is one non-trivial spot that was missed in mlx5_ib_post_send
though:

diff --git a/drivers/infiniband/hw/mlx5/qp.c 
b/drivers/infiniband/hw/mlx5/qp.c
index 7ddfb74..35a18d6 100644
--- a/drivers/infiniband/hw/mlx5/qp.c
+++ b/drivers/infiniband/hw/mlx5/qp.c
@@ -2802,7 +2802,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct 
ib_send_wr *wr,
                                 goto out;
                         }
                         qp->sq.wr_data[idx] = MLX5_IB_WR_UMR;
-                       ctrl->imm = cpu_to_be32(fast_reg_wr(wr)->rkey);
+                       ctrl->imm = cpu_to_be32(umr_wr(wr)->mkey);
                         set_reg_umr_segment(seg, wr);
                         seg += sizeof(struct mlx5_wqe_umr_ctrl_seg);
                         size += sizeof(struct mlx5_wqe_umr_ctrl_seg) / 16;

Care to fold this in?

Cheers,
Sagi.
--
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: shrink struct ib_send_wr V3

[Sagi Grimberg]

On 8/30/2015 6:31 PM, Sagi Grimberg wrote:
>>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>>     driver with an explicit embedding of struct ib_send_wr, similar
>>     to what we do for all other MRs.
>
> That's nice,
>
> There is one non-trivial spot that was missed in mlx5_ib_post_send
> though:
>
> diff --git a/drivers/infiniband/hw/mlx5/qp.c
> b/drivers/infiniband/hw/mlx5/qp.c
> index 7ddfb74..35a18d6 100644
> --- a/drivers/infiniband/hw/mlx5/qp.c
> +++ b/drivers/infiniband/hw/mlx5/qp.c
> @@ -2802,7 +2802,7 @@ int mlx5_ib_post_send(struct ib_qp *ibqp, struct
> ib_send_wr *wr,
>                                  goto out;
>                          }
>                          qp->sq.wr_data[idx] = MLX5_IB_WR_UMR;
> -                       ctrl->imm = cpu_to_be32(fast_reg_wr(wr)->rkey);
> +                       ctrl->imm = cpu_to_be32(umr_wr(wr)->mkey);
>                          set_reg_umr_segment(seg, wr);
>                          seg += sizeof(struct mlx5_wqe_umr_ctrl_seg);
>                          size += sizeof(struct mlx5_wqe_umr_ctrl_seg) / 16;
>
> Care to fold this in?
>

There is another problem I'm seeing in this patch. in reg_umr the
local variable is a type ib_send_wr which is passed to
prep_umr_reg_wqe() which casts it to mlx5_umr_wr (container_of). It
should have probably been mlx5_umr_wr to begin with...

I think this needs to be folded in as well:
diff --git a/drivers/infiniband/hw/mlx5/mr.c 
b/drivers/infiniband/hw/mlx5/mr.c
index 9fac2c1..6f8f3d8 100644
--- a/drivers/infiniband/hw/mlx5/mr.c
+++ b/drivers/infiniband/hw/mlx5/mr.c
@@ -752,7 +752,8 @@ static struct mlx5_ib_mr *reg_umr(struct ib_pd *pd, 
struct ib_umem *umem,
         struct device *ddev = dev->ib_dev.dma_device;
         struct umr_common *umrc = &dev->umrc;
         struct mlx5_ib_umr_context umr_context;
-       struct ib_send_wr wr, *bad;
+       struct mlx5_umr_wr umrwr;
+       struct ib_send_wr *bad;
         struct mlx5_ib_mr *mr;
         struct ib_sge sg;
         int size;
@@ -798,14 +799,14 @@ static struct mlx5_ib_mr *reg_umr(struct ib_pd 
*pd, struct ib_umem *umem,
                 goto free_pas;
         }

-       memset(&wr, 0, sizeof(wr));
-       wr.wr_id = (u64)(unsigned long)&umr_context;
-       prep_umr_reg_wqe(pd, &wr, &sg, dma, npages, mr->mmr.key, page_shift,
-                        virt_addr, len, access_flags);
+       memset(&umrwr, 0, sizeof(umrwr));
+       umrwr.wr.wr_id = (u64)(unsigned long)&umr_context;
+       prep_umr_reg_wqe(pd, &umrwr.wr, &sg, dma, npages, mr->mmr.key,
+                        page_shift, virt_addr, len, access_flags);

         mlx5_ib_init_umr_context(&umr_context);
         down(&umrc->sem);
-       err = ib_post_send(umrc->qp, &wr, &bad);
+       err = ib_post_send(umrc->qp, &umrwr.wr, &bad);
         if (err) {
                 mlx5_ib_warn(dev, "post send failed, err %d\n", err);
                 goto unmap_dma;
@@ -1134,16 +1135,17 @@ static int unreg_umr(struct mlx5_ib_dev *dev, 
struct mlx5_ib_mr *mr)
  {
         struct umr_common *umrc = &dev->umrc;
         struct mlx5_ib_umr_context umr_context;
-       struct ib_send_wr wr, *bad;
+       struct mlx5_umr_wr umrwr;
+       struct ib_send_wr *bad;
         int err;

-       memset(&wr, 0, sizeof(wr));
-       wr.wr_id = (u64)(unsigned long)&umr_context;
-       prep_umr_unreg_wqe(dev, &wr, mr->mmr.key);
+       memset(&umrwr, 0, sizeof(umrwr));
+       umrwr.wr.wr_id = (u64)(unsigned long)&umr_context;
+       prep_umr_unreg_wqe(dev, &umrwr.wr, mr->mmr.key);

         mlx5_ib_init_umr_context(&umr_context);
         down(&umrc->sem);
-       err = ib_post_send(umrc->qp, &wr, &bad);
+       err = ib_post_send(umrc->qp, &umrwr.wr, &bad);
         if (err) {
                 up(&umrc->sem);
                 mlx5_ib_dbg(dev, "err %d\n", err);
--
--
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: shrink struct ib_send_wr V3

[Christoph Hellwig]

On Sun, Aug 30, 2015 at 06:31:35PM +0300, Sagi Grimberg wrote:
>>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>>     driver with an explicit embedding of struct ib_send_wr, similar
>>     to what we do for all other MRs.
>
> That's nice,
>
> There is one non-trivial spot that was missed in mlx5_ib_post_send
> though:

Oh, that was a weird abuse of the old casts.

I've foled both your fixes and force pushed to the wr-cleanup branch.

I do not plan to resend the series until the merge window for 4.4
is open.  Doug, any chance you could pick up the first patch in the
series for 4.3-rc?  It's marked for stable as well.
--
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: shrink struct ib_send_wr V3

[Doug Ledford]

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

On 08/31/2015 12:11 PM, Christoph Hellwig wrote:
> On Sun, Aug 30, 2015 at 06:31:35PM +0300, Sagi Grimberg wrote:
>>>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>>>     driver with an explicit embedding of struct ib_send_wr, similar
>>>     to what we do for all other MRs.
>>
>> That's nice,
>>
>> There is one non-trivial spot that was missed in mlx5_ib_post_send
>> though:
> 
> Oh, that was a weird abuse of the old casts.
> 
> I've foled both your fixes and force pushed to the wr-cleanup branch.
> 
> I do not plan to resend the series until the merge window for 4.4
> is open.  Doug, any chance you could pick up the first patch in the
> series for 4.3-rc?  It's marked for stable as well.

Yes, I can do that.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: shrink struct ib_send_wr V3

[Doug Ledford]

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

On 08/31/2015 08:24 PM, Doug Ledford wrote:
> On 08/31/2015 12:11 PM, Christoph Hellwig wrote:
>> On Sun, Aug 30, 2015 at 06:31:35PM +0300, Sagi Grimberg wrote:
>>>>   - patch 2 now explicitly replaces the weird overloading in the mlx5
>>>>     driver with an explicit embedding of struct ib_send_wr, similar
>>>>     to what we do for all other MRs.
>>>
>>> That's nice,
>>>
>>> There is one non-trivial spot that was missed in mlx5_ib_post_send
>>> though:
>>
>> Oh, that was a weird abuse of the old casts.
>>
>> I've foled both your fixes and force pushed to the wr-cleanup branch.
>>
>> I do not plan to resend the series until the merge window for 4.4
>> is open.  Doug, any chance you could pick up the first patch in the
>> series for 4.3-rc?  It's marked for stable as well.
> 
> Yes, I can do that.
> 

I've applied 1 of 3.  For the remaining two, I plan to pull them into a
test kernel and run some internal tests before committing.  Just FYI.

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]