[PATCH for-next 00/10] iSER support for remote invalidate

[PATCH for-next 00/10] iSER support for remote invalidate

[Sagi Grimberg]

This patchset adds remote invalidation support to iser initiator and
target. The support negotiation for this feature is based on IBTA
annex 12 "Support for iSCSI Extensions for RDMA" carried in rdma_cm
private data.

Remote invalidation allows a peer host to invalidate a remote key
as part of a SEND operation. This feature allows a host to avoid
invalidating an rkey locally. By supporting this feature iser initiator
can save extra latency and processing time yielded by invalidating 
the memory key locally.

The initiator feature support is dependent on:
- fastreg is used (not FMR)
- always_register=Y

In this case the initiator will expose support for remote invalidation,
however it will not blindly rely on the target to do so and will verify
that in the work completion information. The iser target now looks into
the iser header in the CM request and in case the initiator supports
remote invalidation it will respond it will use remote invalidation for
provided remote keys.

Initial Benchmarks (CIB/CX4) shows a moderate IOPs improvements under
high workloads (7%-10%). CX3 devices should show dramatic increase in
performance as it has strict fencing policies for rkey invalidation.
Would be nice to benchmark this in iWARP too (Steve ;)).

- Patch 1 is just a small fixup piggybacked to this set.
- Patches 2-4 are preping the ground for remote invalidate support.
- Patches 5-6 centralize protocol related stuff to a central header.
- Patches 7-8 adds iser target remote invalidation support.
- Patches 9-10 adds iser initiator remote invalidate support.

Jenny Derzhavetz (5):
  IB/iser: Don't register memory for all immediatedata writes
  IB/iser: set intuitive values for mr_valid
  iser-target: Declare correct flags when accepting a connection
  iser-target: Support the remote invalidation exception
  IB/iser: Support the remote invalidation exception

Roi Dayan (1):
  IB/iser: fix module init not cleaning up on error flow

Sagi Grimberg (4):
  IB/iser: Default to fastreg instead of fmr
  iser: Have initiator and target to share protocol structures and
    definitions
  iser-target: Remove unused file iser_proto.h
  IB/iser: Increment the rkey when registering and not when invalidating

 drivers/infiniband/ulp/iser/iscsi_iser.c     |   9 ++-
 drivers/infiniband/ulp/iser/iscsi_iser.h     |  47 +++---------
 drivers/infiniband/ulp/iser/iser_initiator.c |  66 ++++++++++++++--
 drivers/infiniband/ulp/iser/iser_memory.c    |  46 +++++------
 drivers/infiniband/ulp/iser/iser_verbs.c     |  28 ++++---
 drivers/infiniband/ulp/isert/ib_isert.c      |  66 +++++++++++-----
 drivers/infiniband/ulp/isert/ib_isert.h      |  39 +++++++++-
 drivers/infiniband/ulp/isert/isert_proto.h   |  47 ------------
 include/scsi/iser.h                          | 109 +++++++++++++++++++++++++++
 9 files changed, 310 insertions(+), 147 deletions(-)
 delete mode 100644 drivers/infiniband/ulp/isert/isert_proto.h
 create mode 100644 include/scsi/iser.h

-- 
1.8.4.3

[PATCH for-next 01/10] IB/iser: Fix module init not cleaning up on error flow

[Sagi Grimberg]

From: Roi Dayan <roid@mellanox.com>

destroy workqueue on transport register error
release kmem cache on workqueue alloc error

Signed-off-by: Roi Dayan <roid@mellanox.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 8017f3a049fb..f1ddad34b001 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -1063,7 +1063,8 @@ static int __init iser_init(void)
 	release_wq = alloc_workqueue("release workqueue", 0, 0);
 	if (!release_wq) {
 		iser_err("failed to allocate release workqueue\n");
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto err_alloc_wq;
 	}
 
 	iscsi_iser_scsi_transport = iscsi_register_transport(
@@ -1071,12 +1072,14 @@ static int __init iser_init(void)
 	if (!iscsi_iser_scsi_transport) {
 		iser_err("iscsi_register_transport failed\n");
 		err = -EINVAL;
-		goto register_transport_failure;
+		goto err_reg;
 	}
 
 	return 0;
 
-register_transport_failure:
+err_reg:
+	destroy_workqueue(release_wq);
+err_alloc_wq:
 	kmem_cache_destroy(ig.desc_cache);
 
 	return err;
-- 
1.8.4.3

Re: [PATCH for-next 01/10] IB/iser: Fix module init not cleaning up on error flow

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> destroy workqueue on transport register error
> release kmem cache on workqueue alloc error
Sagi and Co

Can you make sure to avoid skipping usage of capital letters when 
beginning a sentence
and use punctuations: comma/s, period/s?

Or.


--
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 for-next 01/10] IB/iser: Fix module init not cleaning up on error flow

[Sagi Grimberg]

> Sagi and Co
>
> Can you make sure to avoid skipping usage of capital letters when
> beginning a sentence
> and use punctuations: comma/s, period/s?

Will do.
--
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 for-next 01/10] IB/iser: Fix module init not cleaning up on error flow

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Sagi Grimberg]

This is a pre-step before adding remote invalidate
support. Also, every ULP should prefer fastreg over
fmr.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iser_memory.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index ea765fb9664d..1103aa6e8d00 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -71,15 +71,14 @@ int iser_assign_reg_ops(struct iser_device *device)
 {
 	struct ib_device_attr *dev_attr = &device->dev_attr;
 
-	/* Assign function handles  - based on FMR support */
+	if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS) {
+		iser_info("FastReg supported, using FastReg for registration\n");
+		device->reg_ops = &fastreg_ops;
+	} else
 	if (device->ib_device->alloc_fmr && device->ib_device->dealloc_fmr &&
 	    device->ib_device->map_phys_fmr && device->ib_device->unmap_fmr) {
 		iser_info("FMR supported, using FMR for registration\n");
 		device->reg_ops = &fmr_ops;
-	} else
-	if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS) {
-		iser_info("FastReg supported, using FastReg for registration\n");
-		device->reg_ops = &fastreg_ops;
 	} else {
 		iser_err("IB device does not support FMRs nor FastRegs, can't register memory\n");
 		return -1;
-- 
1.8.4.3

Re: [PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> This is a pre-step before adding remote invalidate support.

Wouldn't that introduce performance regression on ConnectX3 devices?

Or.


--
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 for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Sagi Grimberg]

> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> This is a pre-step before adding remote invalidate support.
>
> Wouldn't that introduce performance regression on ConnectX3 devices?

With remote invalidate it shouldn't make much of a difference. Plus,
I'd really like us to start phasing out from FMRs...

Re: [PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Or Gerlitz]

On 11/17/2015 11:35 AM, Sagi Grimberg wrote:
>> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>>> This is a pre-step before adding remote invalidate support.
>>
>> Wouldn't that introduce performance regression on ConnectX3 devices?
>
> With remote invalidate it shouldn't make much of a difference. 

Why? the invalidate is just one part of the story, we are doing a 
mapping on IO submission
and CX3 has strong ordering on FRWRs, right?

> Plus, I'd really like us to start phasing out from FMRs... 

We should make sure not to introduce performance regression for HW which 
has
such a big existing install base and is well selling in 2015/16

Re: [PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Sagi Grimberg]

> Why? the invalidate is just one part of the story, we are doing a
> mapping on IO submission
> and CX3 has strong ordering on FRWRs, right?

Yes, this is correct.
We'll test on CX3 to see if this introduces a regression.

> We should make sure not to introduce performance regression for HW which
> has such a big existing install base and is well selling in 2015/16

You are right.

Re: [PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Christoph Hellwig]

> +	if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS) {
> +		iser_info("FastReg supported, using FastReg for registration\n");
> +		device->reg_ops = &fastreg_ops;
> +	} else
>  	if (device->ib_device->alloc_fmr && device->ib_device->dealloc_fmr &&

Should an 'else if'.  If the line gets to long it might be time to add a
rdma_cap_fmr() helper to ib_verbs.h

Re: [PATCH for-next 02/10] IB/iser: Default to fastreg instead of fmr

[Sagi Grimberg]

On 17/11/2015 10:57, Christoph Hellwig wrote:
>> +	if (dev_attr->device_cap_flags & IB_DEVICE_MEM_MGT_EXTENSIONS) {
>> +		iser_info("FastReg supported, using FastReg for registration\n");
>> +		device->reg_ops = &fastreg_ops;
>> +	} else
>>   	if (device->ib_device->alloc_fmr && device->ib_device->dealloc_fmr &&
>
> Should an 'else if'.  If the line gets to long it might be time to add a
> rdma_cap_fmr() helper to ib_verbs.h

OK, I will.

[PATCH for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Sagi Grimberg]

From: Jenny Derzhavetz <jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

When all the task data is sent as immeidatedata, we are
allowed to use the local_dma_lkey as it is not sent to
the wire.

Signed-off-by: Jenny Derzhavetz <jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h     | 3 ++-
 drivers/infiniband/ulp/iser/iser_initiator.c | 5 +++--
 drivers/infiniband/ulp/iser/iser_memory.c    | 9 +++++----
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 8a5998e6a407..fb7fa7aa113c 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -651,7 +651,8 @@ void iser_finalize_rdma_unaligned_sg(struct iscsi_iser_task *iser_task,
 				     enum iser_data_dir cmd_dir);
 
 int iser_reg_rdma_mem(struct iscsi_iser_task *task,
-		      enum iser_data_dir dir);
+		      enum iser_data_dir dir,
+		      bool all_imm);
 void iser_unreg_rdma_mem(struct iscsi_iser_task *task,
 			 enum iser_data_dir dir);
 
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index ffd00c420729..07bf26427ee7 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -72,7 +72,7 @@ static int iser_prepare_read_cmd(struct iscsi_task *task)
 			return err;
 	}
 
-	err = iser_reg_rdma_mem(iser_task, ISER_DIR_IN);
+	err = iser_reg_rdma_mem(iser_task, ISER_DIR_IN, false);
 	if (err) {
 		iser_err("Failed to set up Data-IN RDMA\n");
 		return err;
@@ -126,7 +126,8 @@ iser_prepare_write_cmd(struct iscsi_task *task,
 			return err;
 	}
 
-	err = iser_reg_rdma_mem(iser_task, ISER_DIR_OUT);
+	err = iser_reg_rdma_mem(iser_task, ISER_DIR_OUT,
+				buf_out->data_len == imm_sz);
 	if (err != 0) {
 		iser_err("Failed to register write cmd RDMA mem\n");
 		return err;
diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index 1103aa6e8d00..e500d0df7b85 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -250,7 +250,7 @@ iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
 	struct scatterlist *sg = mem->sg;
 
 	reg->sge.lkey = device->pd->local_dma_lkey;
-	reg->rkey = device->mr->rkey;
+	reg->rkey = device->mr ? device->mr->rkey : 0;
 	reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
 	reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);
 
@@ -553,7 +553,8 @@ iser_reg_data_sg(struct iscsi_iser_task *task,
 }
 
 int iser_reg_rdma_mem(struct iscsi_iser_task *task,
-		      enum iser_data_dir dir)
+		      enum iser_data_dir dir,
+		      bool all_imm)
 {
 	struct ib_conn *ib_conn = &task->iser_conn->ib_conn;
 	struct iser_device *device = ib_conn->device;
@@ -564,8 +565,8 @@ int iser_reg_rdma_mem(struct iscsi_iser_task *task,
 	bool use_dma_key;
 	int err;
 
-	use_dma_key = (mem->dma_nents == 1 && !iser_always_reg &&
-		       scsi_get_prot_op(task->sc) == SCSI_PROT_NORMAL);
+	use_dma_key = mem->dma_nents == 1 && (all_imm || !iser_always_reg) &&
+		      scsi_get_prot_op(task->sc) == SCSI_PROT_NORMAL;
 
 	if (!use_dma_key) {
 		desc = device->reg_ops->reg_desc_get(ib_conn);
-- 
1.8.4.3

--
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 for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> --- a/drivers/infiniband/ulp/iser/iser_memory.c
> +++ b/drivers/infiniband/ulp/iser/iser_memory.c
> @@ -250,7 +250,7 @@ iser_reg_dma(struct iser_device *device, struct iser_data_buf *mem,
>   	struct scatterlist *sg = mem->sg;
>   
>   	reg->sge.lkey = device->pd->local_dma_lkey;
> -	reg->rkey = device->mr->rkey;
> +	reg->rkey = device->mr ? device->mr->rkey : 0;
>   	reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
>   	reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);
>   

what's the role of this hunk? why it belongs here? you are testing 
device->mr but this is
something global and has nothing to do specially with specific IOs for 
which this patch
aims to act

Re: [PATCH for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Sagi Grimberg]

> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> --- a/drivers/infiniband/ulp/iser/iser_memory.c
>> +++ b/drivers/infiniband/ulp/iser/iser_memory.c
>> @@ -250,7 +250,7 @@ iser_reg_dma(struct iser_device *device, struct
>> iser_data_buf *mem,
>>       struct scatterlist *sg = mem->sg;
>>       reg->sge.lkey = device->pd->local_dma_lkey;
>> -    reg->rkey = device->mr->rkey;
>> +    reg->rkey = device->mr ? device->mr->rkey : 0;
>>       reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
>>       reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);
>
> what's the role of this hunk? why it belongs here? you are testing
> device->mr but this is
> something global and has nothing to do specially with specific IOs for
> which this patch
> aims to act

It's because device->mr might not be allocated at all if
always_register=Y, however in this case for all-immediatedata writes
I don't need memory registration and I can use pd->local_dma_lkey.

This hunk prevents a NULL dereference (as I mentioned, device->mr might
not be allocated at all).
--
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 for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Or Gerlitz]

On 11/17/2015 11:41 AM, Sagi Grimberg wrote:
>
>> what's the role of this hunk? why it belongs here? you are testing
>> device->mr but this is something global and has nothing to do 
>> specially with specific IOs for
>> which this patch aims to act
>
> It's because device->mr might not be allocated at all if
> always_register=Y, however in this case for all-immediatedata writes
> I don't need memory registration and I can use pd->local_dma_lkey.
>
> This hunk prevents a NULL dereference (as I mentioned, device->mr might
> not be allocated at all). 

I am not sure to follow, but can't you put this fix into a separate 
patch to make things more clearer?
--
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 for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Sagi Grimberg]

>> It's because device->mr might not be allocated at all if
>> always_register=Y, however in this case for all-immediatedata writes
>> I don't need memory registration and I can use pd->local_dma_lkey.
>>
>> This hunk prevents a NULL dereference (as I mentioned, device->mr might
>> not be allocated at all).
>
> I am not sure to follow, but can't you put this fix into a separate
> patch to make things more clearer?

I don't know how to split this patch any further. Before this patch in
case always_register=Y we *always registered* and never allocated the
global mr. This patch allows us not to do it ifor all-immediatedata
writes. So in order not to hit a NULL deref we use this conditional.
--
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 for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Jason Gunthorpe]

On Tue, Nov 17, 2015 at 11:41:39AM +0200, Sagi Grimberg wrote:
> 
> >On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> >>+++ b/drivers/infiniband/ulp/iser/iser_memory.c
> >>@@ -250,7 +250,7 @@ iser_reg_dma(struct iser_device *device, struct
> >>iser_data_buf *mem,
> >>      struct scatterlist *sg = mem->sg;
> >>      reg->sge.lkey = device->pd->local_dma_lkey;
> >>-    reg->rkey = device->mr->rkey;
> >>+    reg->rkey = device->mr ? device->mr->rkey : 0;
> >>      reg->sge.addr = ib_sg_dma_address(device->ib_device, &sg[0]);
> >>      reg->sge.length = ib_sg_dma_len(device->ib_device, &sg[0]);
> >
> >what's the role of this hunk? why it belongs here? you are testing
> >device->mr but this is
> >something global and has nothing to do specially with specific IOs for
> >which this patch
> >aims to act
> 
> It's because device->mr might not be allocated at all if
> always_register=Y, however in this case for all-immediatedata writes
> I don't need memory registration and I can use pd->local_dma_lkey.

I'm with Or on this, this is really goofy looking.

This routine probably should not be setting the rkey at all, it makes
no sense to have a routine that returns a lkey and a rkey. Those are
always different flows.

Once that is fixed then the above if can be hoisted to the actual
calling code that needs an rkey, at the point where it does something
different when !iser_always_reg is true..

Jason

Re: [PATCH for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Sagi Grimberg]

Jason and Or,

>
> I'm with Or on this, this is really goofy looking.
>
> This routine probably should not be setting the rkey at all, it makes
> no sense to have a routine that returns a lkey and a rkey. Those are
> always different flows.
>
> Once that is fixed then the above if can be hoisted to the actual
> calling code that needs an rkey, at the point where it does something
> different when !iser_always_reg is true..

This change is acceptable to me. However, given that it includes
rework outside the scope of this specific patch which is a performance
regression fix, would it be acceptable to have it incremental and
provide a FIXME comment here?

Re: [PATCH for-next 03/10] IB/iser: Don't register memory for all immediatedata writes

[Jason Gunthorpe]

On Tue, Nov 24, 2015 at 12:04:36PM +0200, Sagi Grimberg wrote:
> Jason and Or,
> 
> >
> >I'm with Or on this, this is really goofy looking.
> >
> >This routine probably should not be setting the rkey at all, it makes
> >no sense to have a routine that returns a lkey and a rkey. Those are
> >always different flows.
> >
> >Once that is fixed then the above if can be hoisted to the actual
> >calling code that needs an rkey, at the point where it does something
> >different when !iser_always_reg is true..
> 
> This change is acceptable to me. However, given that it includes
> rework outside the scope of this specific patch which is a performance
> regression fix, would it be acceptable to have it incremental and
> provide a FIXME comment here?

I'm not bothered by the ordering as long as we are clear where things
are going :)

Jason

[PATCH for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Sagi Grimberg]

From: Jenny Derzhavetz <jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

iser target does not support zero based virtual addresses and
send with invalidate, so it should declare that it doesn't.

Signed-off-by: Jenny Derzhavetz <jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index bdda42638629..667238f6253f 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -3099,12 +3099,18 @@ isert_rdma_accept(struct isert_conn *isert_conn)
 	struct rdma_cm_id *cm_id = isert_conn->cm_id;
 	struct rdma_conn_param cp;
 	int ret;
+	struct iser_cm_hdr rsp_hdr;
 
 	memset(&cp, 0, sizeof(struct rdma_conn_param));
 	cp.initiator_depth = isert_conn->initiator_depth;
 	cp.retry_count = 7;
 	cp.rnr_retry_count = 7;
 
+	memset(&rsp_hdr, 0, sizeof(rsp_hdr));
+	rsp_hdr.flags = (ISERT_ZBVA_NOT_USED | ISERT_SEND_W_INV_NOT_USED);
+	cp.private_data = (void *)&rsp_hdr;
+	cp.private_data_len = sizeof(rsp_hdr);
+
 	ret = rdma_accept(cm_id, &cp);
 	if (ret) {
 		isert_err("rdma_accept() failed with: %d\n", ret);
-- 
1.8.4.3

--
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 for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> From: Jenny Derzhavetz<jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>
> iser target does not support zero based virtual addresses and
> send with invalidate, so it should declare that it doesn't.
>
>

This better go to stable kernels too, however there's little ugliness 
involved since
struct iser_cm_hdr doesn't exist for LIO in those kernels, thoughts? did 
you get
any complaints from probably not Linux initiators interop-ing with LIO 
on that matter?

Or.
--
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 for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Sagi Grimberg]

> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> From: Jenny Derzhavetz<jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>
>> iser target does not support zero based virtual addresses and
>> send with invalidate, so it should declare that it doesn't.
>>
>>
>
> This better go to stable kernels too, however there's little ugliness
> involved since
> struct iser_cm_hdr doesn't exist for LIO in those kernels, thoughts?

Umm, I think older kernels will interop just fine as the initiator
considers rdma_cm accepts with no private data as not supporting any
features (remote invalidate, zbva).

> did you get any complaints from probably not Linux initiators interop-ing with LIO
> on that matter?

Nope... Have you?
--
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 for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Or Gerlitz]

On 11/17/2015 11:48 AM, Sagi Grimberg wrote:
>>> iser target does not support zero based virtual addresses and
>>> send with invalidate, so it should declare that it doesn't.
>>>
>>>
>>
>> This better go to stable kernels too, however there's little ugliness
>> involved since struct iser_cm_hdr doesn't exist for LIO in those 
>> kernels, thoughts?
>
> Umm, I think older kernels will interop just fine as the initiator
> considers rdma_cm accepts with no private data as not supporting any
> features (remote invalidate, zbva).

mmm, maybe we can let it go as you want... so no stabling here.

>
>> did you get any complaints from probably not Linux initiators 
>> interop-ing with LIO
>> on that matter?
>
> Nope... Have you? 

no
--
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 for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Christoph Hellwig]

> +	struct iser_cm_hdr rsp_hdr;
>  
>  	memset(&cp, 0, sizeof(struct rdma_conn_param));
>  	cp.initiator_depth = isert_conn->initiator_depth;
>  	cp.retry_count = 7;
>  	cp.rnr_retry_count = 7;
>  
> +	memset(&rsp_hdr, 0, sizeof(rsp_hdr));
> +	rsp_hdr.flags = (ISERT_ZBVA_NOT_USED | ISERT_SEND_W_INV_NOT_USED);
> +	cp.private_data = (void *)&rsp_hdr;
> +	cp.private_data_len = sizeof(rsp_hdr);

Btw, where does iser_cm_hdr and these flags come from?  I can't find
anything like this in RFC7145.
--
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 for-next 07/10] iser-target: Declare correct flags when accepting a connection

[Sagi Grimberg]

>> +	struct iser_cm_hdr rsp_hdr;
>>
>>   	memset(&cp, 0, sizeof(struct rdma_conn_param));
>>   	cp.initiator_depth = isert_conn->initiator_depth;
>>   	cp.retry_count = 7;
>>   	cp.rnr_retry_count = 7;
>>
>> +	memset(&rsp_hdr, 0, sizeof(rsp_hdr));
>> +	rsp_hdr.flags = (ISERT_ZBVA_NOT_USED | ISERT_SEND_W_INV_NOT_USED);
>> +	cp.private_data = (void *)&rsp_hdr;
>> +	cp.private_data_len = sizeof(rsp_hdr);
>
> Btw, where does iser_cm_hdr and these flags come from?  I can't find
> anything like this in RFC7145.

It's mentioned in the cover-letter:
"The support negotiation for this feature is based on IBTA
annex 12 "Support for iSCSI Extensions for RDMA" carried in rdma_cm
private data."

You can find it at:
https://cw.infinibandta.org/document/dl/7145

[PATCH for-next 09/10] IB/iser: Increment the rkey when registering and not when invalidating

[Sagi Grimberg]

With remote invalidate we won't local invalidate
but we still want to increment the rkey.

Signed-off-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Jenny Derzhavetz <jennyf-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iser_memory.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index fb676ae25c42..52f64073037b 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -412,19 +412,14 @@ iser_set_prot_checks(struct scsi_cmnd *sc, u8 *mask)
 		*mask |= ISER_CHECK_GUARD;
 }
 
-static void
+static inline void
 iser_inv_rkey(struct ib_send_wr *inv_wr, struct ib_mr *mr)
 {
-	u32 rkey;
-
 	inv_wr->opcode = IB_WR_LOCAL_INV;
 	inv_wr->wr_id = ISER_FASTREG_LI_WRID;
 	inv_wr->ex.invalidate_rkey = mr->rkey;
 	inv_wr->send_flags = 0;
 	inv_wr->num_sge = 0;
-
-	rkey = ib_inc_rkey(mr->rkey);
-	ib_update_fast_reg_key(mr, rkey);
 }
 
 static int
@@ -437,6 +432,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 	struct iser_tx_desc *tx_desc = &iser_task->desc;
 	struct ib_sig_attrs *sig_attrs = &tx_desc->sig_attrs;
 	struct ib_sig_handover_wr *wr;
+	struct ib_mr *mr = pi_ctx->sig_mr;
 	int ret;
 
 	memset(sig_attrs, 0, sizeof(*sig_attrs));
@@ -447,7 +443,9 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 	iser_set_prot_checks(iser_task->sc, &sig_attrs->check_mask);
 
 	if (pi_ctx->sig_mr_valid)
-		iser_inv_rkey(iser_tx_next_wr(tx_desc), pi_ctx->sig_mr);
+		iser_inv_rkey(iser_tx_next_wr(tx_desc), mr);
+
+	ib_update_fast_reg_key(mr, ib_inc_rkey(mr->rkey));
 
 	wr = sig_handover_wr(iser_tx_next_wr(tx_desc));
 	wr->wr.opcode = IB_WR_REG_SIG_MR;
@@ -456,7 +454,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 	wr->wr.num_sge = 1;
 	wr->wr.send_flags = 0;
 	wr->sig_attrs = sig_attrs;
-	wr->sig_mr = pi_ctx->sig_mr;
+	wr->sig_mr = mr;
 	if (scsi_prot_sg_count(iser_task->sc))
 		wr->prot = &prot_reg->sge;
 	else
@@ -466,8 +464,8 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 			   IB_ACCESS_REMOTE_WRITE;
 	pi_ctx->sig_mr_valid = 1;
 
-	sig_reg->sge.lkey = pi_ctx->sig_mr->lkey;
-	sig_reg->rkey = pi_ctx->sig_mr->rkey;
+	sig_reg->sge.lkey = mr->lkey;
+	sig_reg->rkey = mr->rkey;
 	sig_reg->sge.addr = 0;
 	sig_reg->sge.length = scsi_transfer_length(iser_task->sc);
 
@@ -491,6 +489,8 @@ static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
 	if (rsc->mr_valid)
 		iser_inv_rkey(iser_tx_next_wr(tx_desc), mr);
 
+	ib_update_fast_reg_key(mr, ib_inc_rkey(mr->rkey));
+
 	n = ib_map_mr_sg(mr, mem->sg, mem->size, SIZE_4K);
 	if (unlikely(n != mem->size)) {
 		iser_err("failed to map sg (%d/%d)\n",
-- 
1.8.4.3

--
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 for-next 09/10] IB/iser: Increment the rkey when registering and not when invalidating

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> With remote invalidate we won't local invalidate
> but we still want to increment the rkey.

To ease with review, as this is low level pre-patch, please push it to 
downstream area
of the series, e.g before the patch that introduces include/scsi/iser.h 
-- also use comma
to make the sentence more readable, maybe something like: "With remote 
invalidate we won't
do local invalidate, but we still want to increment the rkey."

Or.

Re: [PATCH for-next 00/10] iSER support for remote invalidate

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> This patchset adds remote invalidation support to iser initiator and
> target. The support negotiation for this feature is based on IBTA
> annex 12 "Support for iSCSI Extensions for RDMA" carried in rdma_cm
> private data.
On which public git tree/branch this series applies?

Or.
--
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 for-next 00/10] iSER support for remote invalidate

[Sagi Grimberg]

On 17/11/2015 10:10, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> This patchset adds remote invalidation support to iser initiator and
>> target. The support negotiation for this feature is based on IBTA
>> annex 12 "Support for iSCSI Extensions for RDMA" carried in rdma_cm
>> private data.
> On which public git tree/branch this series applies?

On doug's tree.

I'll add a git branch in the next round...
--
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 for-next 04/10] IB/iser: set intuitive values for mr_valid

[Sagi Grimberg]

From: Jenny Derzhavetz <jennyf@mellanox.com>

This parameter is described as "is mr valid indicator".
In other words, it indicates whether memory registration
is valid or not. So intuitive values would be:
mr_valid=True, when memory registration is valid and
mr_valid=False otherwise.

Signed-off-by: Jenny Derzhavetz <jennyf@mellanox.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iser_memory.c | 8 ++++----
 drivers/infiniband/ulp/iser/iser_verbs.c  | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iser_memory.c b/drivers/infiniband/ulp/iser/iser_memory.c
index e500d0df7b85..fb676ae25c42 100644
--- a/drivers/infiniband/ulp/iser/iser_memory.c
+++ b/drivers/infiniband/ulp/iser/iser_memory.c
@@ -446,7 +446,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 
 	iser_set_prot_checks(iser_task->sc, &sig_attrs->check_mask);
 
-	if (!pi_ctx->sig_mr_valid)
+	if (pi_ctx->sig_mr_valid)
 		iser_inv_rkey(iser_tx_next_wr(tx_desc), pi_ctx->sig_mr);
 
 	wr = sig_handover_wr(iser_tx_next_wr(tx_desc));
@@ -464,7 +464,7 @@ iser_reg_sig_mr(struct iscsi_iser_task *iser_task,
 	wr->access_flags = IB_ACCESS_LOCAL_WRITE |
 			   IB_ACCESS_REMOTE_READ |
 			   IB_ACCESS_REMOTE_WRITE;
-	pi_ctx->sig_mr_valid = 0;
+	pi_ctx->sig_mr_valid = 1;
 
 	sig_reg->sge.lkey = pi_ctx->sig_mr->lkey;
 	sig_reg->rkey = pi_ctx->sig_mr->rkey;
@@ -488,7 +488,7 @@ static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
 	struct ib_reg_wr *wr;
 	int n;
 
-	if (!rsc->mr_valid)
+	if (rsc->mr_valid)
 		iser_inv_rkey(iser_tx_next_wr(tx_desc), mr);
 
 	n = ib_map_mr_sg(mr, mem->sg, mem->size, SIZE_4K);
@@ -509,7 +509,7 @@ static int iser_fast_reg_mr(struct iscsi_iser_task *iser_task,
 		     IB_ACCESS_REMOTE_WRITE |
 		     IB_ACCESS_REMOTE_READ;
 
-	rsc->mr_valid = 0;
+	rsc->mr_valid = 1;
 
 	reg->sge.lkey = mr->lkey;
 	reg->rkey = mr->rkey;
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index a66b9dea96d8..09121e774d14 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -299,7 +299,7 @@ iser_alloc_reg_res(struct ib_device *ib_device,
 		iser_err("Failed to allocate ib_fast_reg_mr err=%d\n", ret);
 		return ret;
 	}
-	res->mr_valid = 1;
+	res->mr_valid = 0;
 
 	return 0;
 }
@@ -336,7 +336,7 @@ iser_alloc_pi_ctx(struct ib_device *ib_device,
 		ret = PTR_ERR(pi_ctx->sig_mr);
 		goto sig_mr_failure;
 	}
-	pi_ctx->sig_mr_valid = 1;
+	pi_ctx->sig_mr_valid = 0;
 	desc->pi_ctx->sig_protected = 0;
 
 	return 0;
-- 
1.8.4.3

Re: [PATCH for-next 04/10] IB/iser: set intuitive values for mr_valid

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> This parameter is described as "is mr valid indicator".
> In other words, it indicates whether memory registration
> is valid or not. So intuitive values would be:
> mr_valid=True, when memory registration is valid and
> mr_valid=False otherwise.

and what was there before? the other way around?

on what cases the indicator is false (with the new semantics)?

Also, we have a convention of starting commit titles with capital 
letters, please follow on that "Set iser [...]"

Re: [PATCH for-next 04/10] IB/iser: set intuitive values for mr_valid

[Sagi Grimberg]

On 17/11/2015 09:43, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> This parameter is described as "is mr valid indicator".
>> In other words, it indicates whether memory registration
>> is valid or not. So intuitive values would be:
>> mr_valid=True, when memory registration is valid and
>> mr_valid=False otherwise.
>
> and what was there before? the other way around?
>
> on what cases the indicator is false (with the new semantics)?

This is just an attempt to fix the confusing semantics I gave these
flags. If the MR is valid it needs invalidation, that's it...

>
> Also, we have a convention of starting commit titles with capital
> letters, please follow on that "Set iser [...]"

Will fix...
--
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 for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Sagi Grimberg]

The iser RDMA_CM negotiation protocol is shared by
the initiator and the target, so have a shared header
for the defines and structure. Move relevant items from
the initiator and target headers.

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Jenny Derzhavetz <jennyf@mellanox.com>
Cc: linux-scsi <linux-scsi@vger.kernel.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h     |  41 ++--------
 drivers/infiniband/ulp/iser/iser_initiator.c |   6 +-
 drivers/infiniband/ulp/iser/iser_verbs.c     |   7 +-
 drivers/infiniband/ulp/isert/ib_isert.c      |  22 +++---
 drivers/infiniband/ulp/isert/ib_isert.h      |   6 +-
 include/scsi/iser.h                          | 109 +++++++++++++++++++++++++++
 6 files changed, 135 insertions(+), 56 deletions(-)
 create mode 100644 include/scsi/iser.h

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index fb7fa7aa113c..096d5234bbea 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -48,6 +48,7 @@
 #include <scsi/scsi_transport_iscsi.h>
 #include <scsi/scsi_cmnd.h>
 #include <scsi/scsi_device.h>
+#include <scsi/iser.h>
 
 #include <linux/interrupt.h>
 #include <linux/wait.h>
@@ -154,43 +155,11 @@
 #define ISER_WC_BATCH_COUNT   16
 #define ISER_SIGNAL_CMD_COUNT 32
 
-#define ISER_VER			0x10
-#define ISER_WSV			0x08
-#define ISER_RSV			0x04
-
 #define ISER_FASTREG_LI_WRID		0xffffffffffffffffULL
 #define ISER_BEACON_WRID		0xfffffffffffffffeULL
 
-/**
- * struct iser_hdr - iSER header
- *
- * @flags:        flags support (zbva, remote_inv)
- * @rsvd:         reserved
- * @write_stag:   write rkey
- * @write_va:     write virtual address
- * @reaf_stag:    read rkey
- * @read_va:      read virtual address
- */
-struct iser_hdr {
-	u8      flags;
-	u8      rsvd[3];
-	__be32  write_stag;
-	__be64  write_va;
-	__be32  read_stag;
-	__be64  read_va;
-} __attribute__((packed));
-
-
-#define ISER_ZBVA_NOT_SUPPORTED		0x80
-#define ISER_SEND_W_INV_NOT_SUPPORTED	0x40
-
-struct iser_cm_hdr {
-	u8      flags;
-	u8      rsvd[3];
-} __packed;
-
-/* Constant PDU lengths calculations */
-#define ISER_HEADERS_LEN  (sizeof(struct iser_hdr) + sizeof(struct iscsi_hdr))
+/*Constant PDU lengths calculations */
+#define ISER_HEADERS_LEN       (sizeof(struct iser_ctrl) + sizeof(struct iscsi_hdr))
 
 #define ISER_RECV_DATA_SEG_LEN	128
 #define ISER_RX_PAYLOAD_SIZE	(ISER_HEADERS_LEN + ISER_RECV_DATA_SEG_LEN)
@@ -287,7 +256,7 @@ enum iser_desc_type {
  * @sig_attrs:     Signature attributes
  */
 struct iser_tx_desc {
-	struct iser_hdr              iser_header;
+	struct iser_ctrl             iser_header;
 	struct iscsi_hdr             iscsi_header;
 	enum   iser_desc_type        type;
 	u64		             dma_addr;
@@ -318,7 +287,7 @@ struct iser_tx_desc {
  * @pad:           for sense data TODO: Modify to maximum sense length supported
  */
 struct iser_rx_desc {
-	struct iser_hdr              iser_header;
+	struct iser_ctrl             iser_header;
 	struct iscsi_hdr             iscsi_header;
 	char		             data[ISER_RECV_DATA_SEG_LEN];
 	u64		             dma_addr;
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 07bf26427ee7..6a968e350c14 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -51,7 +51,7 @@ static int iser_prepare_read_cmd(struct iscsi_task *task)
 	struct iscsi_iser_task *iser_task = task->dd_data;
 	struct iser_mem_reg *mem_reg;
 	int err;
-	struct iser_hdr *hdr = &iser_task->desc.iser_header;
+	struct iser_ctrl *hdr = &iser_task->desc.iser_header;
 	struct iser_data_buf *buf_in = &iser_task->data[ISER_DIR_IN];
 
 	err = iser_dma_map_task_data(iser_task,
@@ -104,7 +104,7 @@ iser_prepare_write_cmd(struct iscsi_task *task,
 	struct iscsi_iser_task *iser_task = task->dd_data;
 	struct iser_mem_reg *mem_reg;
 	int err;
-	struct iser_hdr *hdr = &iser_task->desc.iser_header;
+	struct iser_ctrl *hdr = &iser_task->desc.iser_header;
 	struct iser_data_buf *buf_out = &iser_task->data[ISER_DIR_OUT];
 	struct ib_sge *tx_dsg = &iser_task->desc.tx_sg[1];
 
@@ -167,7 +167,7 @@ static void iser_create_send_desc(struct iser_conn	*iser_conn,
 	ib_dma_sync_single_for_cpu(device->ib_device,
 		tx_desc->dma_addr, ISER_HEADERS_LEN, DMA_TO_DEVICE);
 
-	memset(&tx_desc->iser_header, 0, sizeof(struct iser_hdr));
+	memset(&tx_desc->iser_header, 0, sizeof(struct iser_ctrl));
 	tx_desc->iser_header.flags = ISER_VER;
 	tx_desc->num_sge = 1;
 }
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 09121e774d14..74161a566852 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -847,10 +847,9 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
 	conn_param.rnr_retry_count     = 6;
 
 	memset(&req_hdr, 0, sizeof(req_hdr));
-	req_hdr.flags = (ISER_ZBVA_NOT_SUPPORTED |
-			ISER_SEND_W_INV_NOT_SUPPORTED);
-	conn_param.private_data		= (void *)&req_hdr;
-	conn_param.private_data_len	= sizeof(struct iser_cm_hdr);
+	req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
+	conn_param.private_data	= (void *)&req_hdr;
+	conn_param.private_data_len = sizeof(struct iser_cm_hdr);
 
 	ret = rdma_connect(cma_id, &conn_param);
 	if (ret) {
diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 7dbae56e576a..97ac9ffaf1eb 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -1054,8 +1054,8 @@ isert_create_send_desc(struct isert_conn *isert_conn,
 	ib_dma_sync_single_for_cpu(ib_dev, tx_desc->dma_addr,
 				   ISER_HEADERS_LEN, DMA_TO_DEVICE);
 
-	memset(&tx_desc->iser_header, 0, sizeof(struct iser_hdr));
-	tx_desc->iser_header.flags = ISER_VER;
+	memset(&tx_desc->iser_header, 0, sizeof(struct iser_ctrl));
+	tx_desc->iser_header.flags = ISCSI_CTRL;
 
 	tx_desc->num_sge = 1;
 	tx_desc->isert_cmd = isert_cmd;
@@ -1547,22 +1547,22 @@ isert_rx_opcode(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc,
 static void
 isert_rx_do_work(struct iser_rx_desc *rx_desc, struct isert_conn *isert_conn)
 {
-	struct iser_hdr *iser_hdr = &rx_desc->iser_header;
+	struct iser_ctrl *iser_ctrl = &rx_desc->iser_header;
 	uint64_t read_va = 0, write_va = 0;
 	uint32_t read_stag = 0, write_stag = 0;
 	int rc;
 
-	switch (iser_hdr->flags & 0xF0) {
+	switch (iser_ctrl->flags & 0xF0) {
 	case ISCSI_CTRL:
-		if (iser_hdr->flags & ISER_RSV) {
-			read_stag = be32_to_cpu(iser_hdr->read_stag);
-			read_va = be64_to_cpu(iser_hdr->read_va);
+		if (iser_ctrl->flags & ISER_RSV) {
+			read_stag = be32_to_cpu(iser_ctrl->read_stag);
+			read_va = be64_to_cpu(iser_ctrl->read_va);
 			isert_dbg("ISER_RSV: read_stag: 0x%x read_va: 0x%llx\n",
 				  read_stag, (unsigned long long)read_va);
 		}
-		if (iser_hdr->flags & ISER_WSV) {
-			write_stag = be32_to_cpu(iser_hdr->write_stag);
-			write_va = be64_to_cpu(iser_hdr->write_va);
+		if (iser_ctrl->flags & ISER_WSV) {
+			write_stag = be32_to_cpu(iser_ctrl->write_stag);
+			write_va = be64_to_cpu(iser_ctrl->write_va);
 			isert_dbg("ISER_WSV: write_stag: 0x%x write_va: 0x%llx\n",
 				  write_stag, (unsigned long long)write_va);
 		}
@@ -1573,7 +1573,7 @@ isert_rx_do_work(struct iser_rx_desc *rx_desc, struct isert_conn *isert_conn)
 		isert_err("iSER Hello message\n");
 		break;
 	default:
-		isert_warn("Unknown iSER hdr flags: 0x%02x\n", iser_hdr->flags);
+		isert_warn("Unknown iSER hdr flags: 0x%02x\n", iser_ctrl->flags);
 		break;
 	}
 
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 3d7fbc47c343..03ddc3e0087f 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -3,6 +3,8 @@
 #include <linux/in6.h>
 #include <rdma/ib_verbs.h>
 #include <rdma/rdma_cm.h>
+#include <scsi/iser.h>
+
 
 #define DRV_NAME	"isert"
 #define PFX		DRV_NAME ": "
@@ -56,7 +58,7 @@ enum iser_conn_state {
 };
 
 struct iser_rx_desc {
-	struct iser_hdr iser_header;
+	struct iser_ctrl iser_header;
 	struct iscsi_hdr iscsi_header;
 	char		data[ISER_RECV_DATA_SEG_LEN];
 	u64		dma_addr;
@@ -65,7 +67,7 @@ struct iser_rx_desc {
 } __packed;
 
 struct iser_tx_desc {
-	struct iser_hdr iser_header;
+	struct iser_ctrl iser_header;
 	struct iscsi_hdr iscsi_header;
 	enum isert_desc_type type;
 	u64		dma_addr;
diff --git a/include/scsi/iser.h b/include/scsi/iser.h
new file mode 100644
index 000000000000..16098de23f4b
--- /dev/null
+++ b/include/scsi/iser.h
@@ -0,0 +1,109 @@
+/*
+ * Copyright (c) 2015 Mellanox Technologies. All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *	- Redistributions of source code must retain the above
+ *	  copyright notice, this list of conditions and the following
+ *	  disclaimer.
+ *
+ *	- Redistributions in binary form must reproduce the above
+ *	  copyright notice, this list of conditions and the following
+ *	  disclaimer in the documentation and/or other materials
+ *	  provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+#ifndef ISCSI_ISER_H
+#define ISCSI_ISER_H
+
+#define ISER_ZBVA_NOT_SUP		0x80
+#define ISER_SEND_W_INV_NOT_SUP		0x40
+#define ISERT_ZBVA_NOT_USED		0x80
+#define ISERT_SEND_W_INV_NOT_USED	0x40
+
+#define ISCSI_CTRL	0x10
+#define ISER_HELLO	0x20
+#define ISER_HELLORPLY	0x30
+
+#define ISER_VER	0x10
+#define ISER_WSV	0x08
+#define ISER_RSV	0x04
+
+/**
+ * struct iser_cm_hdr - iSER CM header (from iSER Annex A12)
+ *
+ * @flags:        flags support (zbva, send_w_inv)
+ * @rsvd:         reserved
+ */
+struct iser_cm_hdr {
+	u8      flags;
+	u8      rsvd[3];
+} __packed;
+
+/**
+ * struct iser_ctrl - iSER header of iSCSI control PDU
+ *
+ * @flags:        opcode and read/write valid bits
+ * @rsvd:         reserved
+ * @write_stag:   write rkey
+ * @write_va:     write virtual address
+ * @reaf_stag:    read rkey
+ * @read_va:      read virtual address
+ */
+struct iser_ctrl {
+	u8      flags;
+	u8      rsvd[3];
+	__be32  write_stag;
+	__be64  write_va;
+	__be32  read_stag;
+	__be64  read_va;
+} __packed;
+
+/**
+ * struct iser_hello - iSER Hello header
+ *
+ * @opcode:       opcode (must be set to ISER_HELLO)
+ * @max_min_ver:  maximum and minimum iser versions
+ * @iser_ird:     iSER IRD
+ * @rsvd:         reserved
+ */
+struct iser_hello {
+	u8      opcode;
+	u8	max_min_ver;
+	u16	iser_ird;
+	u8	rsvd[20];
+} __packed;
+
+/**
+ * struct iser_hello_rep - iSER Hello reply header
+ *
+ * @opcode_rej:   opcode (must be set to ISER_HELLORPLY)
+ *                lower bit is reject bit
+ * @max_cur_ver:  maximum and current iser versions
+ * @iser_ord:     iSER ORD
+ * @rsvd:         reserved
+ */
+struct iser_hello_rep {
+	u8      opcode_rej;
+	u8	max_cur_ver;
+	u16	iser_ord;
+	u8	rsvd[20];
+} __packed;
+
+#endif /* ISCSI_ISER_H */
-- 
1.8.4.3

Re: [PATCH for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Christoph Hellwig]

On Mon, Nov 16, 2015 at 06:37:36PM +0200, Sagi Grimberg wrote:
> The iser RDMA_CM negotiation protocol is shared by
> the initiator and the target, so have a shared header
> for the defines and structure. Move relevant items from
> the initiator and target headers.

Nice!

But should a header that just documents protocol constants really
have a copyright header?

Re: [PATCH for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Sagi Grimberg]

On 17/11/2015 10:59, Christoph Hellwig wrote:
> On Mon, Nov 16, 2015 at 06:37:36PM +0200, Sagi Grimberg wrote:
>> The iser RDMA_CM negotiation protocol is shared by
>> the initiator and the target, so have a shared header
>> for the defines and structure. Move relevant items from
>> the initiator and target headers.
>
> Nice!
>
> But should a header that just documents protocol constants really
> have a copyright header?

I don't know... Shouldn't it?

I just copied include/scsi/srp.h and changed it...
--
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 for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 11:58:26AM +0200, Sagi Grimberg wrote:
> I don't know... Shouldn't it?
> 
> I just copied include/scsi/srp.h and changed it...

Not a major issue, it just seemed a little strange to me.
--
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 for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> +/**
> + * struct iser_hello - iSER Hello header
> + *
> + * @opcode:       opcode (must be set to ISER_HELLO)
> + * @max_min_ver:  maximum and minimum iser versions
> + * @iser_ird:     iSER IRD
> + * @rsvd:         reserved
> + */
> +struct iser_hello {
> +	u8      opcode;
> +	u8	max_min_ver;
> +	u16	iser_ird;
> +	u8	rsvd[20];
> +} __packed;
> +
> +/**
> + * struct iser_hello_rep - iSER Hello reply header
> + *
> + * @opcode_rej:   opcode (must be set to ISER_HELLORPLY)
> + *                lower bit is reject bit
> + * @max_cur_ver:  maximum and current iser versions
> + * @iser_ord:     iSER ORD
> + * @rsvd:         reserved
> + */
> +struct iser_hello_rep {
> +	u8      opcode_rej;
> +	u8	max_cur_ver;
> +	u16	iser_ord;
> +	u8	rsvd[20];
> +} __packed;
> +

I don't see the point to include these two defs, we don't use them and 
Steve even got iser to work
over iwarp without them, so why care? we should only leave
>
> +#define ISER_HELLO	0x20
> +#define ISER_HELLORPLY	0x30
to allow warnings on them if we get such packets
--
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 for-next 05/10] iser: Have initiator and target to share protocol structures and definitions

[Sagi Grimberg]

On 19/11/2015 09:20, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> +/**
>> + * struct iser_hello - iSER Hello header
>> + *
>> + * @opcode:       opcode (must be set to ISER_HELLO)
>> + * @max_min_ver:  maximum and minimum iser versions
>> + * @iser_ird:     iSER IRD
>> + * @rsvd:         reserved
>> + */
>> +struct iser_hello {
>> +    u8      opcode;
>> +    u8    max_min_ver;
>> +    u16    iser_ird;
>> +    u8    rsvd[20];
>> +} __packed;
>> +
>> +/**
>> + * struct iser_hello_rep - iSER Hello reply header
>> + *
>> + * @opcode_rej:   opcode (must be set to ISER_HELLORPLY)
>> + *                lower bit is reject bit
>> + * @max_cur_ver:  maximum and current iser versions
>> + * @iser_ord:     iSER ORD
>> + * @rsvd:         reserved
>> + */
>> +struct iser_hello_rep {
>> +    u8      opcode_rej;
>> +    u8    max_cur_ver;
>> +    u16    iser_ord;
>> +    u8    rsvd[20];
>> +} __packed;
>> +
>
> I don't see the point to include these two defs, we don't use them and
> Steve even got iser to work
> over iwarp without them, so why care? we should only leave

It's part of the protocol so I figured it should go here even if
it's not used today.

I can remove them if you like...
--
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 for-next 06/10] iser-target: Remove unused file iser_proto.h

[Sagi Grimberg]

We don't need iser_proto.h anymore, remove it and
move (non-protocol) declarations to ib_isert.h

Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
Signed-off-by: Jenny Derzhavetz <jennyf@mellanox.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c    |  1 -
 drivers/infiniband/ulp/isert/ib_isert.h    | 31 ++++++++++++++++++++
 drivers/infiniband/ulp/isert/isert_proto.h | 47 ------------------------------
 3 files changed, 31 insertions(+), 48 deletions(-)
 delete mode 100644 drivers/infiniband/ulp/isert/isert_proto.h

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 97ac9ffaf1eb..bdda42638629 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -29,7 +29,6 @@
 #include <target/iscsi/iscsi_transport.h>
 #include <linux/semaphore.h>
 
-#include "isert_proto.h"
 #include "ib_isert.h"
 
 #define	ISERT_MAX_CONN		8
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 03ddc3e0087f..00edd5b68ca8 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -33,6 +33,37 @@
 #define isert_err(fmt, arg...) \
 	pr_err(PFX "%s: " fmt, __func__ , ## arg)
 
+/*Constant PDU lengths calculations */
+#define ISER_HEADERS_LEN	(sizeof(struct iser_ctrl) + sizeof(struct iscsi_hdr))
+#define ISER_RECV_DATA_SEG_LEN	8192
+#define ISER_RX_PAYLOAD_SIZE	(ISER_HEADERS_LEN + ISER_RECV_DATA_SEG_LEN)
+#define ISER_RX_LOGIN_SIZE	(ISER_HEADERS_LEN + ISCSI_DEF_MAX_RECV_SEG_LEN)
+
+/* QP settings */
+/* Maximal bounds on received asynchronous PDUs */
+#define ISERT_MAX_TX_MISC_PDUS	4 /* NOOP_IN(2) , ASYNC_EVENT(2)   */
+
+#define ISERT_MAX_RX_MISC_PDUS	6 /*
+				   * NOOP_OUT(2), TEXT(1),
+				   * SCSI_TMFUNC(2), LOGOUT(1)
+				   */
+
+#define ISCSI_DEF_XMIT_CMDS_MAX 128 /* from libiscsi.h, must be power of 2 */
+
+#define ISERT_QP_MAX_RECV_DTOS	(ISCSI_DEF_XMIT_CMDS_MAX)
+
+#define ISERT_MIN_POSTED_RX	(ISCSI_DEF_XMIT_CMDS_MAX >> 2)
+
+#define ISERT_INFLIGHT_DATAOUTS	8
+
+#define ISERT_QP_MAX_REQ_DTOS	(ISCSI_DEF_XMIT_CMDS_MAX *    \
+				(1 + ISERT_INFLIGHT_DATAOUTS) + \
+				ISERT_MAX_TX_MISC_PDUS	+ \
+				ISERT_MAX_RX_MISC_PDUS)
+
+#define ISER_RX_PAD_SIZE	(ISER_RECV_DATA_SEG_LEN + 4096 - \
+		(ISER_RX_PAYLOAD_SIZE + sizeof(u64) + sizeof(struct ib_sge)))
+
 #define ISCSI_ISER_SG_TABLESIZE		256
 #define ISER_FASTREG_LI_WRID		0xffffffffffffffffULL
 #define ISER_BEACON_WRID               0xfffffffffffffffeULL
diff --git a/drivers/infiniband/ulp/isert/isert_proto.h b/drivers/infiniband/ulp/isert/isert_proto.h
deleted file mode 100644
index 4dccd313b777..000000000000
--- a/drivers/infiniband/ulp/isert/isert_proto.h
+++ /dev/null
@@ -1,47 +0,0 @@
-/* From iscsi_iser.h */
-
-struct iser_hdr {
-	u8	flags;
-	u8	rsvd[3];
-	__be32	write_stag; /* write rkey */
-	__be64	write_va;
-	__be32	read_stag;  /* read rkey */
-	__be64	read_va;
-} __packed;
-
-/*Constant PDU lengths calculations */
-#define ISER_HEADERS_LEN  (sizeof(struct iser_hdr) + sizeof(struct iscsi_hdr))
-
-#define ISER_RECV_DATA_SEG_LEN  8192
-#define ISER_RX_PAYLOAD_SIZE    (ISER_HEADERS_LEN + ISER_RECV_DATA_SEG_LEN)
-#define ISER_RX_LOGIN_SIZE      (ISER_HEADERS_LEN + ISCSI_DEF_MAX_RECV_SEG_LEN)
-
-/* QP settings */
-/* Maximal bounds on received asynchronous PDUs */
-#define ISERT_MAX_TX_MISC_PDUS	4 /* NOOP_IN(2) , ASYNC_EVENT(2)   */
-
-#define ISERT_MAX_RX_MISC_PDUS	6 /* NOOP_OUT(2), TEXT(1),         *
-				   * SCSI_TMFUNC(2), LOGOUT(1) */
-
-#define ISCSI_DEF_XMIT_CMDS_MAX 128 /* from libiscsi.h, must be power of 2 */
-
-#define ISERT_QP_MAX_RECV_DTOS	(ISCSI_DEF_XMIT_CMDS_MAX)
-
-#define ISERT_MIN_POSTED_RX	(ISCSI_DEF_XMIT_CMDS_MAX >> 2)
-
-#define ISERT_INFLIGHT_DATAOUTS	8
-
-#define ISERT_QP_MAX_REQ_DTOS	(ISCSI_DEF_XMIT_CMDS_MAX *    \
-				(1 + ISERT_INFLIGHT_DATAOUTS) + \
-				ISERT_MAX_TX_MISC_PDUS	+ \
-				ISERT_MAX_RX_MISC_PDUS)
-
-#define ISER_RX_PAD_SIZE	(ISER_RECV_DATA_SEG_LEN + 4096 - \
-		(ISER_RX_PAYLOAD_SIZE + sizeof(u64) + sizeof(struct ib_sge)))
-
-#define ISER_VER	0x10
-#define ISER_WSV	0x08
-#define ISER_RSV	0x04
-#define ISCSI_CTRL	0x10
-#define ISER_HELLO	0x20
-#define ISER_HELLORPLY	0x30
-- 
1.8.4.3

Re: [PATCH for-next 06/10] iser-target: Remove unused file iser_proto.h

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> -/* From iscsi_iser.h */
> -
> -struct iser_hdr {
> -	u8	flags;
> -	u8	rsvd[3];
> -	__be32	write_stag; /* write rkey */
> -	__be64	write_va;
> -	__be32	read_stag;  /* read rkey */
> -	__be64	read_va;
> -} __packed;
> -
> -/*Constant PDU lengths calculations */
> -#define ISER_HEADERS_LEN  (sizeof(struct iser_hdr) + sizeof(struct iscsi_hdr))

move this part of the cleanup to the preceding patch, so it can be 
reviewed and bisected into
regardless of this or any downstream patch of the series.

Or.

Re: [PATCH for-next 06/10] iser-target: Remove unused file iser_proto.h

[Sagi Grimberg]

On 17/11/2015 09:57, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> -/* From iscsi_iser.h */
>> -
>> -struct iser_hdr {
>> -    u8    flags;
>> -    u8    rsvd[3];
>> -    __be32    write_stag; /* write rkey */
>> -    __be64    write_va;
>> -    __be32    read_stag;  /* read rkey */
>> -    __be64    read_va;
>> -} __packed;
>> -
>> -/*Constant PDU lengths calculations */
>> -#define ISER_HEADERS_LEN  (sizeof(struct iser_hdr) + sizeof(struct
>> iscsi_hdr))
>
> move this part of the cleanup to the preceding patch, so it can be
> reviewed and bisected into
> regardless of this or any downstream patch of the series.

You're right... will fix..

Re: [PATCH for-next 06/10] iser-target: Remove unused file iser_proto.h

[Christoph Hellwig]

Sorry for the nitpicking, but:

> +/*Constant PDU lengths calculations */

This really needs a space before the 'C'.

> +#define ISER_HEADERS_LEN	(sizeof(struct iser_ctrl) + sizeof(struct iscsi_hdr))

Also fixing lines over 8- characters would be nice.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH for-next 06/10] iser-target: Remove unused file iser_proto.h

[Sagi Grimberg]

On 17/11/2015 11:00, Christoph Hellwig wrote:
> Sorry for the nitpicking, but:
>
>> +/*Constant PDU lengths calculations */
>
> This really needs a space before the 'C'.
>
>> +#define ISER_HEADERS_LEN	(sizeof(struct iser_ctrl) + sizeof(struct iscsi_hdr))
>
> Also fixing lines over 8- characters would be nice.

It was just a copy-paste kinda thing...

I'll fix though.
--
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 for-next 08/10] iser-target: Support the remote invalidation exception

[Sagi Grimberg]

From: Jenny Derzhavetz <jennyf@mellanox.com>

We'll use remote invalidate, according to negotiation result
during connection establishment. If the initiator declared that
it supports the remote invalidate exception then the target will
use IB_WR_SEND_WITH_INV with the correct rkey for the response.

Signed-off-by: Jenny Derzhavetz <jennyf@mellanox.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/isert/ib_isert.c | 39 +++++++++++++++++++++++++++------
 drivers/infiniband/ulp/isert/ib_isert.h |  2 ++
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/isert/ib_isert.c b/drivers/infiniband/ulp/isert/ib_isert.c
index 667238f6253f..035fd12ec7ae 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.c
+++ b/drivers/infiniband/ulp/isert/ib_isert.c
@@ -679,6 +679,25 @@ out_login_buf:
 	return ret;
 }
 
+static void
+isert_set_nego_params(struct isert_conn *isert_conn,
+		      struct rdma_conn_param *param)
+{
+	struct isert_device *device = isert_conn->device;
+
+	/* Set max inflight RDMA READ requests */
+	isert_conn->initiator_depth = min_t(u8, param->initiator_depth,
+				device->dev_attr.max_qp_init_rd_atom);
+	isert_dbg("Using initiator_depth: %u\n", isert_conn->initiator_depth);
+
+	if (param->private_data) {
+		u8 flags = *(u8 *)param->private_data;
+
+		isert_conn->snd_w_inv = !(flags & ISER_SEND_W_INV_NOT_SUP);
+		isert_info("Using remote invalidation\n");
+	}
+}
+
 static int
 isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 {
@@ -717,11 +736,7 @@ isert_connect_request(struct rdma_cm_id *cma_id, struct rdma_cm_event *event)
 	}
 	isert_conn->device = device;
 
-	/* Set max inflight RDMA READ requests */
-	isert_conn->initiator_depth = min_t(u8,
-				event->param.conn.initiator_depth,
-				device->dev_attr.max_qp_init_rd_atom);
-	isert_dbg("Using initiator_depth: %u\n", isert_conn->initiator_depth);
+	isert_set_nego_params(isert_conn, &event->param.conn);
 
 	ret = isert_conn_setup_qp(isert_conn, cma_id);
 	if (ret)
@@ -1100,7 +1115,14 @@ isert_init_send_wr(struct isert_conn *isert_conn, struct isert_cmd *isert_cmd,
 
 	isert_cmd->rdma_wr.iser_ib_op = ISER_IB_SEND;
 	send_wr->wr_id = (uintptr_t)&isert_cmd->tx_desc;
-	send_wr->opcode = IB_WR_SEND;
+
+	if (isert_conn->snd_w_inv && isert_cmd->inv_rkey) {
+		send_wr->opcode  = IB_WR_SEND_WITH_INV;
+		send_wr->ex.invalidate_rkey = isert_cmd->inv_rkey;
+	} else {
+		send_wr->opcode = IB_WR_SEND;
+	}
+
 	send_wr->sg_list = &tx_desc->tx_sg[0];
 	send_wr->num_sge = isert_cmd->tx_desc.num_sge;
 	send_wr->send_flags = IB_SEND_SIGNALED;
@@ -1489,6 +1511,7 @@ isert_rx_opcode(struct isert_conn *isert_conn, struct iser_rx_desc *rx_desc,
 		isert_cmd->read_va = read_va;
 		isert_cmd->write_stag = write_stag;
 		isert_cmd->write_va = write_va;
+		isert_cmd->inv_rkey = read_stag ? read_stag : write_stag;
 
 		ret = isert_handle_scsi_cmd(isert_conn, isert_cmd, cmd,
 					rx_desc, (unsigned char *)hdr);
@@ -3107,7 +3130,9 @@ isert_rdma_accept(struct isert_conn *isert_conn)
 	cp.rnr_retry_count = 7;
 
 	memset(&rsp_hdr, 0, sizeof(rsp_hdr));
-	rsp_hdr.flags = (ISERT_ZBVA_NOT_USED | ISERT_SEND_W_INV_NOT_USED);
+	rsp_hdr.flags = ISERT_ZBVA_NOT_USED;
+	if (!isert_conn->snd_w_inv)
+		rsp_hdr.flags = rsp_hdr.flags | ISERT_SEND_W_INV_NOT_USED;
 	cp.private_data = (void *)&rsp_hdr;
 	cp.private_data_len = sizeof(rsp_hdr);
 
diff --git a/drivers/infiniband/ulp/isert/ib_isert.h b/drivers/infiniband/ulp/isert/ib_isert.h
index 00edd5b68ca8..b23086d3d08b 100644
--- a/drivers/infiniband/ulp/isert/ib_isert.h
+++ b/drivers/infiniband/ulp/isert/ib_isert.h
@@ -162,6 +162,7 @@ struct isert_cmd {
 	uint32_t		write_stag;
 	uint64_t		read_va;
 	uint64_t		write_va;
+	uint32_t		inv_rkey;
 	u64			pdu_buf_dma;
 	u32			pdu_buf_len;
 	struct isert_conn	*conn;
@@ -209,6 +210,7 @@ struct isert_conn {
 	struct work_struct	release_work;
 	struct ib_recv_wr       beacon;
 	bool                    logout_posted;
+	bool                    snd_w_inv;
 };
 
 #define ISERT_MAX_CQ 64
-- 
1.8.4.3

Re: [PATCH for-next 08/10] iser-target: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> We'll use remote invalidate, according to negotiation result
> during connection establishment. If the initiator declared that
> it supports the remote invalidate exception then the target will
> use IB_WR_SEND_WITH_INV with the correct rkey for the response.

same comment as the one I posted for the initiator patch, check for
IB_DEVICE_MEM_MGT_EXTENSIONS

Or.
--
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 for-next 08/10] iser-target: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:09, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> We'll use remote invalidate, according to negotiation result
>> during connection establishment. If the initiator declared that
>> it supports the remote invalidate exception then the target will
>> use IB_WR_SEND_WITH_INV with the correct rkey for the response.
>
> same comment as the one I posted for the initiator patch, check for
> IB_DEVICE_MEM_MGT_EXTENSIONS

SEND_WITH_INV doesn't have anything to do with
IB_DEVICE_MEM_MGT_EXTENSIONS does it? What is the relations between
the device capability to do FRWR and invalidating a remote rkey?
--
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 for-next 08/10] iser-target: Support the remote invalidation exception

[Or Gerlitz]

On 11/17/2015 11:50 AM, Sagi Grimberg wrote:
>> same comment as the one I posted for the initiator patch, check for
>> IB_DEVICE_MEM_MGT_EXTENSIONS
>
> SEND_WITH_INV doesn't have anything to do with 
> IB_DEVICE_MEM_MGT_EXTENSIONS does it? What is the relations between
> the device capability to do FRWR and invalidating a remote rkey? 

I am not sure.

I expect you as the author and leader for this series to dig into IBTA 
BMME annex and come with answers...

Or.
--
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 for-next 08/10] iser-target: Support the remote invalidation exception

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 11:50:15AM +0200, Sagi Grimberg wrote:
> 
> 
> On 17/11/2015 10:09, Or Gerlitz wrote:
> >On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> >>We'll use remote invalidate, according to negotiation result
> >>during connection establishment. If the initiator declared that
> >>it supports the remote invalidate exception then the target will
> >>use IB_WR_SEND_WITH_INV with the correct rkey for the response.
> >
> >same comment as the one I posted for the initiator patch, check for
> >IB_DEVICE_MEM_MGT_EXTENSIONS
> 
> SEND_WITH_INV doesn't have anything to do with
> IB_DEVICE_MEM_MGT_EXTENSIONS does it? What is the relations between
> the device capability to do FRWR and invalidating a remote rkey?

Both FRs and SEND_WITH_INV are part of the base memory management
extensions and also required for iWarp.
--
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 for-next 08/10] iser-target: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 12:14, Christoph Hellwig wrote:
> On Tue, Nov 17, 2015 at 11:50:15AM +0200, Sagi Grimberg wrote:
>>
>>
>> On 17/11/2015 10:09, Or Gerlitz wrote:
>>> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>>>> We'll use remote invalidate, according to negotiation result
>>>> during connection establishment. If the initiator declared that
>>>> it supports the remote invalidate exception then the target will
>>>> use IB_WR_SEND_WITH_INV with the correct rkey for the response.
>>>
>>> same comment as the one I posted for the initiator patch, check for
>>> IB_DEVICE_MEM_MGT_EXTENSIONS
>>
>> SEND_WITH_INV doesn't have anything to do with
>> IB_DEVICE_MEM_MGT_EXTENSIONS does it? What is the relations between
>> the device capability to do FRWR and invalidating a remote rkey?
>
> Both FRs and SEND_WITH_INV are part of the base memory management
> extensions and also required for iWarp.

Thanks for clarifying. I'll add the check.

Re: [PATCH for-next 08/10] iser-target: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>   	struct isert_conn	*conn;
> @@ -209,6 +210,7 @@ struct isert_conn {
>   	struct work_struct	release_work;
>   	struct ib_recv_wr       beacon;
>   	bool                    logout_posted;
> +	bool                    snd_w_inv;
>   };

We've gone into this aspect few times in the past... my preference
is to use bit flags so the code would look like

conn->flags |= ISER_CONN_SEND_W_INV

or

if (conn->flags & ISER_CONN_LOGOUT_POSTED)

And you didn't want to go that way... but you did used
bit-fields in other areas of the driver, right?

Lets use some sort of BF-ing (bit fields or bit flags) all over the place
and not introduce new booleans everywhere we go.

Doing this over and over makes our fast path structures to grow and
maybe start crossing cache-lines (did you check that?)

Or.

[PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

From: Jenny Derzhavetz <jennyf@mellanox.com>

Declare that we support remote invalidation and be able to detect
the invalidated rkey so we won't invalidate it locally. The spec
mandates that we must not rely on the taget remote invalidate our
rkey so we must check it upon a receive (scsi response) completion.

Signed-off-by: Jenny Derzhavetz <jennyf@mellanox.com>
Signed-off-by: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/iser/iscsi_iser.h     |  3 +-
 drivers/infiniband/ulp/iser/iser_initiator.c | 55 +++++++++++++++++++++++++++-
 drivers/infiniband/ulp/iser/iser_verbs.c     | 19 +++++++---
 3 files changed, 70 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.h b/drivers/infiniband/ulp/iser/iscsi_iser.h
index 096d5234bbea..2e0a24ba18ed 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.h
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.h
@@ -519,6 +519,7 @@ struct iser_conn {
 	u32                          num_rx_descs;
 	unsigned short               scsi_sg_tablesize;
 	unsigned int                 scsi_max_sectors;
+	bool			     snd_w_inv;
 };
 
 /**
@@ -603,7 +604,7 @@ int iser_conn_terminate(struct iser_conn *iser_conn);
 void iser_release_work(struct work_struct *work);
 
 void iser_rcv_completion(struct iser_rx_desc *desc,
-			 unsigned long dto_xfer_len,
+			 struct ib_wc *wc,
 			 struct ib_conn *ib_conn);
 
 void iser_snd_completion(struct iser_tx_desc *desc,
diff --git a/drivers/infiniband/ulp/iser/iser_initiator.c b/drivers/infiniband/ulp/iser/iser_initiator.c
index 6a968e350c14..df6a4b70ffc9 100644
--- a/drivers/infiniband/ulp/iser/iser_initiator.c
+++ b/drivers/infiniband/ulp/iser/iser_initiator.c
@@ -563,11 +563,57 @@ send_control_error:
 	return err;
 }
 
+static int
+iser_check_remote_inv(struct iser_conn *iser_conn,
+		      struct ib_wc *wc,
+		      struct iscsi_hdr *hdr)
+{
+	if (wc->wc_flags & IB_WC_WITH_INVALIDATE) {
+		struct iscsi_task *task;
+		struct iscsi_iser_task *iser_task;
+		u32 rkey = wc->ex.invalidate_rkey;
+
+		iser_dbg("conn %p: remote invalidation\n", iser_conn);
+		if (unlikely(!iser_conn->snd_w_inv)) {
+			iser_err("conn %p: unexepected remote invalidation,"
+				 "terminating connection\n", iser_conn);
+			return -EPROTO;
+		}
+
+		task = iscsi_itt_to_ctask(iser_conn->iscsi_conn, hdr->itt);
+		if (likely(task)) {
+			struct iser_mem_reg *reg;
+			struct iser_fr_desc *desc;
+
+			iser_task = task->dd_data;
+			if (iser_task->dir[ISER_DIR_IN])
+				reg = &iser_task->rdma_reg[ISER_DIR_IN];
+			else
+				reg = &iser_task->rdma_reg[ISER_DIR_OUT];
+			desc = reg->mem_h;
+
+			if (likely(rkey == desc->rsc.mr->rkey)) {
+				desc->rsc.mr_valid = 0;
+			} else if (likely(rkey == desc->pi_ctx->sig_mr->rkey)) {
+				desc->pi_ctx->sig_mr_valid = 0;
+			} else {
+				iser_err("invalidation of unknown rkey=0x%x\n", rkey);
+				return -EINVAL;
+			}
+		} else {
+			iser_err("failed to get task for itt=%d\n", hdr->itt);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
 /**
  * iser_rcv_dto_completion - recv DTO completion
  */
 void iser_rcv_completion(struct iser_rx_desc *rx_desc,
-			 unsigned long rx_xfer_len,
+			 struct ib_wc *wc,
 			 struct ib_conn *ib_conn)
 {
 	struct iser_conn *iser_conn = container_of(ib_conn, struct iser_conn,
@@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
 	struct iscsi_hdr *hdr;
 	u64 rx_dma;
 	int rx_buflen, outstanding, count, err;
+	unsigned long rx_xfer_len = wc->byte_len;
 
 	/* differentiate between login to all other PDUs */
 	if ((char *)rx_desc == iser_conn->login_resp_buf) {
@@ -593,6 +640,12 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
 	iser_dbg("op 0x%x itt 0x%x dlen %d\n", hdr->opcode,
 			hdr->itt, (int)(rx_xfer_len - ISER_HEADERS_LEN));
 
+	if (iser_check_remote_inv(iser_conn, wc, hdr)) {
+		iscsi_conn_failure(iser_conn->iscsi_conn,
+				   ISCSI_ERR_CONN_FAILED);
+		return;
+	}
+
 	iscsi_iser_recv(iser_conn->iscsi_conn, hdr, rx_desc->data,
 			rx_xfer_len - ISER_HEADERS_LEN);
 
diff --git a/drivers/infiniband/ulp/iser/iser_verbs.c b/drivers/infiniband/ulp/iser/iser_verbs.c
index 74161a566852..5dd7cbd8e2e2 100644
--- a/drivers/infiniband/ulp/iser/iser_verbs.c
+++ b/drivers/infiniband/ulp/iser/iser_verbs.c
@@ -847,7 +847,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
 	conn_param.rnr_retry_count     = 6;
 
 	memset(&req_hdr, 0, sizeof(req_hdr));
-	req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
+	req_hdr.flags = ISER_ZBVA_NOT_SUP;
 	conn_param.private_data	= (void *)&req_hdr;
 	conn_param.private_data_len = sizeof(struct iser_cm_hdr);
 
@@ -862,7 +862,8 @@ failure:
 	iser_connect_error(cma_id);
 }
 
-static void iser_connected_handler(struct rdma_cm_id *cma_id)
+static void iser_connected_handler(struct rdma_cm_id *cma_id,
+				   const void *private_data)
 {
 	struct iser_conn *iser_conn;
 	struct ib_qp_attr attr;
@@ -876,6 +877,15 @@ static void iser_connected_handler(struct rdma_cm_id *cma_id)
 	(void)ib_query_qp(cma_id->qp, &attr, ~0, &init_attr);
 	iser_info("remote qpn:%x my qpn:%x\n", attr.dest_qp_num, cma_id->qp->qp_num);
 
+	if (private_data) {
+		u8 flags = *(u8 *)private_data;
+
+		iser_conn->snd_w_inv = !(flags & ISER_SEND_W_INV_NOT_SUP);
+	}
+
+	iser_info("conn %p: negotiated %s invalidation\n",
+		  iser_conn, iser_conn->snd_w_inv ? "remote" : "local");
+
 	iser_conn->state = ISER_CONN_UP;
 	complete(&iser_conn->up_completion);
 }
@@ -927,7 +937,7 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
 		iser_route_handler(cma_id);
 		break;
 	case RDMA_CM_EVENT_ESTABLISHED:
-		iser_connected_handler(cma_id);
+		iser_connected_handler(cma_id, event->param.conn.private_data);
 		break;
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
@@ -1205,8 +1215,7 @@ static void iser_handle_wc(struct ib_wc *wc)
 	if (likely(wc->status == IB_WC_SUCCESS)) {
 		if (wc->opcode == IB_WC_RECV) {
 			rx_desc = (struct iser_rx_desc *)(uintptr_t)wc->wr_id;
-			iser_rcv_completion(rx_desc, wc->byte_len,
-					    ib_conn);
+			iser_rcv_completion(rx_desc, wc, ib_conn);
 		} else
 		if (wc->opcode == IB_WC_SEND) {
 			tx_desc = (struct iser_tx_desc *)(uintptr_t)wc->wr_id;
-- 
1.8.4.3

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>   void iser_rcv_completion(struct iser_rx_desc *rx_desc,
> -			 unsigned long rx_xfer_len,
> +			 struct ib_wc *wc,
>   			 struct ib_conn *ib_conn)
>   {
>   	struct iser_conn *iser_conn = container_of(ib_conn, struct iser_conn,
> @@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc *rx_desc,
>   	struct iscsi_hdr *hdr;
>   	u64 rx_dma;
>   	int rx_buflen, outstanding, count, err;
> +	unsigned long rx_xfer_len = wc->byte_len;

unrelated small enhancement which has nothing to do with this patch, 
remove it from here
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:03, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>>   void iser_rcv_completion(struct iser_rx_desc *rx_desc,
>> -             unsigned long rx_xfer_len,
>> +             struct ib_wc *wc,
>>                struct ib_conn *ib_conn)
>>   {
>>       struct iser_conn *iser_conn = container_of(ib_conn, struct
>> iser_conn,
>> @@ -575,6 +621,7 @@ void iser_rcv_completion(struct iser_rx_desc
>> *rx_desc,
>>       struct iscsi_hdr *hdr;
>>       u64 rx_dma;
>>       int rx_buflen, outstanding, count, err;
>> +    unsigned long rx_xfer_len = wc->byte_len;
>
> unrelated small enhancement which has nothing to do with this patch,
> remove it from here

It is related, we pass wc to check the invalidate stuff down the road...
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> +			if (iser_task->dir[ISER_DIR_IN])
> +				reg = &iser_task->rdma_reg[ISER_DIR_IN];
> +			else
> +				reg = &iser_task->rdma_reg[ISER_DIR_OUT];

and what happens with bidirectional commands?!
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:05, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> +            if (iser_task->dir[ISER_DIR_IN])
>> +                reg = &iser_task->rdma_reg[ISER_DIR_IN];
>> +            else
>> +                reg = &iser_task->rdma_reg[ISER_DIR_OUT];
>
> and what happens with bidirectional commands?!

It won't work, iSER lacks support for bidirectional commands for as
long as I'm involved with it and until we either decide that we care
enough to implement it both in the target and initiator sides or some
array with iser bidi support shows up I don't know how much its worth
our attention.

Thoughts?

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/17/2015 11:53 AM, Sagi Grimberg wrote:
> [...] iSER lacks support for bidirectional commands for as long as I'm 
> involved with it 


Why? we don't support and when did we broke it after the initial 2.6.18 
upstreaming of the driver

Also, do we refuse to queuecommand a bidi? where?

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Tue, Nov 17, 2015 at 12:04:03PM +0200, Or Gerlitz wrote:
> Also, do we refuse to queuecommand a bidi? where?

Or, can you please do the basic research first?  Thanks! (Hint: check
how few drivers support bidi commands, and how it's enabled)

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

> Why? we don't support and when did we broke it after the initial 2.6.18
> upstreaming of the driver
>
> Also, do we refuse to queuecommand a bidi? where?

Or, bidirectional support is not assumed and needs to be actively set
as a request queue flag (see iscsi_sw_tcp_slave_alloc()). AFAICT iser
never exposed support for bidirectional commands.

Did things change from back in 2.6.18 that we ever carried bidi
commands over iser?
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Tue, Nov 17, 2015 at 12:26 PM, Sagi Grimberg
<sagig-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org> wrote:
>
>> Why? we don't support and when did we broke it after the initial 2.6.18
>> upstreaming of the driver
>>
>> Also, do we refuse to queuecommand a bidi? where?
>
>
> Or, bidirectional support is not assumed and needs to be actively set
> as a request queue flag (see iscsi_sw_tcp_slave_alloc()).

> AFAICT iser never exposed support for bidirectional commands.

AFAIR, in the past there weren't explicit means to do that.

What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?

> Did things change from back in 2.6.18 that we ever carried bidi
> commands over iser?

AFAIK, Pete Wyckoff had iser working with BIDI commands for few object
storage projects.

It's been a long time, but I was totally unaware that we don't publish
the whatever means
needed by upper layers.

I copied some 2010 email address of his here...

Or.
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

> AFAIR, in the past there weren't explicit means to do that.
>
> What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?

In theory, nothing.
In practice, iser is missing customer demands, iser targets that
support bidi and testing...

If someone cared enough about it then I assume we'd hear about it by
now...

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Wed, Nov 18, 2015 at 1:38 PM, Sagi Grimberg <sagig@dev.mellanox.co.il> wrote:
>> AFAIR, in the past there weren't explicit means to do that.
>> What's makes iscsi tcp eligible to support bidi's that we don't have @ iser?

> In theory, nothing. In practice, iser is missing customer demands, iser targets that
> support bidi and testing...

> If someone cared enough about it then I assume we'd hear about it by now...

Sagi, it works in TGT and AFAIR with the initiator too.

Looking on this paper of Pete Wyckoff  [1] I see that he says that
few changes to the initiator were needed, not sure which.

Or.


[1] http://pw.padd.com/papers/dalessandro-iser-snapi07.pdf

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Wed, Nov 18, 2015 at 03:33:01PM +0200, Or Gerlitz wrote:
> Sagi, it works in TGT and AFAIR with the initiator too.
> 
> Looking on this paper of Pete Wyckoff  [1] I see that he says that
> few changes to the initiator were needed, not sure which.

Or, can you please stop it?

Fortunately neither the iSER target or initiator ever support the
nightmare called bidi commands, and I'be happy o kill of that cruft
entirely if we could.

Did you buy shares in a defunct T10 OSD vendor or why do you even care?

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Wed, Nov 18, 2015 at 3:52 PM, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> Fortunately neither the iSER target or initiator ever support the
> nightmare called bidi commands,

I honestly don't know why you call it nightmare nor what make you
make that strong assertion. I checked with Alex N. the TGT iser
transport author and he confirmed to me that bidi's worked on TGT,
as for the Linux upstream initiator, I thought that was the case too, but
I never saw it my eyes, and Pete's paper states he had to change the code,
so on that I can't be sure.

>  and I'be happy o kill of that cruft entirely if we could.

that's beyond the scope of this thread, I guess..
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Wed, Nov 18, 2015 at 03:58:48PM +0200, Or Gerlitz wrote:
> > Fortunately neither the iSER target or initiator ever support the
> > nightmare called bidi commands,
> 
> I honestly don't know why you call it nightmare nor what make you
> make that strong assertion. 

Beause I actually had to deal with block layer code implementing the
bidi semantics.

> I checked with Alex N. the TGT iser
> transport author and he confirmed to me that bidi's worked on TGT,
> as for the Linux upstream initiator, I thought that was the case too, but
> I never saw it my eyes, and Pete's paper states he had to change the code,
> so on that I can't be sure.

No need to trust words, we have git.  There was absolutely nothing
relating to bidi in either the initial iSER checking nor in the
changelogs since, and neither has the initial BIDI checking touched
iSER.

> that's beyond the scope of this thread, I guess..

As is iSER BIDI suppport.  Let's thank Sagi for doing a very important
performance feature for iSER instead of having this endless discussion
about a pointless fringe feature!
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/18/2015 4:10 PM, Christoph Hellwig wrote:
> There was absolutely nothing relating to bidi in either the initial iSER checking

This is wrong assertion.

Look on the code throughout the iser path done from iser_send_command, 
we allowed the command associated with the
iscsi task to be IN, OUT, both or none, when we do all the dma-mapping, 
memory registrations and such for either of the
needed directions and same on the completion path.

Sagi, do we still do IN, OUT, both or none? if not, where this stopped 
to be supported? how large would be the fix?

Or.


[1] maybe start with 2.6.20,  I guess we improved later... till the 
point where things were potentially started
to be caught down, as I realized now that Sagi wasn't fully aligned on 
that aspect of the driver
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

> Sagi, do we still do IN, OUT, both or none? if not, where this stopped
> to be supported? how large would be the fix?

Or, it's hard for me to say where exactly it stopped being supported
because I've never tested it and I'm not convinced it was ever
supported.

I'm exhausted with this discussion so I'll change the tiny condition
and bidi will remain unsupported until we'll decide we want to get it
working.
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Christoph Hellwig]

On Thu, Nov 19, 2015 at 09:12:20AM +0200, Or Gerlitz wrote:
> This is wrong assertion.
> 
> Look on the code throughout the iser path done from iser_send_command, we
> allowed the command associated with the
> iscsi task to be IN, OUT, both or none, when we do all the dma-mapping,
> memory registrations and such for either of the
> needed directions and same on the completion path.

Internal code structure is one thing, ever supporting bidi is another
one.  Without QUEUE_FLAG_BIDI a driver has never supported bidirectional
requests.  And iSER never had that flag set in mainline.  So even if you
spent of time supporting bidi in iSER it never was supported and all
that great support was entirely untested the last 10 years.
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On Thu, Nov 19, 2015 at 12:01 PM, Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> wrote:

> Internal code structure is one thing, ever supporting bidi is another
> one.  Without QUEUE_FLAG_BIDI a driver has never supported bidirectional
> requests.  And iSER never had that flag set in mainline.  So even if you
> spent of time supporting bidi in iSER it never was supported and all
> that great support was entirely untested the last 10 years.

Christoph,

To make it clear iser's role is very well defined and strict w.r.t
supporting bidis, we should

on IO submission path

1. dma map the SC properly in, out, both or none
2. memory register what's needed
3. put in the iser header 1,2 or 0 keys

on IO completion path

4. set a pointer to the iscsi response header etc
5. memory unregister what's needed
6. dma unmap properly
7. call up to the iscsi/scsi stack with that pointer

anything else that I missed?

AFAIK steps 1..7 were supported ok in the past, probably now not, we
should fix at some point



Or.
--
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 for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

> Sagi, it works in TGT and AFAIR with the initiator too.
>
> Looking on this paper of Pete Wyckoff  [1] I see that he says that
> few changes to the initiator were needed, not sure which.

I see. I wasn't aware that TGT supports bidi. However, AFAICT the
initiator support was never fully introduced upstream nor in our mlnx
backports (perhaps in an off-tree implementation). As I see it, bidi
functionality, is broken for a long time (if it was ever supported).

So I'd suggest we (you and me Or) look at bidi separately and try to
find out if someone wants it (not for academic research).

Would you mind if we don't include bidi considerations in this patchset?

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: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/18/2015 4:16 PM, Sagi Grimberg wrote:
>> Sagi, it works in TGT and AFAIR with the initiator too.
>>
>> Looking on this paper of Pete Wyckoff  [1] I see that he says that
>> few changes to the initiator were needed, not sure which.
>
> I see. I wasn't aware that TGT supports bidi. However, AFAICT the
> initiator support was never fully introduced upstream nor in our mlnx
> backports (perhaps in an off-tree implementation). As I see it, bidi
> functionality, is broken for a long time (if it was ever supported).

Sounds like we weren't communicating enough while reviewing the patches
since you joined as maintainer... lets improve.

> So I'd suggest we (you and me Or) look at bidi separately and try to
> find out if someone wants it (not for academic research).

I didn't follow on the comment, what's wrong with having the upstream 
kernel serve
for academics? features used for academics today might turn to 
production tomorrow.
It's not that we're writing a whole new driver for that, there's one 
piece in our
design/code which is good for that purpose, this is perfectly fine.


> Would you mind if we don't include bidi considerations in this patchset? 

You should not further break it, whatever is still there should remain.  
As for breakages
that were introduced over the last few cycles, we should think that to do.

Or.

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Or Gerlitz]

On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
> @@ -847,7 +847,7 @@ static void iser_route_handler(struct rdma_cm_id *cma_id)
>   	conn_param.rnr_retry_count     = 6;
>   
>   	memset(&req_hdr, 0, sizeof(req_hdr));
> -	req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
> +	req_hdr.flags = ISER_ZBVA_NOT_SUP;

isn't there a property of the **local** device we need to check before 
advertizing that
to the target? to be on the safe side, I would do  that only over 
devices that support
IB_DEVICE_MEM_MGT_EXTENSIONS, as non-local invalidations are part of the 
BMME ext
of IBTA, right?

Re: [PATCH for-next 10/10] IB/iser: Support the remote invalidation exception

[Sagi Grimberg]

On 17/11/2015 10:08, Or Gerlitz wrote:
> On 11/16/2015 6:37 PM, Sagi Grimberg wrote:
>> --- a/drivers/infiniband/ulp/iser/iser_verbs.c
>> +++ b/drivers/infiniband/ulp/iser/iser_verbs.c
>> @@ -847,7 +847,7 @@ static void iser_route_handler(struct rdma_cm_id
>> *cma_id)
>>       conn_param.rnr_retry_count     = 6;
>>       memset(&req_hdr, 0, sizeof(req_hdr));
>> -    req_hdr.flags = (ISER_ZBVA_NOT_SUP | ISER_SEND_W_INV_NOT_SUP);
>> +    req_hdr.flags = ISER_ZBVA_NOT_SUP;
>
> isn't there a property of the **local** device we need to check before
> advertizing that
> to the target? to be on the safe side, I would do  that only over
> devices that support
> IB_DEVICE_MEM_MGT_EXTENSIONS, as non-local invalidations are part of the
> BMME ext
> of IBTA, right?

This was dependent on using fastreg (which depends on
IB_DEVICE_MEM_MGT_EXTENSIONS) at some point but it must have got lost
at some point..

Will fix. Thanks!
--
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 for-next 00/10] iSER support for remote invalidate

[Christoph Hellwig]

On Mon, Nov 16, 2015 at 06:37:31PM +0200, Sagi Grimberg wrote:
> This patchset adds remote invalidation support to iser initiator and
> target. The support negotiation for this feature is based on IBTA
> annex 12 "Support for iSCSI Extensions for RDMA" carried in rdma_cm
> private data.

Oh, okay - this answers my question where the private data comes from,
might be worth documenting it in the header what standard each bit
comes from.