Re: [PATCH v12 08/26] nvme-tcp: Add DDP data-path

Sagi Grimberg [off-list ref] writes:

On 7/12/23 19:14, Aurelien Aptel wrote:

quoted

+static int nvme_tcp_req_map_ddp_sg(struct nvme_tcp_request *req, struct request *rq)

Why do you pass both req and rq? You can derive each from the other.

Thanks, we will remove the redundant parameter.

quoted

+{
+     int ret;
+
+     req->ddp.sg_table.sgl = req->ddp.first_sgl;
+     ret = sg_alloc_table_chained(&req->ddp.sg_table,
+                                  blk_rq_nr_phys_segments(rq),
+                                  req->ddp.sg_table.sgl, SG_CHUNK_SIZE);
+     if (ret)
+             return -ENOMEM;
+     req->ddp.nents = blk_rq_map_sg(rq->q, rq, req->ddp.sg_table.sgl);

General question, I'm assuming that the hca knows how to deal with
a controller that sends c2hdata in parts?

Yes, the hardware supports the offloading of multiple c2hdata PDUs per IO.

quoted

+static int nvme_tcp_setup_ddp(struct nvme_tcp_queue *queue, u16 command_id,
+                           struct request *rq)

I think you can use nvme_cid(rq) instead of passing the command_id.

Thanks, we will use it.

quoted

+{
+     struct net_device *netdev = queue->ctrl->offloading_netdev;
+     struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
+     int ret;
+
+     if (rq_data_dir(rq) != READ ||
+         queue->ctrl->offload_io_threshold > blk_rq_payload_bytes(rq))
+             return 0;
+
+     req->ddp.command_id = command_id;
+     ret = nvme_tcp_req_map_ddp_sg(req, rq);

Don't see why map_ddp_sg is not open-coded here, its the only call-site,
and its pretty much does exactly what its called.

Sure, we will open code it.

quoted

@@ -1308,6 +1407,15 @@ static int nvme_tcp_try_send_cmd_pdu(struct nvme_tcp_request *req)
      else
              msg.msg_flags |= MSG_EOR;

+     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags)) {
+             ret = nvme_tcp_setup_ddp(queue, pdu->cmd.common.command_id,
+                                      blk_mq_rq_from_pdu(req));
+             WARN_ONCE(ret, "ddp setup failed (queue 0x%x, cid 0x%x, ret=%d)",
+                       nvme_tcp_queue_id(queue),
+                       pdu->cmd.common.command_id,
+                       ret);
+     }

Any reason why this is done here when sending the command pdu and not
in setup time?

We wish to interact with the HW from the same CPU per queue, hence we
are calling setup_ddp() after queue->io_cpu == raw_smp_processor_id()
was checked in nvme_tcp_queue_request().