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

[Or Gerlitz]

On Sun, Feb 14, 2021 at 8:30 PM David Ahern <dsahern at gmail.com> wrote:
>
> On 2/11/21 2:10 PM, Boris Pismenny 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)) {
> > +             dev_info_ratelimited(queue->ctrl->ctrl.device, "netdev not found\n");
>
> again, unnecessary. you only get here because the rquest is marked
> offloaded and that only happens if the netdev exists and supports DDP.
>
> > +             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);
> > +     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, req->status, req->result))
> > +             nvme_complete_rq(rq);
> > +}
> > +
> > +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;
> > +
> > +     if (unlikely(!netdev)) {
> > +             dev_info_ratelimited(queue->ctrl->ctrl.device, "netdev not found\n");
>
> similarly here. you can't get here if netdev is null.
>
> > +             return -EINVAL;
> > +     }
> > +
> > +     req->ddp.command_id = command_id;
> > +     ret = nvme_tcp_req_map_sg(req, rq);
> > +     if (ret)
> > +             return -ENOMEM;
> > +
> > +     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)
> >  {
> >       struct net_device *netdev = queue->ctrl->offloading_netdev;
> > @@ -343,7 +417,7 @@ static void nvme_tcp_resync_response(struct nvme_tcp_queue *queue,
> >               return;
> >
> >       if (unlikely(!netdev)) {
> > -             pr_info_ratelimited("%s: netdev not found\n", __func__);
> > +             dev_info_ratelimited(queue->ctrl->ctrl.device, "netdev not found\n");
>
> and per comment on the last patch, this is not needed.
>
> > @@ -849,10 +953,39 @@ 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)
> >  {
> > +     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;
> >       union nvme_result res = {};
> >
> > -     if (!nvme_try_complete_req(rq, cpu_to_le16(status << 1), res))
> > -             nvme_complete_rq(rq);
> > +     nvme_tcp_complete_request(rq, cpu_to_le16(status << 1), res, pdu->command_id);
> > +}
> > +
> > +
> > +static int nvme_tcp_consume_skb(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> > +                             unsigned int *offset, struct iov_iter *iter, int recv_len)
> > +{
> > +     int ret;
> > +
> > +#ifdef CONFIG_TCP_DDP
> > +     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags)) {
> > +             if (queue->data_digest)
> > +                     ret = skb_ddp_copy_and_hash_datagram_iter(skb, *offset, iter, recv_len,
> > +                                     queue->rcv_hash);
> > +             else
> > +                     ret = skb_ddp_copy_datagram_iter(skb, *offset, iter, recv_len);
> > +     } else {
> > +#endif
>
> why not make that a helper defined in the CONFIG_TCP_DDP section with an
> inline for the unset case. Keeps this code from being polluted with the
> ifdef checks.

will check that

>
> > +             if (queue->data_digest)
> > +                     ret = skb_copy_and_hash_datagram_iter(skb, *offset, iter, recv_len,
> > +                                     queue->rcv_hash);
> > +             else
> > +                     ret = skb_copy_datagram_iter(skb, *offset, iter, recv_len);
> > +#ifdef CONFIG_TCP_DDP
> > +     }
> > +#endif
> > +
> > +     return ret;
> >  }
> >
> >  static int nvme_tcp_recv_data(struct nvme_tcp_queue *queue, struct sk_buff *skb,
> > @@ -899,12 +1032,7 @@ 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);
> > +             ret = nvme_tcp_consume_skb(queue, skb, offset, &req->iter, recv_len);
> >               if (ret) {
> >                       dev_err(queue->ctrl->ctrl.device,
> >                               "queue %d failed to copy request %#x data",
> > @@ -1128,6 +1256,7 @@ static int nvme_tcp_try_send_cmd_pdu(struct nvme_tcp_request *req)
> >       bool inline_data = nvme_tcp_has_inline_data(req);
> >       u8 hdgst = nvme_tcp_hdgst_len(queue);
> >       int len = sizeof(*pdu) + hdgst - req->offset;
> > +     struct request *rq = blk_mq_rq_from_pdu(req);
> >       int flags = MSG_DONTWAIT;
> >       int ret;
> >
> > @@ -1136,6 +1265,10 @@ static int nvme_tcp_try_send_cmd_pdu(struct nvme_tcp_request *req)
> >       else
> >               flags |= MSG_EOR;
> >
> > +     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags) &&
> > +         blk_rq_nr_phys_segments(rq) && rq_data_dir(rq) == READ)
> > +             nvme_tcp_setup_ddp(queue, pdu->cmd.common.command_id, rq);
> > +
>
> For consistency, shouldn't this be wrapped in the CONFIG_TCP_DDP check too?

We tried to avoid the wrapping in some places where it was
possible to do without adding confusion, this one is a good
example IMOH.


> >       if (queue->hdr_digest && !req->offset)
> >               nvme_tcp_hdgst(queue->snd_hash, pdu, sizeof(*pdu));
> >