Re: [PATCH v3 net-next 07/21] nvme-tcp: Add DDP data-path

[Or Gerlitz <gerlitz.or@gmail.com>]

On Wed, Feb 3, 2021 at 10:54 AM Sagi Grimberg <sagi@grimberg.me> wrote:

> > +static
> > +int nvme_tcp_teardown_ddp(struct nvme_tcp_queue *queue,
> > +                       u16 command_id,
> > +                       struct request *rq)
> > +{
> > +     struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> > +     struct net_device *netdev = queue->ctrl->offloading_netdev;
> > +     int ret;
> > +
> > +     if (unlikely(!netdev)) {
> > +             pr_info_ratelimited("%s: netdev not found\n", __func__);
> > +             return -EINVAL;
> > +     }
> > +
> > +     ret = netdev->tcp_ddp_ops->tcp_ddp_teardown(netdev, queue->sock->sk,
> > +                                                 &req->ddp, rq);
> > +     sg_free_table_chained(&req->ddp.sg_table, SG_CHUNK_SIZE);
> > +     req->offloaded = false;
>
> Why is the offloaded = false needed here? you also clear it when you setup.

yep, there two places where we needlessly falsified the offloaded flag
- will remove them

The lifetime rules are - set to false on cmd setup and set to true in
ddp setup if it succeeded

>
> > +     return ret;
> > +}
> > +
> > +static void nvme_tcp_ddp_teardown_done(void *ddp_ctx)
> > +{
> > +     struct request *rq = ddp_ctx;
> > +     struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> > +
> > +     if (!nvme_try_complete_req(rq, cpu_to_le16(req->status << 1), req->result))
> > +             nvme_complete_rq(rq);
>
> Why is the status shifted here? it was taken from the cqe as is..

there are two cases

1. the status is taken from the cqe as is
2. the status is taken from the req with shift left (the success bit
IO read flow)

for #2 we already do the shift left in nvme_tcp_end_request so no need to
repeat it here,  will fix

>
> > +}
> > +
> > +static
> > +int nvme_tcp_setup_ddp(struct nvme_tcp_queue *queue,
> > +                    u16 command_id,
> > +                    struct request *rq)
> > +{
> > +     struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> > +     struct net_device *netdev = queue->ctrl->offloading_netdev;
> > +     int ret;
> > +
> > +     req->offloaded = false;
> > +
> > +     if (unlikely(!netdev)) {
> > +             pr_info_ratelimited("%s: netdev not found\n", __func__);
>
> dev_info_ratelimited please.

ok

> > +             return -EINVAL;
> > +     }
> > +
> > +     req->ddp.command_id = command_id;
> > +     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);
> > +
> > +     ret = netdev->tcp_ddp_ops->tcp_ddp_setup(netdev,
> > +                                              queue->sock->sk,
> > +                                              &req->ddp);
> > +     if (!ret)
> > +             req->offloaded = true;
> > +     return ret;
> > +}
> > +
> >   static
> >   int nvme_tcp_offload_socket(struct nvme_tcp_queue *queue)
> >   {
> > @@ -377,6 +446,25 @@ bool nvme_tcp_resync_request(struct sock *sk, u32 seq, u32 flags)
> >
> >   #else
> >
> > +static
> > +int nvme_tcp_setup_ddp(struct nvme_tcp_queue *queue,
> > +                    u16 command_id,
> > +                    struct request *rq)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +static
> > +int nvme_tcp_teardown_ddp(struct nvme_tcp_queue *queue,
> > +                       u16 command_id,
> > +                       struct request *rq)
> > +{
> > +     return -EINVAL;
> > +}
> > +
> > +static void nvme_tcp_ddp_teardown_done(void *ddp_ctx)
> > +{}
> > +
> >   static
> >   int nvme_tcp_offload_socket(struct nvme_tcp_queue *queue)
> >   {
> > @@ -665,6 +753,7 @@ static void nvme_tcp_error_recovery(struct nvme_ctrl *ctrl)
> >   static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> >               struct nvme_completion *cqe)
> >   {
> > +     struct nvme_tcp_request *req;
> >       struct request *rq;
> >
> >       rq = blk_mq_tag_to_rq(nvme_tcp_tagset(queue), cqe->command_id);
> > @@ -676,8 +765,15 @@ static int nvme_tcp_process_nvme_cqe(struct nvme_tcp_queue *queue,
> >               return -EINVAL;
> >       }
> >
> > -     if (!nvme_try_complete_req(rq, cqe->status, cqe->result))
> > -             nvme_complete_rq(rq);
> > +     req = blk_mq_rq_to_pdu(rq);
> > +     if (req->offloaded) {
> > +             req->status = cqe->status;
> > +             req->result = cqe->result;
> > +             nvme_tcp_teardown_ddp(queue, cqe->command_id, rq);
> > +     } else {
> > +             if (!nvme_try_complete_req(rq, cqe->status, cqe->result))
> > +                     nvme_complete_rq(rq);
> > +     }
>
> Maybe move this to nvme_tcp_complete_request as it is called from two
> code paths.

to make sure, add

void nvme_tcp_complete_request(struct request *rq, u16 status, union
nvme_result *res, u16 status)

and invoke it from the two calls sites?

> >       queue->nr_cqe++;
> >
> >       return 0;
> > @@ -871,9 +967,18 @@ static int nvme_tcp_recv_pdu(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> >   static inline void nvme_tcp_end_request(struct request *rq, u16 status)
> >   {
> >       union nvme_result res = {};
> > +     struct nvme_tcp_request *req = blk_mq_rq_to_pdu(rq);
> > +     struct nvme_tcp_queue *queue = req->queue;
> > +     struct nvme_tcp_data_pdu *pdu = (void *)queue->pdu;
> >
> > -     if (!nvme_try_complete_req(rq, cpu_to_le16(status << 1), res))
> > -             nvme_complete_rq(rq);
> > +     if (req->offloaded) {
> > +             req->status = cpu_to_le16(status << 1);
> > +             req->result = res;
> > +             nvme_tcp_teardown_ddp(queue, pdu->command_id, rq);
> > +     } else {
> > +             if (!nvme_try_complete_req(rq, cpu_to_le16(status << 1), res))
> > +                     nvme_complete_rq(rq);
> > +     }
> >   }
> >
> >   static int nvme_tcp_recv_data(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> > @@ -920,12 +1025,22 @@ static int nvme_tcp_recv_data(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> >               recv_len = min_t(size_t, recv_len,
> >                               iov_iter_count(&req->iter));
> >
> > -             if (queue->data_digest)
> > -                     ret = skb_copy_and_hash_datagram_iter(skb, *offset,
> > -                             &req->iter, recv_len, queue->rcv_hash);
> > -             else
> > -                     ret = skb_copy_datagram_iter(skb, *offset,
> > -                                     &req->iter, recv_len);
> > +             if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags)) {
> > +                     if (queue->data_digest)
> > +                             ret = skb_ddp_copy_and_hash_datagram_iter(skb, *offset,
> > +                                             &req->iter, recv_len, queue->rcv_hash);
> > +                     else
> > +                             ret = skb_ddp_copy_datagram_iter(skb, *offset,
> > +                                             &req->iter, recv_len);
> > +             } else {
> > +                     if (queue->data_digest)
> > +                             ret = skb_copy_and_hash_datagram_iter(skb, *offset,
> > +                                             &req->iter, recv_len, queue->rcv_hash);
> > +                     else
> > +                             ret = skb_copy_datagram_iter(skb, *offset,
> > +                                             &req->iter, recv_len);
> > +             }
> > +
>
> Maybe move this hunk to nvme_tcp_consume_skb or something?

ok, we can add

int nvme_tcp_consume_skb(queue, skb, offset, iter, recv_len)

and put this hunk there

> > @@ -2464,6 +2584,7 @@ static blk_status_t nvme_tcp_setup_cmd_pdu(struct nvme_ns *ns,
> >       req->data_len = blk_rq_nr_phys_segments(rq) ?
> >                               blk_rq_payload_bytes(rq) : 0;
> >       req->curr_bio = rq->bio;
> > +     req->offloaded = false;
>
> offloaded is being cleared lots of times, and I'm not clear what are
> the lifetime rules here.

replied above