Skip to contentSkip to conversation

Chelsio iSCSI target offload driver

PATCH Superseded Varun Prakash 2 participants 36 messages Apr 9, 2016 – Apr 13, 2016

Subsystems (4)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

v2 cover Chelsio iSCSI target offload driver

This is v2 patch series for Chelsio iSCSI target offload driver(cxgbit.ko).

cxgbit.ko registers with iSCSI target transport and offloads multiple CPU intensive tasks to Chelsio T5 adapters.

T5 adapters has following offload features for
iSCSI
-TCP/IP offload
-iSCSI PDU recovery by reassembling TCP segments.
-Header and Data Digest offload.
-iSCSI segmentation offload(ISO).
-Direct Data Placement(DDP).

-v2
- added hw offload transport type (Nicholas).
- added PDU alloc, free, xmit callbacks (Nicholas).
- created single patch for all cxgbit.ko files (Nicholas).
- merged definition and usage of int (*iscsit_validate_params)() in a single patch (Christoph).
- merged definition and usage of void (*iscsit_release_cmd)() in a single patch (Christoph).
- renamed void (*iscsit_rx_pdu)() to void (*iscsit_get_rx_pdu)() (Sagi).
- added dependency on INET in Kconfig (Arnd).
- added check for NULL sg in cxgbit_set_one_ppod() (Dan Carpenter).

Please review.

Thanks

Varun Prakash (16):
iscsi-target: add callback to alloc and free PDU
iscsi-target: add int (*iscsit_xmit_pdu)()
iscsi-target: add int (*iscsit_xmit_datain_pdu)()
iscsi-target: add void (*iscsit_release_cmd)()
iscsi-target: add void (*iscsit_get_rx_pdu)()
iscsi-target: split iscsi_target_rx_thread()
iscsi-target: add int (*iscsit_validate_params)()
iscsi-target: add void (*iscsit_get_r2t_ttt)()
iscsi-target: move iscsit_thread_check_cpumask()
iscsi-target: use conn->network_transport in text rsp
iscsi-target: add new offload transport type
iscsi-target: clear tx_thread_active
iscsi-target: call complete on conn_logout_comp
iscsi-target: export symbols
iscsi-target: fix seq_end_offset calculation
cxgbit: add files for cxgbit.ko

 drivers/infiniband/ulp/isert/ib_isert.c           |   10 +
 drivers/target/iscsi/Kconfig                      |    2 +
 drivers/target/iscsi/Makefile                     |    1 +
 drivers/target/iscsi/cxgbit/Kconfig               |    7 +
 drivers/target/iscsi/cxgbit/Makefile              |    6 +
 drivers/target/iscsi/cxgbit/cxgbit.h              |  358 ++++
 drivers/target/iscsi/cxgbit/cxgbit_cm.c           | 2086 +++++++++++++++++++++
 drivers/target/iscsi/cxgbit/cxgbit_ddp.c          |  325 ++++
 drivers/target/iscsi/cxgbit/cxgbit_lro.h          |   72 +
 drivers/target/iscsi/cxgbit/cxgbit_main.c         |  703 +++++++
 drivers/target/iscsi/cxgbit/cxgbit_target.c       | 1601 ++++++++++++++++
 drivers/target/iscsi/iscsi_target.c               |  740 +++-----
 drivers/target/iscsi/iscsi_target_configfs.c      |   76 +
 drivers/target/iscsi/iscsi_target_datain_values.c |    1 +
 drivers/target/iscsi/iscsi_target_erl0.c          |    6 +-
 drivers/target/iscsi/iscsi_target_login.c         |   13 +-
 drivers/target/iscsi/iscsi_target_nego.c          |    1 +
 drivers/target/iscsi/iscsi_target_parameters.c    |    1 +
 drivers/target/iscsi/iscsi_target_util.c          |    5 +
 include/target/iscsi/iscsi_target_core.h          |   27 +
 include/target/iscsi/iscsi_transport.h            |   45 +
 21 files changed, 5648 insertions(+), 438 deletions(-)
 create mode 100644 drivers/target/iscsi/cxgbit/Kconfig
 create mode 100644 drivers/target/iscsi/cxgbit/Makefile
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit.h
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_cm.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_ddp.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_lro.h
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_main.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_target.c
Signature · 1 line
2.0.2
#1 lore raw

PATCH v2 1/16 iscsi-target: add callback to alloc and free PDU

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().

---
 drivers/target/iscsi/iscsi_target.c    | 76 ++++++++++++++++++++++++++++------
 include/target/iscsi/iscsi_transport.h |  2 +
 2 files changed, 65 insertions(+), 13 deletions(-)
drivers/target/iscsi/iscsi_target.c +63 -13
index 961202f..fdb33ba 100644
@@ -499,6 +499,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 	__iscsit_free_cmd(cmd, scsi_cmd, true);
 }
 
+static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+{
+	return cmd->pdu;
+}
+
 static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 {
 	return TARGET_PROT_NORMAL;
@@ -519,6 +524,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_queue_data_in	= iscsit_queue_rsp,
 	.iscsit_queue_status	= iscsit_queue_rsp,
 	.iscsit_aborted_task	= iscsit_aborted_task,
+	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
@@ -2537,7 +2543,10 @@ static int iscsit_send_conn_drop_async_message(
 	cmd->tx_size = ISCSI_HDR_LEN;
 	cmd->iscsi_opcode = ISCSI_OP_ASYNC_EVENT;
 
-	hdr			= (struct iscsi_async *) cmd->pdu;
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	hdr->opcode		= ISCSI_OP_ASYNC_EVENT;
 	hdr->flags		= ISCSI_FLAG_CMD_FINAL;
 	cmd->init_task_tag	= RESERVED_ITT;
@@ -2630,7 +2639,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
 static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
-	struct iscsi_data_rsp *hdr = (struct iscsi_data_rsp *)&cmd->pdu[0];
+	struct iscsi_data_rsp *hdr;
 	struct iscsi_datain datain;
 	struct iscsi_datain_req *dr;
 	struct kvec *iov;
@@ -2675,6 +2684,10 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 			set_statsn = true;
 	}
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_datain_pdu(cmd, conn, &datain, hdr, set_statsn);
 
 	iov = &cmd->iov_data[0];
@@ -2843,13 +2856,20 @@ EXPORT_SYMBOL(iscsit_build_logout_rsp);
 static int
 iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
+	struct iscsi_logout_rsp *hdr;
 	struct kvec *iov;
 	int niov = 0, tx_size, rc;
 
-	rc = iscsit_build_logout_rsp(cmd, conn,
-			(struct iscsi_logout_rsp *)&cmd->pdu[0]);
-	if (rc < 0)
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
+	rc = iscsit_build_logout_rsp(cmd, conn, hdr);
+	if (rc < 0) {
+		if (conn->conn_transport->iscsit_free_pdu)
+			conn->conn_transport->iscsit_free_pdu(conn, cmd);
 		return rc;
+	}
 
 	tx_size = ISCSI_HDR_LEN;
 	iov = &cmd->iov_misc[0];
@@ -2909,9 +2929,13 @@ static int iscsit_send_unsolicited_nopin(
 	struct iscsi_conn *conn,
 	int want_response)
 {
-	struct iscsi_nopin *hdr = (struct iscsi_nopin *)&cmd->pdu[0];
+	struct iscsi_nopin *hdr;
 	int tx_size = ISCSI_HDR_LEN, ret;
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_nopin_rsp(cmd, conn, hdr, false);
 
 	if (conn->conn_ops->HeaderDigest) {
@@ -2950,11 +2974,15 @@ static int iscsit_send_unsolicited_nopin(
 static int
 iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
-	struct iscsi_nopin *hdr = (struct iscsi_nopin *)&cmd->pdu[0];
+	struct iscsi_nopin *hdr;
 	struct kvec *iov;
 	u32 padding = 0;
 	int niov = 0, tx_size;
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_nopin_rsp(cmd, conn, hdr, true);
 
 	tx_size = ISCSI_HDR_LEN;
@@ -3028,7 +3056,10 @@ static int iscsit_send_r2t(
 	if (!r2t)
 		return -1;
 
-	hdr			= (struct iscsi_r2t_rsp *) cmd->pdu;
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	memset(hdr, 0, ISCSI_HDR_LEN);
 	hdr->opcode		= ISCSI_OP_R2T;
 	hdr->flags		|= ISCSI_FLAG_CMD_FINAL;
@@ -3203,12 +3234,16 @@ EXPORT_SYMBOL(iscsit_build_rsp_pdu);
 
 static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
-	struct iscsi_scsi_rsp *hdr = (struct iscsi_scsi_rsp *)&cmd->pdu[0];
+	struct iscsi_scsi_rsp *hdr;
 	struct kvec *iov;
 	u32 padding = 0, tx_size = 0;
 	int iov_count = 0;
 	bool inc_stat_sn = (cmd->i_state == ISTATE_SEND_STATUS);
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_rsp_pdu(cmd, conn, inc_stat_sn, hdr);
 
 	iov = &cmd->iov_misc[0];
@@ -3322,9 +3357,13 @@ EXPORT_SYMBOL(iscsit_build_task_mgt_rsp);
 static int
 iscsit_send_task_mgt_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
-	struct iscsi_tm_rsp *hdr = (struct iscsi_tm_rsp *)&cmd->pdu[0];
+	struct iscsi_tm_rsp *hdr;
 	u32 tx_size = 0;
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_task_mgt_rsp(cmd, conn, hdr);
 
 	cmd->iov_misc[0].iov_base	= cmd->pdu;
@@ -3582,14 +3621,21 @@ static int iscsit_send_text_rsp(
 	struct iscsi_cmd *cmd,
 	struct iscsi_conn *conn)
 {
-	struct iscsi_text_rsp *hdr = (struct iscsi_text_rsp *)cmd->pdu;
+	struct iscsi_text_rsp *hdr;
 	struct kvec *iov;
 	u32 tx_size = 0;
 	int text_length, iov_count = 0, rc;
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	rc = iscsit_build_text_rsp(cmd, conn, hdr, ISCSI_TCP);
-	if (rc < 0)
+	if (rc < 0) {
+		if (conn->conn_transport->iscsit_free_pdu)
+			conn->conn_transport->iscsit_free_pdu(conn, cmd);
 		return rc;
+	}
 
 	text_length = rc;
 	iov = &cmd->iov_misc[0];
@@ -3653,10 +3699,14 @@ static int iscsit_send_reject(
 	struct iscsi_cmd *cmd,
 	struct iscsi_conn *conn)
 {
-	struct iscsi_reject *hdr = (struct iscsi_reject *)&cmd->pdu[0];
+	struct iscsi_reject *hdr;
 	struct kvec *iov;
 	u32 iov_count = 0, tx_size;
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_reject(cmd, conn, hdr);
 
 	iov = &cmd->iov_misc[0];
include/target/iscsi/iscsi_transport.h +2 -0
index 90e37fa..526b715 100644
@@ -22,6 +22,8 @@ struct iscsit_transport {
 	int (*iscsit_queue_data_in)(struct iscsi_conn *, struct iscsi_cmd *);
 	int (*iscsit_queue_status)(struct iscsi_conn *, struct iscsi_cmd *);
 	void (*iscsit_aborted_task)(struct iscsi_conn *, struct iscsi_cmd *);
+	void *(*iscsit_alloc_pdu)(struct iscsi_conn *, struct iscsi_cmd *);
+	void (*iscsit_free_pdu)(struct iscsi_conn *, struct iscsi_cmd *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer
#2 lore raw

Re: [PATCH v2 01/16] iscsi-target: add callback to alloc and free PDU

Quoted 10 lines · Varun Prakash

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().

Can you please explain why are you adding two different callouts? Who (Chelsio T5) will need it, and why they can't use the in-cmd pdu?

Quoted 72 lines
---
  drivers/target/iscsi/iscsi_target.c    | 76 ++++++++++++++++++++++++++++------
  include/target/iscsi/iscsi_transport.h |  2 +
  2 files changed, 65 insertions(+), 13 deletions(-)
drivers/target/iscsi/iscsi_target.c +16 -3
index 961202f..fdb33ba 100644
@@ -499,6 +499,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
  	__iscsit_free_cmd(cmd, scsi_cmd, true);
  }
 
+static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+{
+	return cmd->pdu;
+}
+
  static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
  {
  	return TARGET_PROT_NORMAL;
@@ -519,6 +524,7 @@ static struct iscsit_transport iscsi_target_transport = {
  	.iscsit_queue_data_in	= iscsit_queue_rsp,
  	.iscsit_queue_status	= iscsit_queue_rsp,
  	.iscsit_aborted_task	= iscsit_aborted_task,
+	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
  	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
  };
 
@@ -2537,7 +2543,10 @@ static int iscsit_send_conn_drop_async_message(
  	cmd->tx_size = ISCSI_HDR_LEN;
  	cmd->iscsi_opcode = ISCSI_OP_ASYNC_EVENT;
 
-	hdr			= (struct iscsi_async *) cmd->pdu;
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
  	hdr->opcode		= ISCSI_OP_ASYNC_EVENT;
  	hdr->flags		= ISCSI_FLAG_CMD_FINAL;
  	cmd->init_task_tag	= RESERVED_ITT;
@@ -2630,7 +2639,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
  static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
  {
-	struct iscsi_data_rsp *hdr = (struct iscsi_data_rsp *)&cmd->pdu[0];
+	struct iscsi_data_rsp *hdr;
  	struct iscsi_datain datain;
  	struct iscsi_datain_req *dr;
  	struct kvec *iov;
@@ -2675,6 +2684,10 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
  			set_statsn = true;
  	}
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
  	iscsit_build_datain_pdu(cmd, conn, &datain, hdr, set_statsn);
 
  	iov = &cmd->iov_data[0];
@@ -2843,13 +2856,20 @@ EXPORT_SYMBOL(iscsit_build_logout_rsp);
  static int
  iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
  {
+	struct iscsi_logout_rsp *hdr;
  	struct kvec *iov;
  	int niov = 0, tx_size, rc;
 
-	rc = iscsit_build_logout_rsp(cmd, conn,

drivers/target/iscsi/iscsi_target.c EXPORT_SYMBOL(iscsit_build_logout_rsp); · line 2870

Quoted 10 lines
drivers/target/iscsi/iscsi_target.c +8 -2
-			(struct iscsi_logout_rsp *)&cmd->pdu[0]);
-	if (rc < 0)
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
+	rc = iscsit_build_logout_rsp(cmd, conn, hdr);
+	if (rc < 0) {
+		if (conn->conn_transport->iscsit_free_pdu)
+			conn->conn_transport->iscsit_free_pdu(conn, cmd);

This creates a weird asymmetry where alloc is called unconditionally while free is conditional, I'd say implement an empty free for iscsit. Same for the rest of the code...

P.S. I didn't see any non-error path call to free_pdu, is that a possible leak (for drivers that actually allocate a PDU)?

On another unrelated note, I'd be very happy if we lose the iscsit_ prefix from all the callouts, it clear to everyone that it iscsi, no need for an explicit reminder...

#3 lore raw

Re: [PATCH v2 01/16] iscsi-target: add callback to alloc and free PDU

Quoted 9 lines · Sagi Grimberg
Quoted 6 lines · Varun Prakash

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

Quoted 8 lines
Quoted 4 lines

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().

Can you please explain why are you adding two different callouts? Who (Chelsio T5) will need it, and why they can't use the in-cmd pdu?

I am adding these to avoid per PDU 48 bytes(BHS) memcpy from cmd->pdu to transport driver tx buffer, iscsi-target can directly form iscsi hdr in transport driver tx buffer.

Quoted 72 lines
Quoted 72 lines
---
 drivers/target/iscsi/iscsi_target.c    | 76 ++++++++++++++++++++++++++++------
 include/target/iscsi/iscsi_transport.h |  2 +
 2 files changed, 65 insertions(+), 13 deletions(-)
drivers/target/iscsi/iscsi_target.c +16 -2
index 961202f..fdb33ba 100644
@@ -499,6 +499,11 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 	__iscsit_free_cmd(cmd, scsi_cmd, true);
 }
 
+static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+{
+	return cmd->pdu;
+}
+
 static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 {
 	return TARGET_PROT_NORMAL;
@@ -519,6 +524,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_queue_data_in	= iscsit_queue_rsp,
 	.iscsit_queue_status	= iscsit_queue_rsp,
 	.iscsit_aborted_task	= iscsit_aborted_task,
+	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
@@ -2537,7 +2543,10 @@ static int iscsit_send_conn_drop_async_message(
 	cmd->tx_size = ISCSI_HDR_LEN;
 	cmd->iscsi_opcode = ISCSI_OP_ASYNC_EVENT;
 
-	hdr			= (struct iscsi_async *) cmd->pdu;
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	hdr->opcode		= ISCSI_OP_ASYNC_EVENT;
 	hdr->flags		= ISCSI_FLAG_CMD_FINAL;
 	cmd->init_task_tag	= RESERVED_ITT;
@@ -2630,7 +2639,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 
 static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
-	struct iscsi_data_rsp *hdr = (struct iscsi_data_rsp *)&cmd->pdu[0];
+	struct iscsi_data_rsp *hdr;
 	struct iscsi_datain datain;
 	struct iscsi_datain_req *dr;
 	struct kvec *iov;
@@ -2675,6 +2684,10 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 			set_statsn = true;
 	}
 
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
 	iscsit_build_datain_pdu(cmd, conn, &datain, hdr, set_statsn);
 
 	iov = &cmd->iov_data[0];
@@ -2843,13 +2856,20 @@ EXPORT_SYMBOL(iscsit_build_logout_rsp);
 static int
 iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
+	struct iscsi_logout_rsp *hdr;
 	struct kvec *iov;
 	int niov = 0, tx_size, rc;
 
Quoted 15 lines

drivers/target/iscsi/iscsi_target.c EXPORT_SYMBOL(iscsit_build_logout_rsp); · line 2870

Quoted 11 lines
drivers/target/iscsi/iscsi_target.c +8 -3
-	rc = iscsit_build_logout_rsp(cmd, conn,
-			(struct iscsi_logout_rsp *)&cmd->pdu[0]);
-	if (rc < 0)
+	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
+	if (unlikely(!hdr))
+		return -ENOMEM;
+
+	rc = iscsit_build_logout_rsp(cmd, conn, hdr);
+	if (rc < 0) {
+		if (conn->conn_transport->iscsit_free_pdu)
+			conn->conn_transport->iscsit_free_pdu(conn, cmd);

This creates a weird asymmetry where alloc is called unconditionally while free is conditional, I'd say implement an empty free for iscsit. Same for the rest of the code...

Ok, I will add empty pdu free function for iscsi-target.

Quoted 3 lines

P.S. I didn't see any non-error path call to free_pdu, is that a possible leak (for drivers that actually allocate a PDU)?

In non error path there is a call to xmit_pdu after pdu allocation so no leak.

Quoted 4 lines

On another unrelated note, I'd be very happy if we lose the iscsit_ prefix from all the callouts, it clear to everyone that it iscsi, no need for an explicit reminder...

Ok, I will remove iscsit_ prefix if Nicholas agrees on this.

maintainer
#4 lore raw

Re: [PATCH v2 01/16] iscsi-target: add callback to alloc and free PDU

Quoted 13 lines
Quoted 13 lines · Varun Prakash
Quoted 10 lines

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().

Quoted 7 lines
Quoted 3 lines

Can you please explain why are you adding two different callouts? Who (Chelsio T5) will need it, and why they can't use the in-cmd pdu?

I am adding these to avoid per PDU 48 bytes(BHS) memcpy from cmd->pdu to transport driver tx buffer, iscsi-target can directly form iscsi hdr in transport driver tx buffer.

Is that what it's for? Looks meaningless to me, do you have an indication that this is some sort of bottleneck?

I see you memset in your pdu_alloc, where is the gain here anyway?

#5 lore raw

Re: [PATCH v2 01/16] iscsi-target: add callback to alloc and free PDU

Quoted 21 lines · Sagi Grimberg
Quoted 13 lines
Quoted 13 lines · Varun Prakash
Quoted 10 lines

Add two callbacks to struct iscsit_transport -

1. void *(*iscsit_alloc_pdu)()
iscsi-target uses this callback for
iSCSI PDU allocation.

2. void (*iscsit_free_pdu)
iscsi-target uses this callback
to free an iSCSI PDU which was
allocated by iscsit_alloc_pdu().

Quoted 7 lines
Quoted 3 lines

Can you please explain why are you adding two different callouts? Who (Chelsio T5) will need it, and why they can't use the in-cmd pdu?

I am adding these to avoid per PDU 48 bytes(BHS) memcpy from cmd->pdu to transport driver tx buffer, iscsi-target can directly form iscsi hdr in transport driver tx buffer.

Quoted 5 lines

Is that what it's for? Looks meaningless to me, do you have an indication that this is some sort of bottleneck?

I see you memset in your pdu_alloc, where is the gain here anyway?

We can definitely avoid memcpy if iscsi-target fills all the fields in iscsi hdr, in current code some fields are assumed to be zero so a memset is required in transport driver.
I will drop this patch in -v3, thanks.

#6 lore raw

PATCH v2 2/16 iscsi-target: add int (*iscsit_xmit_pdu)()

In reply to Varun Prakash

Add int (*iscsit_xmit_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to transmit an
iSCSI PDU.

---
 drivers/target/iscsi/iscsi_target.c    | 391 +++++++++------------------------
 include/target/iscsi/iscsi_transport.h |   2 +
 2 files changed, 102 insertions(+), 291 deletions(-)
drivers/target/iscsi/iscsi_target.c +100 -291
index fdb33ba..0e7a481 100644
@@ -504,6 +504,79 @@ static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 	return cmd->pdu;
 }
 
+static void iscsit_do_crypto_hash_buf(struct ahash_request *, const void *,
+				      u32, u32, u8 *, u8 *);
+static void iscsit_tx_thread_wait_for_tcp(struct iscsi_conn *);
+
+static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
+			   const void *data_buf, u32 data_buf_len)
+{
+	struct iscsi_hdr *hdr = (struct iscsi_hdr *)cmd->pdu;
+	struct kvec *iov;
+	u32 niov = 0, tx_size = ISCSI_HDR_LEN;
+	int ret;
+
+	iov = &cmd->iov_misc[0];
+	iov[niov].iov_base	= cmd->pdu;
+	iov[niov++].iov_len	= ISCSI_HDR_LEN;
+
+	if (conn->conn_ops->HeaderDigest) {
+		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
+
+		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
+					  ISCSI_HDR_LEN, 0, NULL,
+					  (u8 *)header_digest);
+
+		iov[0].iov_len += ISCSI_CRC_LEN;
+		tx_size += ISCSI_CRC_LEN;
+		pr_debug("Attaching CRC32C HeaderDigest"
+			 " to opcode 0x%x 0x%08x\n",
+			 hdr->opcode, *header_digest);
+	}
+
+	if (data_buf_len) {
+		u32 padding = ((-data_buf_len) & 3);
+
+		iov[niov].iov_base	= (void *)data_buf;
+		iov[niov++].iov_len	= data_buf_len;
+		tx_size += data_buf_len;
+
+		if (padding != 0) {
+			iov[niov].iov_base = &cmd->pad_bytes;
+			iov[niov++].iov_len = padding;
+			tx_size += padding;
+			pr_debug("Attaching %u additional"
+				 " padding bytes.\n", padding);
+		}
+
+		if (conn->conn_ops->DataDigest) {
+			iscsit_do_crypto_hash_buf(conn->conn_tx_hash,
+						  data_buf, data_buf_len,
+						  padding,
+						  (u8 *)&cmd->pad_bytes,
+						  (u8 *)&cmd->data_crc);
+
+			iov[niov].iov_base = &cmd->data_crc;
+			iov[niov++].iov_len = ISCSI_CRC_LEN;
+			tx_size += ISCSI_CRC_LEN;
+			pr_debug("Attached DataDigest for %u"
+				 " bytes opcode 0x%x, CRC 0x%08x\n",
+				 data_buf_len, hdr->opcode, cmd->data_crc);
+		}
+	}
+
+	cmd->iov_misc_count = niov;
+	cmd->tx_size = tx_size;
+
+	ret = iscsit_send_tx_data(cmd, conn, 1);
+	if (ret < 0) {
+		iscsit_tx_thread_wait_for_tcp(conn);
+		return ret;
+	}
+
+	return 0;
+}
+
 static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 {
 	return TARGET_PROT_NORMAL;
@@ -525,6 +598,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_queue_status	= iscsit_queue_rsp,
 	.iscsit_aborted_task	= iscsit_aborted_task,
 	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
+	.iscsit_xmit_pdu	= iscsit_xmit_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
@@ -2540,7 +2614,6 @@ static int iscsit_send_conn_drop_async_message(
 {
 	struct iscsi_async *hdr;
 
-	cmd->tx_size = ISCSI_HDR_LEN;
 	cmd->iscsi_opcode = ISCSI_OP_ASYNC_EVENT;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
@@ -2561,25 +2634,11 @@ static int iscsit_send_conn_drop_async_message(
 	hdr->param2		= cpu_to_be16(conn->sess->sess_ops->DefaultTime2Wait);
 	hdr->param3		= cpu_to_be16(conn->sess->sess_ops->DefaultTime2Retain);
 
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		cmd->tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32C HeaderDigest to"
-			" Async Message 0x%08x\n", *header_digest);
-	}
-
-	cmd->iov_misc[0].iov_base	= cmd->pdu;
-	cmd->iov_misc[0].iov_len	= cmd->tx_size;
-	cmd->iov_misc_count		= 1;
-
 	pr_debug("Sending Connection Dropped Async Message StatSN:"
 		" 0x%08x, for CID: %hu on CID: %hu\n", cmd->stat_sn,
 			cmd->logout_cid, conn->cid);
-	return 0;
+
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, NULL, 0);
 }
 
 static void iscsit_tx_thread_wait_for_tcp(struct iscsi_conn *conn)
@@ -2857,8 +2916,7 @@ static int
 iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
 	struct iscsi_logout_rsp *hdr;
-	struct kvec *iov;
-	int niov = 0, tx_size, rc;
+	int rc;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -2871,26 +2929,7 @@ iscsit_send_logout(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 		return rc;
 	}
 
-	tx_size = ISCSI_HDR_LEN;
-	iov = &cmd->iov_misc[0];
-	iov[niov].iov_base	= cmd->pdu;
-	iov[niov++].iov_len	= ISCSI_HDR_LEN;
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, &cmd->pdu[0],
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32C HeaderDigest to"
-			" Logout Response 0x%08x\n", *header_digest);
-	}
-	cmd->iov_misc_count = niov;
-	cmd->tx_size = tx_size;
-
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, NULL, 0);
 }
 
 void
@@ -2930,7 +2969,7 @@ static int iscsit_send_unsolicited_nopin(
 	int want_response)
 {
 	struct iscsi_nopin *hdr;
-	int tx_size = ISCSI_HDR_LEN, ret;
+	int ret;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -2938,30 +2977,12 @@ static int iscsit_send_unsolicited_nopin(
 
 	iscsit_build_nopin_rsp(cmd, conn, hdr, false);
 
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32C HeaderDigest to"
-			" NopIN 0x%08x\n", *header_digest);
-	}
-
-	cmd->iov_misc[0].iov_base	= cmd->pdu;
-	cmd->iov_misc[0].iov_len	= tx_size;
-	cmd->iov_misc_count	= 1;
-	cmd->tx_size		= tx_size;
-
 	pr_debug("Sending Unsolicited NOPIN TTT: 0x%08x StatSN:"
 		" 0x%08x CID: %hu\n", hdr->ttt, cmd->stat_sn, conn->cid);
 
-	ret = iscsit_send_tx_data(cmd, conn, 1);
-	if (ret < 0) {
-		iscsit_tx_thread_wait_for_tcp(conn);
+	ret = conn->conn_transport->iscsit_xmit_pdu(conn, cmd, NULL, 0);
+	if (ret < 0)
 		return ret;
-	}
 
 	spin_lock_bh(&cmd->istate_lock);
 	cmd->i_state = want_response ?
@@ -2975,9 +2996,6 @@ static int
 iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
 	struct iscsi_nopin *hdr;
-	struct kvec *iov;
-	u32 padding = 0;
-	int niov = 0, tx_size;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -2985,69 +3003,20 @@ iscsit_send_nopin(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 
 	iscsit_build_nopin_rsp(cmd, conn, hdr, true);
 
-	tx_size = ISCSI_HDR_LEN;
-	iov = &cmd->iov_misc[0];
-	iov[niov].iov_base	= cmd->pdu;
-	iov[niov++].iov_len	= ISCSI_HDR_LEN;
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32C HeaderDigest"
-			" to NopIn 0x%08x\n", *header_digest);
-	}
-
 	/*
 	 * NOPOUT Ping Data is attached to struct iscsi_cmd->buf_ptr.
 	 * NOPOUT DataSegmentLength is at struct iscsi_cmd->buf_ptr_size.
 	 */
-	if (cmd->buf_ptr_size) {
-		iov[niov].iov_base	= cmd->buf_ptr;
-		iov[niov++].iov_len	= cmd->buf_ptr_size;
-		tx_size += cmd->buf_ptr_size;
-
-		pr_debug("Echoing back %u bytes of ping"
-			" data.\n", cmd->buf_ptr_size);
-
-		padding = ((-cmd->buf_ptr_size) & 3);
-		if (padding != 0) {
-			iov[niov].iov_base = &cmd->pad_bytes;
-			iov[niov++].iov_len = padding;
-			tx_size += padding;
-			pr_debug("Attaching %u additional"
-				" padding bytes.\n", padding);
-		}
-		if (conn->conn_ops->DataDigest) {
-			iscsit_do_crypto_hash_buf(conn->conn_tx_hash,
-				cmd->buf_ptr, cmd->buf_ptr_size,
-				padding, (u8 *)&cmd->pad_bytes,
-				(u8 *)&cmd->data_crc);
+	pr_debug("Echoing back %u bytes of ping data.\n", cmd->buf_ptr_size);
 
-			iov[niov].iov_base = &cmd->data_crc;
-			iov[niov++].iov_len = ISCSI_CRC_LEN;
-			tx_size += ISCSI_CRC_LEN;
-			pr_debug("Attached DataDigest for %u"
-				" bytes of ping data, CRC 0x%08x\n",
-				cmd->buf_ptr_size, cmd->data_crc);
-		}
-	}
-
-	cmd->iov_misc_count = niov;
-	cmd->tx_size = tx_size;
-
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, cmd->buf_ptr,
+						     cmd->buf_ptr_size);
 }
 
 static int iscsit_send_r2t(
 	struct iscsi_cmd *cmd,
 	struct iscsi_conn *conn)
 {
-	int tx_size = 0;
 	struct iscsi_r2t *r2t;
 	struct iscsi_r2t_rsp *hdr;
 	int ret;
@@ -3075,38 +3044,18 @@ static int iscsit_send_r2t(
 	hdr->data_offset	= cpu_to_be32(r2t->offset);
 	hdr->data_length	= cpu_to_be32(r2t->xfer_len);
 
-	cmd->iov_misc[0].iov_base	= cmd->pdu;
-	cmd->iov_misc[0].iov_len	= ISCSI_HDR_LEN;
-	tx_size += ISCSI_HDR_LEN;
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 HeaderDigest for R2T"
-			" PDU 0x%08x\n", *header_digest);
-	}
-
 	pr_debug("Built %sR2T, ITT: 0x%08x, TTT: 0x%08x, StatSN:"
 		" 0x%08x, R2TSN: 0x%08x, Offset: %u, DDTL: %u, CID: %hu\n",
 		(!r2t->recovery_r2t) ? "" : "Recovery ", cmd->init_task_tag,
 		r2t->targ_xfer_tag, ntohl(hdr->statsn), r2t->r2t_sn,
 			r2t->offset, r2t->xfer_len, conn->cid);
 
-	cmd->iov_misc_count = 1;
-	cmd->tx_size = tx_size;
-
 	spin_lock_bh(&cmd->r2t_lock);
 	r2t->sent_r2t = 1;
 	spin_unlock_bh(&cmd->r2t_lock);
 
-	ret = iscsit_send_tx_data(cmd, conn, 1);
+	ret = conn->conn_transport->iscsit_xmit_pdu(conn, cmd, NULL, 0);
 	if (ret < 0) {
-		iscsit_tx_thread_wait_for_tcp(conn);
 		return ret;
 	}
 
@@ -3235,10 +3184,9 @@ EXPORT_SYMBOL(iscsit_build_rsp_pdu);
 static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
 	struct iscsi_scsi_rsp *hdr;
-	struct kvec *iov;
-	u32 padding = 0, tx_size = 0;
-	int iov_count = 0;
 	bool inc_stat_sn = (cmd->i_state == ISTATE_SEND_STATUS);
+	void *data_buf = NULL;
+	u32 padding = 0, data_buf_len = 0;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -3246,11 +3194,6 @@ static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 
 	iscsit_build_rsp_pdu(cmd, conn, inc_stat_sn, hdr);
 
-	iov = &cmd->iov_misc[0];
-	iov[iov_count].iov_base	= cmd->pdu;
-	iov[iov_count++].iov_len = ISCSI_HDR_LEN;
-	tx_size += ISCSI_HDR_LEN;
-
 	/*
 	 * Attach SENSE DATA payload to iSCSI Response PDU
 	 */
@@ -3262,56 +3205,23 @@ static int iscsit_send_response(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 
 		padding		= -(cmd->se_cmd.scsi_sense_length) & 3;
 		hton24(hdr->dlength, (u32)cmd->se_cmd.scsi_sense_length);
-		iov[iov_count].iov_base	= cmd->sense_buffer;
-		iov[iov_count++].iov_len =
-				(cmd->se_cmd.scsi_sense_length + padding);
-		tx_size += cmd->se_cmd.scsi_sense_length;
+		data_buf = cmd->sense_buffer;
+		data_buf_len = cmd->se_cmd.scsi_sense_length + padding;
 
 		if (padding) {
 			memset(cmd->sense_buffer +
 				cmd->se_cmd.scsi_sense_length, 0, padding);
-			tx_size += padding;
 			pr_debug("Adding %u bytes of padding to"
 				" SENSE.\n", padding);
 		}
 
-		if (conn->conn_ops->DataDigest) {
-			iscsit_do_crypto_hash_buf(conn->conn_tx_hash,
-				cmd->sense_buffer,
-				(cmd->se_cmd.scsi_sense_length + padding),
-				0, NULL, (u8 *)&cmd->data_crc);
-
-			iov[iov_count].iov_base    = &cmd->data_crc;
-			iov[iov_count++].iov_len     = ISCSI_CRC_LEN;
-			tx_size += ISCSI_CRC_LEN;
-
-			pr_debug("Attaching CRC32 DataDigest for"
-				" SENSE, %u bytes CRC 0x%08x\n",
-				(cmd->se_cmd.scsi_sense_length + padding),
-				cmd->data_crc);
-		}
-
 		pr_debug("Attaching SENSE DATA: %u bytes to iSCSI"
 				" Response PDU\n",
 				cmd->se_cmd.scsi_sense_length);
 	}
 
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, cmd->pdu,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 HeaderDigest for Response"
-				" PDU 0x%08x\n", *header_digest);
-	}
-
-	cmd->iov_misc_count = iov_count;
-	cmd->tx_size = tx_size;
-
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, data_buf,
+						     data_buf_len);
 }
 
 static u8 iscsit_convert_tcm_tmr_rsp(struct se_tmr_req *se_tmr)
@@ -3358,7 +3268,6 @@ static int
 iscsit_send_task_mgt_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
 	struct iscsi_tm_rsp *hdr;
-	u32 tx_size = 0;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -3366,26 +3275,7 @@ iscsit_send_task_mgt_rsp(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 
 	iscsit_build_task_mgt_rsp(cmd, conn, hdr);
 
-	cmd->iov_misc[0].iov_base	= cmd->pdu;
-	cmd->iov_misc[0].iov_len	= ISCSI_HDR_LEN;
-	tx_size += ISCSI_HDR_LEN;
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		cmd->iov_misc[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 HeaderDigest for Task"
-			" Mgmt Response PDU 0x%08x\n", *header_digest);
-	}
-
-	cmd->iov_misc_count = 1;
-	cmd->tx_size = tx_size;
-
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, NULL, 0);
 }
 
 static bool iscsit_check_inaddr_any(struct iscsi_np *np)
@@ -3622,60 +3512,21 @@ static int iscsit_send_text_rsp(
 	struct iscsi_conn *conn)
 {
 	struct iscsi_text_rsp *hdr;
-	struct kvec *iov;
-	u32 tx_size = 0;
-	int text_length, iov_count = 0, rc;
+	int text_length;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
 		return -ENOMEM;
 
-	rc = iscsit_build_text_rsp(cmd, conn, hdr, ISCSI_TCP);
-	if (rc < 0) {
+	text_length = iscsit_build_text_rsp(cmd, conn, hdr, ISCSI_TCP);
+	if (text_length < 0) {
 		if (conn->conn_transport->iscsit_free_pdu)
 			conn->conn_transport->iscsit_free_pdu(conn, cmd);
-		return rc;
-	}
-
-	text_length = rc;
-	iov = &cmd->iov_misc[0];
-	iov[iov_count].iov_base = cmd->pdu;
-	iov[iov_count++].iov_len = ISCSI_HDR_LEN;
-	iov[iov_count].iov_base	= cmd->buf_ptr;
-	iov[iov_count++].iov_len = text_length;
-
-	tx_size += (ISCSI_HDR_LEN + text_length);
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 HeaderDigest for"
-			" Text Response PDU 0x%08x\n", *header_digest);
-	}
-
-	if (conn->conn_ops->DataDigest) {
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash,
-				cmd->buf_ptr, text_length,
-				0, NULL, (u8 *)&cmd->data_crc);
-
-		iov[iov_count].iov_base	= &cmd->data_crc;
-		iov[iov_count++].iov_len = ISCSI_CRC_LEN;
-		tx_size	+= ISCSI_CRC_LEN;
-
-		pr_debug("Attaching DataDigest for %u bytes of text"
-			" data, CRC 0x%08x\n", text_length,
-			cmd->data_crc);
+		return text_length;
 	}
 
-	cmd->iov_misc_count = iov_count;
-	cmd->tx_size = tx_size;
-
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, cmd->buf_ptr,
+						     text_length);
 }
 
 void
@@ -3700,8 +3551,6 @@ static int iscsit_send_reject(
 	struct iscsi_conn *conn)
 {
 	struct iscsi_reject *hdr;
-	struct kvec *iov;
-	u32 iov_count = 0, tx_size;
 
 	hdr = conn->conn_transport->iscsit_alloc_pdu(conn, cmd);
 	if (unlikely(!hdr))
@@ -3709,44 +3558,11 @@ static int iscsit_send_reject(
 
 	iscsit_build_reject(cmd, conn, hdr);
 
-	iov = &cmd->iov_misc[0];
-	iov[iov_count].iov_base = cmd->pdu;
-	iov[iov_count++].iov_len = ISCSI_HDR_LEN;
-	iov[iov_count].iov_base = cmd->buf_ptr;
-	iov[iov_count++].iov_len = ISCSI_HDR_LEN;
-
-	tx_size = (ISCSI_HDR_LEN + ISCSI_HDR_LEN);
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, hdr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 HeaderDigest for"
-			" REJECT PDU 0x%08x\n", *header_digest);
-	}
-
-	if (conn->conn_ops->DataDigest) {
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, cmd->buf_ptr,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)&cmd->data_crc);
-
-		iov[iov_count].iov_base = &cmd->data_crc;
-		iov[iov_count++].iov_len  = ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-		pr_debug("Attaching CRC32 DataDigest for REJECT"
-				" PDU 0x%08x\n", cmd->data_crc);
-	}
-
-	cmd->iov_misc_count = iov_count;
-	cmd->tx_size = tx_size;
-
 	pr_debug("Built Reject PDU StatSN: 0x%08x, Reason: 0x%02x,"
 		" CID: %hu\n", ntohl(hdr->statsn), hdr->reason, conn->cid);
 
-	return 0;
+	return conn->conn_transport->iscsit_xmit_pdu(conn, cmd, cmd->buf_ptr,
+						     ISCSI_HDR_LEN);
 }
 
 void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
@@ -3939,13 +3755,6 @@ check_rsp_state:
 	if (ret < 0)
 		goto err;
 
-	if (iscsit_send_tx_data(cmd, conn, 1) < 0) {
-		iscsit_tx_thread_wait_for_tcp(conn);
-		iscsit_unmap_iovec(cmd);
-		goto err;
-	}
-	iscsit_unmap_iovec(cmd);
-
 	switch (state) {
 	case ISTATE_SEND_LOGOUTRSP:
 		if (!iscsit_logout_post_handler(cmd, conn))
include/target/iscsi/iscsi_transport.h +2 -0
index 526b715..f09973f 100644
@@ -24,6 +24,8 @@ struct iscsit_transport {
 	void (*iscsit_aborted_task)(struct iscsi_conn *, struct iscsi_cmd *);
 	void *(*iscsit_alloc_pdu)(struct iscsi_conn *, struct iscsi_cmd *);
 	void (*iscsit_free_pdu)(struct iscsi_conn *, struct iscsi_cmd *);
+	int (*iscsit_xmit_pdu)(struct iscsi_conn *, struct iscsi_cmd *,
+			       const void *, u32);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
#8 lore raw

PATCH v2 3/16 iscsi-target: add int (*iscsit_xmit_datain_pdu)()

In reply to Varun Prakash

Add int (*iscsit_xmit_datain_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to transmit a DATAIN
iSCSI PDU.

---
 drivers/target/iscsi/iscsi_target.c    | 143 +++++++++++++++++++--------------
 include/target/iscsi/iscsi_transport.h |   3 +
 2 files changed, 86 insertions(+), 60 deletions(-)
drivers/target/iscsi/iscsi_target.c +83 -60
index 0e7a481..9e65e5d 100644
@@ -577,6 +577,84 @@ static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	return 0;
 }
 
+static int iscsit_map_iovec(struct iscsi_cmd *, struct kvec *, u32, u32);
+static void iscsit_unmap_iovec(struct iscsi_cmd *);
+static u32 iscsit_do_crypto_hash_sg(struct ahash_request *, struct iscsi_cmd *,
+				    u32, u32, u32, u8 *);
+static int
+iscsit_xmit_datain_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
+		       struct iscsi_datain_req *dr, struct iscsi_datain *datain)
+{
+	struct kvec *iov;
+	u32 iov_count = 0, tx_size = 0;
+	int ret, iov_ret;
+
+	iov = &cmd->iov_data[0];
+	iov[iov_count].iov_base	= cmd->pdu;
+	iov[iov_count++].iov_len = ISCSI_HDR_LEN;
+	tx_size += ISCSI_HDR_LEN;
+
+	if (conn->conn_ops->HeaderDigest) {
+		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
+
+		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, cmd->pdu,
+					  ISCSI_HDR_LEN, 0, NULL,
+					  (u8 *)header_digest);
+
+		iov[0].iov_len += ISCSI_CRC_LEN;
+		tx_size += ISCSI_CRC_LEN;
+
+		pr_debug("Attaching CRC32 HeaderDigest for DataIN PDU 0x%08x\n",
+			 *header_digest);
+	}
+
+	iov_ret = iscsit_map_iovec(cmd, &cmd->iov_data[1],
+				   datain->offset, datain->length);
+	if (iov_ret < 0)
+		return -1;
+
+	iov_count += iov_ret;
+	tx_size += datain->length;
+
+	cmd->padding = ((-datain->length) & 3);
+	if (cmd->padding) {
+		iov[iov_count].iov_base		= cmd->pad_bytes;
+		iov[iov_count++].iov_len	= cmd->padding;
+		tx_size += cmd->padding;
+
+		pr_debug("Attaching %u padding bytes\n", cmd->padding);
+	}
+
+	if (conn->conn_ops->DataDigest) {
+		cmd->data_crc = iscsit_do_crypto_hash_sg(conn->conn_tx_hash,
+							 cmd, datain->offset,
+							 datain->length,
+							 cmd->padding,
+							 cmd->pad_bytes);
+
+		iov[iov_count].iov_base	= &cmd->data_crc;
+		iov[iov_count++].iov_len = ISCSI_CRC_LEN;
+		tx_size += ISCSI_CRC_LEN;
+
+		pr_debug("Attached CRC32C DataDigest %d bytes, crc 0x%08x\n",
+			 datain->length + cmd->padding, cmd->data_crc);
+	}
+
+	cmd->iov_data_count = iov_count;
+	cmd->tx_size = tx_size;
+
+	ret = iscsit_fe_sendpage_sg(cmd, conn);
+
+	iscsit_unmap_iovec(cmd);
+
+	if (ret < 0) {
+		iscsit_tx_thread_wait_for_tcp(conn);
+		return ret;
+	}
+
+	return 0;
+}
+
 static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 {
 	return TARGET_PROT_NORMAL;
@@ -599,6 +677,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_aborted_task	= iscsit_aborted_task,
 	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
 	.iscsit_xmit_pdu	= iscsit_xmit_pdu,
+	.iscsit_xmit_datain_pdu = iscsit_xmit_datain_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
@@ -2701,9 +2780,7 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 	struct iscsi_data_rsp *hdr;
 	struct iscsi_datain datain;
 	struct iscsi_datain_req *dr;
-	struct kvec *iov;
-	u32 iov_count = 0, tx_size = 0;
-	int eodr = 0, ret, iov_ret;
+	int eodr = 0, ret;
 	bool set_statsn = false;
 
 	memset(&datain, 0, sizeof(struct iscsi_datain));
@@ -2749,64 +2826,10 @@ static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 
 	iscsit_build_datain_pdu(cmd, conn, &datain, hdr, set_statsn);
 
-	iov = &cmd->iov_data[0];
-	iov[iov_count].iov_base	= cmd->pdu;
-	iov[iov_count++].iov_len	= ISCSI_HDR_LEN;
-	tx_size += ISCSI_HDR_LEN;
-
-	if (conn->conn_ops->HeaderDigest) {
-		u32 *header_digest = (u32 *)&cmd->pdu[ISCSI_HDR_LEN];
-
-		iscsit_do_crypto_hash_buf(conn->conn_tx_hash, cmd->pdu,
-				ISCSI_HDR_LEN, 0, NULL, (u8 *)header_digest);
-
-		iov[0].iov_len += ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-
-		pr_debug("Attaching CRC32 HeaderDigest"
-			" for DataIN PDU 0x%08x\n", *header_digest);
-	}
-
-	iov_ret = iscsit_map_iovec(cmd, &cmd->iov_data[1],
-				datain.offset, datain.length);
-	if (iov_ret < 0)
-		return -1;
-
-	iov_count += iov_ret;
-	tx_size += datain.length;
-
-	cmd->padding = ((-datain.length) & 3);
-	if (cmd->padding) {
-		iov[iov_count].iov_base		= cmd->pad_bytes;
-		iov[iov_count++].iov_len	= cmd->padding;
-		tx_size += cmd->padding;
-
-		pr_debug("Attaching %u padding bytes\n",
-				cmd->padding);
-	}
-	if (conn->conn_ops->DataDigest) {
-		cmd->data_crc = iscsit_do_crypto_hash_sg(conn->conn_tx_hash, cmd,
-			 datain.offset, datain.length, cmd->padding, cmd->pad_bytes);
-
-		iov[iov_count].iov_base	= &cmd->data_crc;
-		iov[iov_count++].iov_len = ISCSI_CRC_LEN;
-		tx_size += ISCSI_CRC_LEN;
-
-		pr_debug("Attached CRC32C DataDigest %d bytes, crc"
-			" 0x%08x\n", datain.length+cmd->padding, cmd->data_crc);
-	}
-
-	cmd->iov_data_count = iov_count;
-	cmd->tx_size = tx_size;
-
-	ret = iscsit_fe_sendpage_sg(cmd, conn);
-
-	iscsit_unmap_iovec(cmd);
-
-	if (ret < 0) {
-		iscsit_tx_thread_wait_for_tcp(conn);
+	ret = conn->conn_transport->iscsit_xmit_datain_pdu(conn, cmd,
+							   dr, &datain);
+	if (ret < 0)
 		return ret;
-	}
 
 	if (dr->dr_complete) {
 		eodr = (cmd->se_cmd.se_cmd_flags & SCF_TRANSPORT_TASK_SENSE) ?
include/target/iscsi/iscsi_transport.h +3 -0
index f09973f..77ae41e 100644
@@ -26,6 +26,9 @@ struct iscsit_transport {
 	void (*iscsit_free_pdu)(struct iscsi_conn *, struct iscsi_cmd *);
 	int (*iscsit_xmit_pdu)(struct iscsi_conn *, struct iscsi_cmd *,
 			       const void *, u32);
+	int (*iscsit_xmit_datain_pdu)(struct iscsi_conn *, struct iscsi_cmd *,
+				      struct iscsi_datain_req *,
+				      struct iscsi_datain *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer
#9 lore raw

Re: [PATCH v2 03/16] iscsi-target: add int (*iscsit_xmit_datain_pdu)()

Quoted 16 lines · Varun Prakash

Add int (*iscsit_xmit_datain_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to transmit a DATAIN
iSCSI PDU.

---
  drivers/target/iscsi/iscsi_target.c    | 143 +++++++++++++++++++--------------
  include/target/iscsi/iscsi_transport.h |   3 +
  2 files changed, 86 insertions(+), 60 deletions(-)
drivers/target/iscsi/iscsi_target.c +0 -0
index 0e7a481..9e65e5d 100644

drivers/target/iscsi/iscsi_target.c static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd, · line 586

Quoted 10 lines
drivers/target/iscsi/iscsi_target.c +7 -0
@@ -577,6 +577,84 @@ static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
  	return 0;
  }
 
+static int iscsit_map_iovec(struct iscsi_cmd *, struct kvec *, u32, u32);
+static void iscsit_unmap_iovec(struct iscsi_cmd *);
+static u32 iscsit_do_crypto_hash_sg(struct ahash_request *, struct iscsi_cmd *,
+				    u32, u32, u32, u8 *);
+static int
+iscsit_xmit_datain_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
+		       struct iscsi_datain_req *dr, struct iscsi_datain *datain)

Looks very similar to xmit_pdu(), if we add a datain pointer that can be null for normal pdus would that reduce code duplication?

#10 lore raw

Re: [PATCH v2 03/16] iscsi-target: add int (*iscsit_xmit_datain_pdu)()

Quoted 22 lines · Sagi Grimberg
Quoted 19 lines · Varun Prakash

Add int (*iscsit_xmit_datain_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to transmit a DATAIN
iSCSI PDU.

---
 drivers/target/iscsi/iscsi_target.c    | 143 +++++++++++++++++++--------------
 include/target/iscsi/iscsi_transport.h |   3 +
 2 files changed, 86 insertions(+), 60 deletions(-)
drivers/target/iscsi/iscsi_target.c +0 -0
index 0e7a481..9e65e5d 100644
@@ -577,6 +577,84 @@ static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	return 0;
 }
 
Quoted 10 lines

drivers/target/iscsi/iscsi_target.c static int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd, · line 586

Quoted 7 lines
drivers/target/iscsi/iscsi_target.c +7 -0
+static int iscsit_map_iovec(struct iscsi_cmd *, struct kvec *, u32, u32);
+static void iscsit_unmap_iovec(struct iscsi_cmd *);
+static u32 iscsit_do_crypto_hash_sg(struct ahash_request *, struct iscsi_cmd *,
+				    u32, u32, u32, u8 *);
+static int
+iscsit_xmit_datain_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
+		       struct iscsi_datain_req *dr, struct iscsi_datain *datain)

Looks very similar to xmit_pdu(), if we add a datain pointer that can be null for normal pdus would that reduce code duplication?

Yes, we can have a common xmit function for all pdus -

int iscsit_xmit_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd,

		    struct iscsi_datain_req *dr, const void *buf, u32 buf_len);

dr will be NULL for non datain pdus and buf will point to data buffer.
For datain pdus buf will point to datain(struct iscsi_datain) buffer, buf_len will be zero.

I will add this in -v3, thanks.

#11 lore raw

PATCH v2 4/16 iscsi-target: add void (*iscsit_release_cmd)()

In reply to Varun Prakash

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.

---
 drivers/target/iscsi/iscsi_target_util.c | 3 +++
 include/target/iscsi/iscsi_transport.h   | 1 +
 2 files changed, 4 insertions(+)
drivers/target/iscsi/iscsi_target_util.c +3 -0
index 428b0d9..a533017 100644
@@ -725,6 +725,9 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd,
 		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
 		iscsit_remove_cmd_from_response_queue(cmd, conn);
 	}
+
+	if (conn && conn->conn_transport->iscsit_release_cmd)
+		conn->conn_transport->iscsit_release_cmd(conn, cmd);
 }
 
 void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
include/target/iscsi/iscsi_transport.h +1 -0
index 77ae41e..19cef94 100644
@@ -29,6 +29,7 @@ struct iscsit_transport {
 	int (*iscsit_xmit_datain_pdu)(struct iscsi_conn *, struct iscsi_cmd *,
 				      struct iscsi_datain_req *,
 				      struct iscsi_datain *);
+	void (*iscsit_release_cmd)(struct iscsi_conn *, struct iscsi_cmd *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer
#12 lore raw

Re: [PATCH v2 04/16] iscsi-target: add void (*iscsit_release_cmd)()

Quoted 5 lines

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.

I'd really like to see some reasoning on why you add abstraction callouts. It may have a valid reason but it needs to be documented in the change log...

drivers/target/iscsi/iscsi_target_util.c void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd, · line 731

Quoted 12 lines
drivers/target/iscsi/iscsi_target_util.c +3 -0
index 428b0d9..a533017 100644
@@ -725,6 +725,9 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd,
  		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
  		iscsit_remove_cmd_from_response_queue(cmd, conn);
  	}
+
+	if (conn && conn->conn_transport->iscsit_release_cmd)
+		conn->conn_transport->iscsit_release_cmd(conn, cmd);
  }

Did you verify that you get here with conn = NULL (given that you test it)? If so, then can you please document why is it expected for this function to be called twice that we need to make it safe?

If not, then I'd move this check to be a WARN_ON/BUG_ON to hunt down when is this happening.

#13 lore raw

Re: [PATCH v2 04/16] iscsi-target: add void (*iscsit_release_cmd)()

Quoted 9 lines · Sagi Grimberg
Quoted 5 lines

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.

I'd really like to see some reasoning on why you add abstraction callouts. It may have a valid reason but it needs to be documented in the change log...

This is for releasing DDP resource and sg page
in case of PASSTHROUGH_SG_TO_MEM_NOALLOC.
I will update commit log in -v3.

Quoted 18 lines

drivers/target/iscsi/iscsi_target_util.c void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd, · line 731

Quoted 13 lines
drivers/target/iscsi/iscsi_target_util.c +3 -0
index 428b0d9..a533017 100644
@@ -725,6 +725,9 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd,
 		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
 		iscsit_remove_cmd_from_response_queue(cmd, conn);
 	}
+
+	if (conn && conn->conn_transport->iscsit_release_cmd)
+		conn->conn_transport->iscsit_release_cmd(conn, cmd);
 }

Did you verify that you get here with conn = NULL (given that you test it)? If so, then can you please document why is it expected for this function to be called twice that we need to make it safe?

Quoted 2 lines

If not, then I'd move this check to be a WARN_ON/BUG_ON to hunt down when is this happening.

There is already a check for NULL conn in __iscsit_free_cmd()

	if (conn && check_queues) {
  		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
  		iscsit_remove_cmd_from_response_queue(cmd, conn);
	}

cmd->conn can become NULL in ERL2 error recovery.

#14 lore raw

PATCH v2 5/16 iscsi-target: add void (*iscsit_get_rx_pdu)()

In reply to Varun Prakash

Add void (*iscsit_get_rx_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to receive and
process Rx iSCSI PDUs.

---
 drivers/infiniband/ulp/isert/ib_isert.c | 10 ++++++++++
 drivers/target/iscsi/iscsi_target.c     | 10 ++--------
 include/target/iscsi/iscsi_transport.h  |  1 +
 3 files changed, 13 insertions(+), 8 deletions(-)
drivers/infiniband/ulp/isert/ib_isert.c +10 -0
index 411e446..9118d7c 100644
@@ -3273,6 +3273,15 @@ static void isert_free_conn(struct iscsi_conn *conn)
 	isert_put_conn(isert_conn);
 }
 
+static void isert_get_rx_pdu(struct iscsi_conn *conn)
+{
+	struct completion comp;
+
+	init_completion(&comp);
+
+	wait_for_completion_interruptible(&comp);
+}
+
 static struct iscsit_transport iser_target_transport = {
 	.name			= "IB/iSER",
 	.transport_type		= ISCSI_INFINIBAND,
@@ -3291,6 +3300,7 @@ static struct iscsit_transport iser_target_transport = {
 	.iscsit_queue_data_in	= isert_put_datain,
 	.iscsit_queue_status	= isert_put_response,
 	.iscsit_aborted_task	= isert_aborted_task,
+	.iscsit_get_rx_pdu	= isert_get_rx_pdu,
 	.iscsit_get_sup_prot_ops = isert_get_sup_prot_ops,
 };
 
drivers/target/iscsi/iscsi_target.c +2 -8
index 9e65e5d..4311636 100644
@@ -3989,14 +3989,8 @@ int iscsi_target_rx_thread(void *arg)
 	if (rc < 0 || iscsi_target_check_conn_state(conn))
 		return 0;
 
-	if (conn->conn_transport->transport_type == ISCSI_INFINIBAND) {
-		struct completion comp;
-
-		init_completion(&comp);
-		rc = wait_for_completion_interruptible(&comp);
-		if (rc < 0)
-			goto transport_err;
-
+	if (conn->conn_transport->iscsit_get_rx_pdu) {
+		conn->conn_transport->iscsit_get_rx_pdu(conn);
 		goto transport_err;
 	}
 
include/target/iscsi/iscsi_transport.h +1 -0
index 19cef94..1f63d8c 100644
@@ -30,6 +30,7 @@ struct iscsit_transport {
 				      struct iscsi_datain_req *,
 				      struct iscsi_datain *);
 	void (*iscsit_release_cmd)(struct iscsi_conn *, struct iscsi_cmd *);
+	void (*iscsit_get_rx_pdu)(struct iscsi_conn *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer Acked-by on v2 5/16
#15 lore raw

Re: [PATCH v2 05/16] iscsi-target: add void (*iscsit_get_rx_pdu)()

Quoted 5 lines

Add void (*iscsit_get_rx_pdu)() to
struct iscsit_transport, iscsi-target
uses this callback to receive and
process Rx iSCSI PDUs.

Same comment on change logs.

The iser bit looks harmless

Though I agree with hch that we don't really need
rx threads in iSER, but that's another story...

#16 lore raw

PATCH v2 6/16 iscsi-target: split iscsi_target_rx_thread()

In reply to Varun Prakash

split iscsi_target_rx_thread() into two parts,
1. iscsi_target_rx_thread() is common to all
transport drivers, it will call Rx function
registered by transport driver.

2. iscsit_get_rx_pdu() is Rx function for
ISCSI_TCP transport.

---
 drivers/target/iscsi/iscsi_target.c | 59 +++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 26 deletions(-)
drivers/target/iscsi/iscsi_target.c +33 -26
index 4311636..98d96d4 100644
@@ -480,6 +480,7 @@ int iscsit_del_np(struct iscsi_np *np)
 
 static int iscsit_immediate_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
 static int iscsit_response_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
+static void iscsit_get_rx_pdu(struct iscsi_conn *);
 
 static int iscsit_queue_rsp(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 {
@@ -678,6 +679,7 @@ static struct iscsit_transport iscsi_target_transport = {
 	.iscsit_alloc_pdu	= iscsit_alloc_pdu,
 	.iscsit_xmit_pdu	= iscsit_xmit_pdu,
 	.iscsit_xmit_datain_pdu = iscsit_xmit_datain_pdu,
+	.iscsit_get_rx_pdu	= iscsit_get_rx_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
 };
 
@@ -3969,30 +3971,12 @@ static bool iscsi_target_check_conn_state(struct iscsi_conn *conn)
 	return ret;
 }
 
-int iscsi_target_rx_thread(void *arg)
+static void iscsit_get_rx_pdu(struct iscsi_conn *conn)
 {
-	int ret, rc;
+	int ret;
 	u8 buffer[ISCSI_HDR_LEN], opcode;
 	u32 checksum = 0, digest = 0;
-	struct iscsi_conn *conn = arg;
 	struct kvec iov;
-	/*
-	 * Allow ourselves to be interrupted by SIGINT so that a
-	 * connection recovery / failure event can be triggered externally.
-	 */
-	allow_signal(SIGINT);
-	/*
-	 * Wait for iscsi_post_login_handler() to complete before allowing
-	 * incoming iscsi/tcp socket I/O, and/or failing the connection.
-	 */
-	rc = wait_for_completion_interruptible(&conn->rx_login_comp);
-	if (rc < 0 || iscsi_target_check_conn_state(conn))
-		return 0;
-
-	if (conn->conn_transport->iscsit_get_rx_pdu) {
-		conn->conn_transport->iscsit_get_rx_pdu(conn);
-		goto transport_err;
-	}
 
 	while (!kthread_should_stop()) {
 		/*
@@ -4010,7 +3994,7 @@ int iscsi_target_rx_thread(void *arg)
 		ret = rx_data(conn, &iov, 1, ISCSI_HDR_LEN);
 		if (ret != ISCSI_HDR_LEN) {
 			iscsit_rx_thread_wait_for_tcp(conn);
-			goto transport_err;
+			return;
 		}
 
 		if (conn->conn_ops->HeaderDigest) {
@@ -4020,7 +4004,7 @@ int iscsi_target_rx_thread(void *arg)
 			ret = rx_data(conn, &iov, 1, ISCSI_CRC_LEN);
 			if (ret != ISCSI_CRC_LEN) {
 				iscsit_rx_thread_wait_for_tcp(conn);
-				goto transport_err;
+				return;
 			}
 
 			iscsit_do_crypto_hash_buf(conn->conn_rx_hash,
@@ -4044,7 +4028,7 @@ int iscsi_target_rx_thread(void *arg)
 		}
 
 		if (conn->conn_state == TARG_CONN_STATE_IN_LOGOUT)
-			goto transport_err;
+			return;
 
 		opcode = buffer[0] & ISCSI_OPCODE_MASK;
 
@@ -4055,15 +4039,38 @@ int iscsi_target_rx_thread(void *arg)
 			" while in Discovery Session, rejecting.\n", opcode);
 			iscsit_add_reject(conn, ISCSI_REASON_PROTOCOL_ERROR,
 					  buffer);
-			goto transport_err;
+			return;
 		}
 
 		ret = iscsi_target_rx_opcode(conn, buffer);
 		if (ret < 0)
-			goto transport_err;
+			return;
 	}
+}
+
+int iscsi_target_rx_thread(void *arg)
+{
+	int rc;
+	struct iscsi_conn *conn = arg;
+
+	/*
+	 * Allow ourselves to be interrupted by SIGINT so that a
+	 * connection recovery / failure event can be triggered externally.
+	 */
+	allow_signal(SIGINT);
+	/*
+	 * Wait for iscsi_post_login_handler() to complete before allowing
+	 * incoming iscsi/tcp socket I/O, and/or failing the connection.
+	 */
+	rc = wait_for_completion_interruptible(&conn->rx_login_comp);
+	if (rc < 0 || iscsi_target_check_conn_state(conn))
+		return 0;
+
+	if (!conn->conn_transport->iscsit_get_rx_pdu)
+		return 0;
+
+	conn->conn_transport->iscsit_get_rx_pdu(conn);
 
-transport_err:
 	if (!signal_pending(current))
 		atomic_set(&conn->transport_failed, 1);
 	iscsit_take_action_for_connection_exit(conn);
Signature · 1 line
2.0.2
#17 lore raw

PATCH v2 7/16 iscsi-target: add int (*iscsit_validate_params)()

In reply to Varun Prakash

Add int (*iscsit_validate_params)() to
struct iscsit_transport, iscsi-target
uses this callback for validating
conn operational parameters.

---
 drivers/target/iscsi/iscsi_target_login.c | 10 ++++++++++
 include/target/iscsi/iscsi_transport.h    |  1 +
 2 files changed, 11 insertions(+)
drivers/target/iscsi/iscsi_target_login.c +10 -0
index 8436d56..83574eea 100644
@@ -1387,6 +1387,16 @@ static int __iscsi_target_login_thread(struct iscsi_np *np)
 			goto old_sess_out;
 	}
 
+	if (conn->conn_transport->iscsit_validate_params) {
+		ret = conn->conn_transport->iscsit_validate_params(conn);
+		if (ret < 0) {
+			if (zero_tsih)
+				goto new_sess_out;
+			else
+				goto old_sess_out;
+		}
+	}
+
 	ret = iscsi_target_start_negotiation(login, conn);
 	if (ret < 0)
 		goto new_sess_out;
include/target/iscsi/iscsi_transport.h +1 -0
index 1f63d8c..dbef892 100644
@@ -31,6 +31,7 @@ struct iscsit_transport {
 				      struct iscsi_datain *);
 	void (*iscsit_release_cmd)(struct iscsi_conn *, struct iscsi_cmd *);
 	void (*iscsit_get_rx_pdu)(struct iscsi_conn *);
+	int (*iscsit_validate_params)(struct iscsi_conn *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer
#18 lore raw

Re: [PATCH v2 07/16] iscsi-target: add int (*iscsit_validate_params)()

Quoted 5 lines

Add int (*iscsit_validate_params)() to
struct iscsit_transport, iscsi-target
uses this callback for validating
conn operational parameters.

Again, why is this needed?

#19 lore raw

Re: [PATCH v2 07/16] iscsi-target: add int (*iscsit_validate_params)()

Quoted 7 lines · Sagi Grimberg
Quoted 5 lines

Add int (*iscsit_validate_params)() to
struct iscsit_transport, iscsi-target
uses this callback for validating
conn operational parameters.

Again, why is this needed?

This is for updating the value of
MAXXMITDATASEGMENTLENGTH to the value supported
by the adapter.
I will update the commit log in -v3.

#20 lore raw

PATCH v2 8/16 iscsi-target: add void (*iscsit_get_r2t_ttt)()

In reply to Varun Prakash

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.

---
 drivers/target/iscsi/iscsi_target.c    | 5 ++++-
 include/target/iscsi/iscsi_transport.h | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target.c +4 -1
index 98d96d4..ec785c8 100644
@@ -3060,7 +3060,10 @@ static int iscsit_send_r2t(
 	int_to_scsilun(cmd->se_cmd.orig_fe_lun,
 			(struct scsi_lun *)&hdr->lun);
 	hdr->itt		= cmd->init_task_tag;
-	r2t->targ_xfer_tag	= session_get_next_ttt(conn->sess);
+	if (conn->conn_transport->iscsit_get_r2t_ttt)
+		conn->conn_transport->iscsit_get_r2t_ttt(conn, cmd, r2t);
+	else
+		r2t->targ_xfer_tag = session_get_next_ttt(conn->sess);
 	hdr->ttt		= cpu_to_be32(r2t->targ_xfer_tag);
 	hdr->statsn		= cpu_to_be32(conn->stat_sn);
 	hdr->exp_cmdsn		= cpu_to_be32(conn->sess->exp_cmd_sn);
include/target/iscsi/iscsi_transport.h +2 -0
index dbef892..f949e70 100644
@@ -32,6 +32,8 @@ struct iscsit_transport {
 	void (*iscsit_release_cmd)(struct iscsi_conn *, struct iscsi_cmd *);
 	void (*iscsit_get_rx_pdu)(struct iscsi_conn *);
 	int (*iscsit_validate_params)(struct iscsi_conn *);
+	void (*iscsit_get_r2t_ttt)(struct iscsi_conn *, struct iscsi_cmd *,
+				   struct iscsi_r2t *);
 	enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
Signature · 1 line
2.0.2
maintainer
#21 lore raw

Re: [PATCH v2 08/16] iscsi-target: add void (*iscsit_get_r2t_ttt)()

Quoted 5 lines

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.

Your driver allocates ttt's? That looks like bad layering to me. This definitely deserves an explanation...

#22 lore raw

Re: [PATCH v2 08/16] iscsi-target: add void (*iscsit_get_r2t_ttt)()

Quoted 8 lines · Sagi Grimberg
Quoted 5 lines

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.

Your driver allocates ttt's? That looks like bad layering to me. This definitely deserves an explanation...

cxgbit.ko allocates ttt only for r2t pdus to do Direct Data Placement of Data Out pdus, adapter uses the ttt value in Data Out pdus to place data directly in the host buffers.

maintainer
#23 lore raw

Re: [PATCH v2 08/16] iscsi-target: add void (*iscsit_get_r2t_ttt)()

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.

Quoted 6 lines
Quoted 2 lines

Your driver allocates ttt's? That looks like bad layering to me. This definitely deserves an explanation...

cxgbit.ko allocates ttt only for r2t pdus to do Direct Data Placement of Data Out pdus, adapter uses the ttt value in Data Out pdus to place data directly in the host buffers.

How do you guarantee that the core doesn't conflict with your internal ttt's?

#24 lore raw

Re: [PATCH v2 08/16] iscsi-target: add void (*iscsit_get_r2t_ttt)()

Quoted 9 lines · Sagi Grimberg
Quoted 9 lines
Quoted 8 lines
Quoted 5 lines

Add void (*iscsit_get_r2t_ttt)() to
struct iscsit_transport, iscsi-target
uses this callback to get
r2t->targ_xfer_tag.

Your driver allocates ttt's? That looks like bad layering to me. This definitely deserves an explanation...

Quoted 6 lines
Quoted 3 lines

cxgbit.ko allocates ttt only for r2t pdus to do Direct Data Placement of Data Out pdus, adapter uses the ttt value in Data Out pdus to place data directly in the host buffers.

How do you guarantee that the core doesn't conflict with your internal ttt's?

- iscsi-target does not make any decision based on r2t->targ_xfer_tag.

- From iscsi RFC https://tools.ietf.org/html/rfc7143#section-11.8.5
There is no protocol rule about the Target Transfer Tag except that the value 0xffffffff is reserved and MUST NOT be sent by a target in an R2T.

- T5 adapter checks ttt only for Data Out pdus, so even if it is same for other pdus it does not matter.

#25 lore raw

PATCH v2 9/16 iscsi-target: move iscsit_thread_check_cpumask()

In reply to Varun Prakash

Move iscsit_thread_check_cpumask() to header file so that ISCSI_HW_OFFLOAD and other transport drivers can call this function to ensure both tx and rx thread runs on same cpu.

---
 drivers/target/iscsi/iscsi_target.c      | 26 --------------------------
 include/target/iscsi/iscsi_target_core.h | 26 ++++++++++++++++++++++++++
 2 files changed, 26 insertions(+), 26 deletions(-)
drivers/target/iscsi/iscsi_target.c +0 -26
index ec785c8..b655f0c 100644
@@ -3618,32 +3618,6 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
 	cpumask_setall(conn->conn_cpumask);
 }
 
-static inline void iscsit_thread_check_cpumask(
-	struct iscsi_conn *conn,
-	struct task_struct *p,
-	int mode)
-{
-	/*
-	 * mode == 1 signals iscsi_target_tx_thread() usage.
-	 * mode == 0 signals iscsi_target_rx_thread() usage.
-	 */
-	if (mode == 1) {
-		if (!conn->conn_tx_reset_cpumask)
-			return;
-		conn->conn_tx_reset_cpumask = 0;
-	} else {
-		if (!conn->conn_rx_reset_cpumask)
-			return;
-		conn->conn_rx_reset_cpumask = 0;
-	}
-	/*
-	 * Update the CPU mask for this single kthread so that
-	 * both TX and RX kthreads are scheduled to run on the
-	 * same CPU.
-	 */
-	set_cpus_allowed_ptr(p, conn->conn_cpumask);
-}
-
 static int
 iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
 {
include/target/iscsi/iscsi_target_core.h +26 -0
index c3371fa..01c41f2 100644
@@ -890,4 +890,30 @@ static inline u32 session_get_next_ttt(struct iscsi_session *session)
 }
 
 extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, itt_t);
+
+static inline void iscsit_thread_check_cpumask(
+	struct iscsi_conn *conn,
+	struct task_struct *p,
+	int mode)
+{
+	/*
+	 * mode == 1 signals iscsi_target_tx_thread() usage.
+	 * mode == 0 signals iscsi_target_rx_thread() usage.
+	 */
+	if (mode == 1) {
+		if (!conn->conn_tx_reset_cpumask)
+			return;
+		conn->conn_tx_reset_cpumask = 0;
+	} else {
+		if (!conn->conn_rx_reset_cpumask)
+			return;
+		conn->conn_rx_reset_cpumask = 0;
+	}
+	/*
+	 * Update the CPU mask for this single kthread so that
+	 * both TX and RX kthreads are scheduled to run on the
+	 * same CPU.
+	 */
+	set_cpus_allowed_ptr(p, conn->conn_cpumask);
+}
 #endif /* ISCSI_TARGET_CORE_H */
Signature · 1 line
2.0.2
#26 lore raw

PATCH v2 10/16 iscsi-target: use conn->network_transport in text rsp

In reply to Varun Prakash

Use conn->network_transport instead of
ISCSI_TCP to build text response.

---
 drivers/target/iscsi/iscsi_target.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/target/iscsi/iscsi_target.c +2 -1
index b655f0c..e61249d 100644
@@ -3546,7 +3546,8 @@ static int iscsit_send_text_rsp(
 	if (unlikely(!hdr))
 		return -ENOMEM;
 
-	text_length = iscsit_build_text_rsp(cmd, conn, hdr, ISCSI_TCP);
+	text_length = iscsit_build_text_rsp(cmd, conn, hdr,
+					    conn->network_transport);
 	if (text_length < 0) {
 		if (conn->conn_transport->iscsit_free_pdu)
 			conn->conn_transport->iscsit_free_pdu(conn, cmd);
Signature · 1 line
2.0.2
#28 lore raw

PATCH v2 11/16 iscsi-target: add new offload transport type

In reply to Varun Prakash

Add new transport type ISCSI_HW_OFFLOAD,
hw offload transport drivers will use
this transport type.

---
 drivers/target/iscsi/iscsi_target_configfs.c | 76 ++++++++++++++++++++++++++++
 include/target/iscsi/iscsi_target_core.h     |  1 +
 2 files changed, 77 insertions(+)
drivers/target/iscsi/iscsi_target_configfs.c +76 -0
index 97e5b69..3565e07 100644
@@ -182,12 +182,88 @@ out:
 	return rc;
 }
 
+static ssize_t lio_target_np_hw_offload_show(struct config_item *item,
+					     char *page)
+{
+	struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+	struct iscsi_tpg_np *tpg_np_hw_offload;
+	ssize_t rb;
+
+	tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+						       ISCSI_HW_OFFLOAD);
+	if (tpg_np_hw_offload)
+		rb = sprintf(page, "1\n");
+	else
+		rb = sprintf(page, "0\n");
+
+	return rb;
+}
+
+static ssize_t lio_target_np_hw_offload_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+	struct iscsi_np *np;
+	struct iscsi_portal_group *tpg;
+	struct iscsi_tpg_np *tpg_np_hw_offload = NULL;
+	u32 op;
+	int rc = 0;
+
+	rc = kstrtou32(page, 0, &op);
+	if (rc)
+		return rc;
+
+	if ((op != 1) && (op != 0)) {
+		pr_err("Illegal value for tpg_enable: %u\n", op);
+		return -EINVAL;
+	}
+
+	np = tpg_np->tpg_np;
+	if (!np) {
+		pr_err("Unable to locate struct iscsi_np from"
+		       " struct iscsi_tpg_np\n");
+		return -EINVAL;
+	}
+
+	tpg = tpg_np->tpg;
+	if (iscsit_get_tpg(tpg) < 0)
+		return -EINVAL;
+
+	if (op) {
+		tpg_np_hw_offload = iscsit_tpg_add_network_portal(tpg,
+				&np->np_sockaddr, tpg_np, ISCSI_HW_OFFLOAD);
+
+		if (IS_ERR(tpg_np_hw_offload)) {
+			rc = PTR_ERR(tpg_np_hw_offload);
+			goto out;
+		}
+	} else {
+		tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+				ISCSI_HW_OFFLOAD);
+
+		if (tpg_np_hw_offload) {
+			rc = iscsit_tpg_del_network_portal(tpg,
+							   tpg_np_hw_offload);
+			if (rc < 0)
+				goto out;
+		}
+	}
+
+	iscsit_put_tpg(tpg);
+	return count;
+out:
+	iscsit_put_tpg(tpg);
+	return rc;
+}
+
 CONFIGFS_ATTR(lio_target_np_, sctp);
 CONFIGFS_ATTR(lio_target_np_, iser);
+CONFIGFS_ATTR(lio_target_np_, hw_offload);
 
 static struct configfs_attribute *lio_target_portal_attrs[] = {
 	&lio_target_np_attr_sctp,
 	&lio_target_np_attr_iser,
+	&lio_target_np_attr_hw_offload,
 	NULL,
 };
 
include/target/iscsi/iscsi_target_core.h +1 -0
index 01c41f2..110c1fd 100644
@@ -74,6 +74,7 @@ enum iscsit_transport_type {
 	ISCSI_IWARP_TCP				= 3,
 	ISCSI_IWARP_SCTP			= 4,
 	ISCSI_INFINIBAND			= 5,
+	ISCSI_HW_OFFLOAD			= 6,
 };
 
 /* RFC-3720 7.1.4  Standard Connection State Diagram for a Target */
Signature · 1 line
2.0.2
maintainer
#29 lore raw

Re: [PATCH v2 11/16] iscsi-target: add new offload transport type

Quoted 68 lines
drivers/target/iscsi/iscsi_target_configfs.c +67 -0
+static ssize_t lio_target_np_hw_offload_show(struct config_item *item,
+					     char *page)
+{
+	struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+	struct iscsi_tpg_np *tpg_np_hw_offload;
+	ssize_t rb;
+
+	tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+						       ISCSI_HW_OFFLOAD);
+	if (tpg_np_hw_offload)
+		rb = sprintf(page, "1\n");
+	else
+		rb = sprintf(page, "0\n");
+
+	return rb;
+}
+
+static ssize_t lio_target_np_hw_offload_store(struct config_item *item,
+					      const char *page, size_t count)
+{
+	struct iscsi_tpg_np *tpg_np = to_iscsi_tpg_np(item);
+	struct iscsi_np *np;
+	struct iscsi_portal_group *tpg;
+	struct iscsi_tpg_np *tpg_np_hw_offload = NULL;
+	u32 op;
+	int rc = 0;
+
+	rc = kstrtou32(page, 0, &op);
+	if (rc)
+		return rc;
+
+	if ((op != 1) && (op != 0)) {
+		pr_err("Illegal value for tpg_enable: %u\n", op);
+		return -EINVAL;
+	}
+
+	np = tpg_np->tpg_np;
+	if (!np) {
+		pr_err("Unable to locate struct iscsi_np from"
+		       " struct iscsi_tpg_np\n");
+		return -EINVAL;
+	}
+
+	tpg = tpg_np->tpg;
+	if (iscsit_get_tpg(tpg) < 0)
+		return -EINVAL;
+
+	if (op) {
+		tpg_np_hw_offload = iscsit_tpg_add_network_portal(tpg,
+				&np->np_sockaddr, tpg_np, ISCSI_HW_OFFLOAD);
+
+		if (IS_ERR(tpg_np_hw_offload)) {
+			rc = PTR_ERR(tpg_np_hw_offload);
+			goto out;
+		}
+	} else {
+		tpg_np_hw_offload = iscsit_tpg_locate_child_np(tpg_np,
+				ISCSI_HW_OFFLOAD);
+
+		if (tpg_np_hw_offload) {
+			rc = iscsit_tpg_del_network_portal(tpg,
+							   tpg_np_hw_offload);
+			if (rc < 0)
+				goto out;
+		}
+	}
+

drivers/target/iscsi/iscsi_target_configfs.c out: · line 261

Quoted 10 lines
drivers/target/iscsi/iscsi_target_configfs.c +8 -0
+	iscsit_put_tpg(tpg);
+	return count;
+out:
+	iscsit_put_tpg(tpg);
+	return rc;
+}
+
  CONFIGFS_ATTR(lio_target_np_, sctp);
  CONFIGFS_ATTR(lio_target_np_, iser);
+CONFIGFS_ATTR(lio_target_np_, hw_offload);

I'd be happy to see new transports being added with some initiative to reduce the code duplication here (pretty please :))

#30 lore raw

PATCH v2 12/16 iscsi-target: clear tx_thread_active

In reply to Varun Prakash

clear tx_thread_active for ISCSI_HW_OFFLOAD
transport in logout_post_handler functions.

---
 drivers/target/iscsi/iscsi_target.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/target/iscsi/iscsi_target.c +4 -2
index e61249d..959644f 100644
@@ -4417,7 +4417,8 @@ static void iscsit_logout_post_handler_closesession(
 	 * always sleep waiting for RX/TX thread shutdown to complete
 	 * within iscsit_close_connection().
 	 */
-	if (conn->conn_transport->transport_type == ISCSI_TCP)
+	if ((conn->conn_transport->transport_type == ISCSI_TCP) ||
+	    (conn->conn_transport->transport_type == ISCSI_HW_OFFLOAD))
 		sleep = cmpxchg(&conn->tx_thread_active, true, false);
 
 	atomic_set(&conn->conn_logout_remove, 0);
@@ -4434,7 +4435,8 @@ static void iscsit_logout_post_handler_samecid(
 {
 	int sleep = 1;
 
-	if (conn->conn_transport->transport_type == ISCSI_TCP)
+	if ((conn->conn_transport->transport_type == ISCSI_TCP) ||
+	    (conn->conn_transport->transport_type == ISCSI_HW_OFFLOAD))
 		sleep = cmpxchg(&conn->tx_thread_active, true, false);
 
 	atomic_set(&conn->conn_logout_remove, 0);
Signature · 1 line
2.0.2
#31 lore raw

PATCH v2 13/16 iscsi-target: call complete on conn_logout_comp

In reply to Varun Prakash

ISCSI_HW_OFFLOAD transport drivers waits
on conn_logout_comp as ISCSI_TCP driver
so call complete if transport type
is ISCSI_HW_OFFLOAD.

---
 drivers/target/iscsi/iscsi_target.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/target/iscsi/iscsi_target.c +6 -4
index 959644f..edce5af 100644
@@ -4101,16 +4101,18 @@ int iscsit_close_connection(
 	pr_debug("Closing iSCSI connection CID %hu on SID:"
 		" %u\n", conn->cid, sess->sid);
 	/*
-	 * Always up conn_logout_comp for the traditional TCP case just in case
-	 * the RX Thread in iscsi_target_rx_opcode() is sleeping and the logout
-	 * response never got sent because the connection failed.
+	 * Always up conn_logout_comp for the traditional TCP and HW_OFFLOAD
+	 * case just in case the RX Thread in iscsi_target_rx_opcode() is
+	 * sleeping and the logout response never got sent because the
+	 * connection failed.
 	 *
 	 * However for iser-target, isert_wait4logout() is using conn_logout_comp
 	 * to signal logout response TX interrupt completion.  Go ahead and skip
 	 * this for iser since isert_rx_opcode() does not wait on logout failure,
 	 * and to avoid iscsi_conn pointer dereference in iser-target code.
 	 */
-	if (conn->conn_transport->transport_type == ISCSI_TCP)
+	if ((conn->conn_transport->transport_type == ISCSI_TCP) ||
+	    (conn->conn_transport->transport_type == ISCSI_HW_OFFLOAD))
 		complete(&conn->conn_logout_comp);
 
 	if (!strcmp(current->comm, ISCSI_RX_THREAD_NAME)) {
Signature · 1 line
2.0.2
#32 lore raw

PATCH v2 14/16 iscsi-target: export symbols

In reply to Varun Prakash

export symbols for ISCSI_HW_OFFLOAD
transport drivers.

---
 drivers/target/iscsi/iscsi_target.c               | 25 ++++++++++-------
 drivers/target/iscsi/iscsi_target_datain_values.c |  1 +
 drivers/target/iscsi/iscsi_target_login.c         |  3 ++-
 drivers/target/iscsi/iscsi_target_nego.c          |  1 +
 drivers/target/iscsi/iscsi_target_parameters.c    |  1 +
 drivers/target/iscsi/iscsi_target_util.c          |  2 ++
 include/target/iscsi/iscsi_transport.h            | 33 +++++++++++++++++++++++
 7 files changed, 56 insertions(+), 10 deletions(-)
drivers/target/iscsi/iscsi_target.c +16 -9
index edce5af..941f676 100644
@@ -478,17 +478,16 @@ int iscsit_del_np(struct iscsi_np *np)
 	return 0;
 }
 
-static int iscsit_immediate_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
-static int iscsit_response_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
 static void iscsit_get_rx_pdu(struct iscsi_conn *);
 
-static int iscsit_queue_rsp(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+int iscsit_queue_rsp(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 {
 	iscsit_add_cmd_to_response_queue(cmd, cmd->conn, cmd->i_state);
 	return 0;
 }
+EXPORT_SYMBOL(iscsit_queue_rsp);
 
-static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
+void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 {
 	bool scsi_cmd = (cmd->iscsi_opcode == ISCSI_OP_SCSI_CMD);
 
@@ -499,6 +498,7 @@ static void iscsit_aborted_task(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 
 	__iscsit_free_cmd(cmd, scsi_cmd, true);
 }
+EXPORT_SYMBOL(iscsit_aborted_task);
 
 static void *iscsit_alloc_pdu(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 {
@@ -795,7 +795,7 @@ static void __exit iscsi_target_cleanup_module(void)
 	kfree(iscsit_global);
 }
 
-static int iscsit_add_reject(
+int iscsit_add_reject(
 	struct iscsi_conn *conn,
 	u8 reason,
 	unsigned char *buf)
@@ -825,6 +825,7 @@ static int iscsit_add_reject(
 
 	return -1;
 }
+EXPORT_SYMBOL(iscsit_add_reject);
 
 static int iscsit_add_reject_from_cmd(
 	struct iscsi_cmd *cmd,
@@ -880,6 +881,7 @@ int iscsit_reject_cmd(struct iscsi_cmd *cmd, u8 reason, unsigned char *buf)
 {
 	return iscsit_add_reject_from_cmd(cmd, reason, false, buf);
 }
+EXPORT_SYMBOL(iscsit_reject_cmd);
 
 /*
  * Map some portion of the allocated scatterlist to an iovec, suitable for
@@ -2496,7 +2498,7 @@ iscsit_handle_logout_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 }
 EXPORT_SYMBOL(iscsit_handle_logout_cmd);
 
-static int iscsit_handle_snack(
+int iscsit_handle_snack(
 	struct iscsi_conn *conn,
 	unsigned char *buf)
 {
@@ -2549,6 +2551,7 @@ static int iscsit_handle_snack(
 
 	return 0;
 }
+EXPORT_SYMBOL(iscsit_handle_snack);
 
 static void iscsit_rx_thread_wait_for_tcp(struct iscsi_conn *conn)
 {
@@ -2732,7 +2735,7 @@ static void iscsit_tx_thread_wait_for_tcp(struct iscsi_conn *conn)
 	}
 }
 
-static void
+void
 iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 			struct iscsi_datain *datain, struct iscsi_data_rsp *hdr,
 			bool set_statsn)
@@ -2776,6 +2779,7 @@ iscsit_build_datain_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 		cmd->init_task_tag, ntohl(hdr->statsn), ntohl(hdr->datasn),
 		ntohl(hdr->offset), datain->length, conn->cid);
 }
+EXPORT_SYMBOL(iscsit_build_datain_pdu);
 
 static int iscsit_send_datain(struct iscsi_cmd *cmd, struct iscsi_conn *conn)
 {
@@ -3174,6 +3178,7 @@ int iscsit_build_r2ts_for_cmd(
 
 	return 0;
 }
+EXPORT_SYMBOL(iscsit_build_r2ts_for_cmd);
 
 void iscsit_build_rsp_pdu(struct iscsi_cmd *cmd, struct iscsi_conn *conn,
 			bool inc_stat_sn, struct iscsi_scsi_rsp *hdr)
@@ -3619,7 +3624,7 @@ void iscsit_thread_get_cpumask(struct iscsi_conn *conn)
 	cpumask_setall(conn->conn_cpumask);
 }
 
-static int
+int
 iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
 {
 	int ret;
@@ -3661,6 +3666,7 @@ iscsit_immediate_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state
 err:
 	return -1;
 }
+EXPORT_SYMBOL(iscsit_immediate_queue);
 
 static int
 iscsit_handle_immediate_queue(struct iscsi_conn *conn)
@@ -3685,7 +3691,7 @@ iscsit_handle_immediate_queue(struct iscsi_conn *conn)
 	return 0;
 }
 
-static int
+int
 iscsit_response_queue(struct iscsi_conn *conn, struct iscsi_cmd *cmd, int state)
 {
 	int ret;
@@ -3790,6 +3796,7 @@ check_rsp_state:
 err:
 	return -1;
 }
+EXPORT_SYMBOL(iscsit_response_queue);
 
 static int iscsit_handle_response_queue(struct iscsi_conn *conn)
 {
drivers/target/iscsi/iscsi_target_datain_values.c +1 -0
index fb3b52b..647d4a5 100644
@@ -524,3 +524,4 @@ struct iscsi_datain_req *iscsit_get_datain_values(
 
 	return NULL;
 }
+EXPORT_SYMBOL(iscsit_get_datain_values);
drivers/target/iscsi/iscsi_target_login.c +2 -1
index 83574eea..a09498f 100644
@@ -258,7 +258,7 @@ static void iscsi_login_set_conn_values(
 	mutex_unlock(&auth_id_lock);
 }
 
-static __printf(2, 3) int iscsi_change_param_sprintf(
+__printf(2, 3) int iscsi_change_param_sprintf(
 	struct iscsi_conn *conn,
 	const char *fmt, ...)
 {
@@ -279,6 +279,7 @@ static __printf(2, 3) int iscsi_change_param_sprintf(
 
 	return 0;
 }
+EXPORT_SYMBOL(iscsi_change_param_sprintf);
 
 /*
  *	This is the leading connection of a new session,
drivers/target/iscsi/iscsi_target_nego.c +1 -0
index 9fc9117..ca06132 100644
@@ -269,6 +269,7 @@ int iscsi_target_check_login_request(
 
 	return 0;
 }
+EXPORT_SYMBOL(iscsi_target_check_login_request);
 
 static int iscsi_target_check_first_request(
 	struct iscsi_conn *conn,
drivers/target/iscsi/iscsi_target_parameters.c +1 -0
index 3a1f9a7..0efa80b 100644
@@ -680,6 +680,7 @@ struct iscsi_param *iscsi_find_param_from_key(
 	pr_err("Unable to locate key \"%s\".\n", key);
 	return NULL;
 }
+EXPORT_SYMBOL(iscsi_find_param_from_key);
 
 int iscsi_extract_key_value(char *textbuf, char **key, char **value)
 {
drivers/target/iscsi/iscsi_target_util.c +2 -0
index a533017..43e176f 100644
@@ -514,6 +514,7 @@ void iscsit_add_cmd_to_immediate_queue(
 
 	wake_up(&conn->queues_wq);
 }
+EXPORT_SYMBOL(iscsit_add_cmd_to_immediate_queue);
 
 struct iscsi_queue_req *iscsit_get_cmd_from_immediate_queue(struct iscsi_conn *conn)
 {
@@ -776,6 +777,7 @@ void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
 		break;
 	}
 }
+EXPORT_SYMBOL(iscsit_free_cmd);
 
 int iscsit_check_session_usage_count(struct iscsi_session *sess)
 {
include/target/iscsi/iscsi_transport.h +33 -0
index f949e70..05aeedc 100644
@@ -89,6 +89,18 @@ extern void iscsit_build_reject(struct iscsi_cmd *, struct iscsi_conn *,
 extern int iscsit_build_logout_rsp(struct iscsi_cmd *, struct iscsi_conn *,
 				struct iscsi_logout_rsp *);
 extern int iscsit_logout_post_handler(struct iscsi_cmd *, struct iscsi_conn *);
+extern int iscsit_queue_rsp(struct iscsi_conn *, struct iscsi_cmd *);
+extern void iscsit_aborted_task(struct iscsi_conn *, struct iscsi_cmd *);
+extern int iscsit_add_reject(struct iscsi_conn *, u8, unsigned char *);
+extern int iscsit_reject_cmd(struct iscsi_cmd *, u8, unsigned char *);
+extern int iscsit_handle_snack(struct iscsi_conn *, unsigned char *);
+extern void iscsit_build_datain_pdu(struct iscsi_cmd *, struct iscsi_conn *,
+				    struct iscsi_datain *,
+				    struct iscsi_data_rsp *, bool);
+extern int iscsit_build_r2ts_for_cmd(struct iscsi_conn *, struct iscsi_cmd *,
+				     bool);
+extern int iscsit_immediate_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
+extern int iscsit_response_queue(struct iscsi_conn *, struct iscsi_cmd *, int);
 /*
  * From iscsi_target_device.c
  */
@@ -114,3 +126,24 @@ extern struct iscsi_cmd *iscsit_allocate_cmd(struct iscsi_conn *, int);
 extern int iscsit_sequence_cmd(struct iscsi_conn *, struct iscsi_cmd *,
 			       unsigned char *, __be32);
 extern void iscsit_release_cmd(struct iscsi_cmd *);
+extern void iscsit_free_cmd(struct iscsi_cmd *, bool);
+extern void iscsit_add_cmd_to_immediate_queue(struct iscsi_cmd *,
+					      struct iscsi_conn *, u8);
+
+/*
+ * From iscsi_target_nego.c
+ */
+extern int iscsi_target_check_login_request(struct iscsi_conn *,
+					    struct iscsi_login *);
+
+/*
+ * From iscsi_target_login.c
+ */
+extern __printf(2, 3) int iscsi_change_param_sprintf(
+	struct iscsi_conn *, const char *, ...);
+
+/*
+ * From iscsi_target_parameters.c
+ */
+extern struct iscsi_param *iscsi_find_param_from_key(
+	char *, struct iscsi_param_list *);
Signature · 1 line
2.0.2
#33 lore raw

PATCH v2 15/16 iscsi-target: fix seq_end_offset calculation

In reply to Varun Prakash

In case of unsolicited data, seq end offset for first sequence must be equal to the min of firstburstlength and data length.

---
 drivers/target/iscsi/iscsi_target_erl0.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/target/iscsi/iscsi_target_erl0.c +3 -3
index 210f6e4..a0fb5a8 100644
@@ -44,10 +44,10 @@ void iscsit_set_dataout_sequence_values(
 	 */
 	if (cmd->unsolicited_data) {
 		cmd->seq_start_offset = cmd->write_data_done;
-		cmd->seq_end_offset = (cmd->write_data_done +
-			((cmd->se_cmd.data_length >
+		cmd->seq_end_offset = (cmd->se_cmd.data_length >
 			  conn->sess->sess_ops->FirstBurstLength) ?
-			 conn->sess->sess_ops->FirstBurstLength : cmd->se_cmd.data_length));
+			 conn->sess->sess_ops->FirstBurstLength :
+			 cmd->se_cmd.data_length;
 		return;
 	}
 
Signature · 1 line
2.0.2
maintainer
#34 lore raw

Re: [PATCH v2 15/16] iscsi-target: fix seq_end_offset calculation

Fixes should go in the front of the series (probably even better detached from the set), and this also looks like stable material...

#35 lore raw

PATCH v2 16/16 cxgbit: add files for cxgbit.ko

In reply to Varun Prakash

cxgbit.h - This file contains data structure
definitions for cxgbit.ko.

cxgbit_lro.h - This file contains data structure
definitions for LRO support.

cxgbit_main.c - This file contains code for
registering with iscsi target transport and
cxgb4 driver.

cxgbit_cm.c - This file contains code for
connection management.

cxgbit_target.c - This file contains code
for processing iSCSI PDU.

cxgbit_ddp.c - This file contains code for Direct Data Placement.
v2: added check for NULL sg in cxgbit_set_one_ppod()

cxgbit: add Kconfig and Makefile
v2: added dependency on INET

iscsi-target: update Kconfig and Makefile
for compiling cxgbit.ko.

---
 drivers/target/iscsi/Kconfig                |    2 +
 drivers/target/iscsi/Makefile               |    1 +
 drivers/target/iscsi/cxgbit/Kconfig         |    7 +
 drivers/target/iscsi/cxgbit/Makefile        |    6 +
 drivers/target/iscsi/cxgbit/cxgbit.h        |  358 +++++
 drivers/target/iscsi/cxgbit/cxgbit_cm.c     | 2086 +++++++++++++++++++++++++++
 drivers/target/iscsi/cxgbit/cxgbit_ddp.c    |  325 +++++
 drivers/target/iscsi/cxgbit/cxgbit_lro.h    |   72 +
 drivers/target/iscsi/cxgbit/cxgbit_main.c   |  703 +++++++++
 drivers/target/iscsi/cxgbit/cxgbit_target.c | 1601 ++++++++++++++++++++
 10 files changed, 5161 insertions(+)
 create mode 100644 drivers/target/iscsi/cxgbit/Kconfig
 create mode 100644 drivers/target/iscsi/cxgbit/Makefile
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit.h
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_cm.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_ddp.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_lro.h
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_main.c
 create mode 100644 drivers/target/iscsi/cxgbit/cxgbit_target.c
drivers/target/iscsi/Kconfig +2 -0
index 8345fb4..bbdbf9c 100644
@@ -7,3 +7,5 @@ config ISCSI_TARGET
 	help
 	Say M here to enable the ConfigFS enabled Linux-iSCSI.org iSCSI
 	Target Mode Stack.
+
+source	"drivers/target/iscsi/cxgbit/Kconfig"
drivers/target/iscsi/Makefile +1 -0
index 0f43be9..0f18295 100644
@@ -18,3 +18,4 @@ iscsi_target_mod-y +=		iscsi_target_parameters.o \
 				iscsi_target_transport.o
 
 obj-$(CONFIG_ISCSI_TARGET)	+= iscsi_target_mod.o
+obj-$(CONFIG_ISCSI_TARGET_CXGB4) += cxgbit/
drivers/target/iscsi/cxgbit/Kconfig new file +7 -0
new file mode 100644
index 0000000..c9b6a3c
@@ -0,0 +1,7 @@
+config ISCSI_TARGET_CXGB4
+	tristate "Chelsio iSCSI target offload driver"
+	depends on ISCSI_TARGET && CHELSIO_T4 && INET
+	select CHELSIO_T4_UWIRE
+	---help---
+	To compile this driver as module, choose M here: the module
+	will be called cxgbit.
drivers/target/iscsi/cxgbit/Makefile new file +6 -0
new file mode 100644
index 0000000..bd56c07
@@ -0,0 +1,6 @@
+ccflags-y := -Idrivers/net/ethernet/chelsio/cxgb4
+ccflags-y += -Idrivers/target/iscsi
+
+obj-$(CONFIG_ISCSI_TARGET_CXGB4)  += cxgbit.o
+
+cxgbit-y  := cxgbit_main.o cxgbit_cm.o cxgbit_target.o cxgbit_ddp.o
drivers/target/iscsi/cxgbit/cxgbit.h new file +358 -0

358 lines not on this page · raw message

drivers/target/iscsi/cxgbit/cxgbit_cm.c new file +2086 -0

2,086 lines not on this page · raw message

drivers/target/iscsi/cxgbit/cxgbit_ddp.c new file +325 -0

325 lines not on this page · raw message

drivers/target/iscsi/cxgbit/cxgbit_lro.h new file +72 -0

72 lines not on this page · raw message

drivers/target/iscsi/cxgbit/cxgbit_main.c new file +703 -0

137 lines not on this page · raw message

drivers/target/iscsi/cxgbit/cxgbit_target.c new file +1601 -0

1,601 lines not on this page · raw message

The rest of this diff is not shown. View the raw message.

Signature · 1 line
2.0.2

Part of the diff is not shown here. View the raw message.