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

[Varun Prakash]

On Wed, Apr 13, 2016 at 12:56:25PM +0300, Sagi Grimberg wrote:
> 
> >>On 09/04/16 16:11, Varun Prakash wrote:
> >>>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?
> >
> >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?

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. 
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sagi Grimberg]

On 09/04/16 16:11, Varun Prakash wrote:

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?


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?
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Varun Prakash]

On Sun, Apr 10, 2016 at 08:33:53PM +0300, Sagi Grimberg wrote:
> 
> 
> On 09/04/16 16:11, Varun Prakash wrote:
> >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?

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. 

> 
> >
> >Signed-off-by: Varun Prakash 
> >---
> >  drivers/target/iscsi/iscsi_target.c| 76 
> > --
> >  include/target/iscsi/iscsi_transport.h |  2 +
> >  2 files changed, 65 insertions(+), 13 deletions(-)
> >
> >diff --git a/drivers/target/iscsi/iscsi_target.c 
> >b/drivers/target/iscsi/iscsi_target.c
> >index 961202f..fdb33ba 100644
> >--- a/drivers/target/iscsi/iscsi_target.c
> >+++ b/drivers/target/iscsi/iscsi_target.c
> >@@ -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 *)>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, , hdr, set_statsn);
> >
> > iov = >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 *)>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.
 
> 
> 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.
  
> 
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sagi Grimberg]

On 09/04/16 16:11, Varun Prakash wrote:

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?



Signed-off-by: Varun Prakash 
---
  drivers/target/iscsi/iscsi_target.c| 76 --
  include/target/iscsi/iscsi_transport.h |  2 +
  2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 961202f..fdb33ba 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -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 *)>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, , hdr, set_statsn);

iov = >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 *)>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...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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().

Signed-off-by: Varun Prakash 
---
 drivers/target/iscsi/iscsi_target.c| 76 --
 include/target/iscsi/iscsi_transport.h |  2 +
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c 
b/drivers/target/iscsi/iscsi_target.c
index 961202f..fdb33ba 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -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 *)>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, , hdr, set_statsn);
 
iov = >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 *)>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 = >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 *)>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 *)>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  |=