[PATCH v4 01/20] nvme-tcp-offload: Add nvme-tcp-offload - NVMeTCP HW offload ULP

[Shai Malin]

On Thu, 1 Jul 2021 at 16:34, Christoph Hellwig <hch at lst.de> wrote:
>
> > +/* Kernel includes */
>
>
> > +/* Driver includes */
>
> I think these comments are a little pointless.

Will be removed.

>
> > +int nvme_tcp_ofld_register_dev(struct nvme_tcp_ofld_dev *dev)
>
> Can you spell out offload everywhere?

Sure.

>
> > +int nvme_tcp_ofld_report_queue_err(struct nvme_tcp_ofld_queue *queue)
> > +{
> > +     /* Placeholder - invoke error recovery flow */
> > +
> > +     return 0;
> > +}
>
> Please don't add any placeholders like this.  The whole file is
> still pretty small with all the patches applied, so no need to split it.

Sure.

>
> > +static struct nvmf_transport_ops nvme_tcp_ofld_transport = {
> > +     .name           = "tcp_offload",
> > +     .module         = THIS_MODULE,
> > +     .required_opts  = NVMF_OPT_TRADDR,
> > +     .allowed_opts   = NVMF_OPT_TRSVCID | NVMF_OPT_NR_WRITE_QUEUES  |
> > +                       NVMF_OPT_HOST_TRADDR | NVMF_OPT_CTRL_LOSS_TMO |
> > +                       NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_HDR_DIGEST |
> > +                       NVMF_OPT_DATA_DIGEST | NVMF_OPT_NR_POLL_QUEUES |
> > +                       NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
> > +};
> > +
> > +static int __init nvme_tcp_ofld_init_module(void)
> > +{
> > +     nvmf_register_transport(&nvme_tcp_ofld_transport);
> > +
> > +     return 0;
> > +}
> > +
> > +static void __exit nvme_tcp_ofld_cleanup_module(void)
> > +{
> > +     nvmf_unregister_transport(&nvme_tcp_ofld_transport);
> > +}
>
> Looking at the final result this doesn't do much.  Assuming we want
> to support these kinds of whacky offloads (which I'd rather not do),
> the proper way would be to allow registering multiple transport_ops
> structures for a given name rather adding an indirection that duplicates
> a whole lot of code.

In that case, would you prefer that we invoke the tcp-offload from
within the tcp flow?
Should it be with the same transport name (“tcp”) or with a different
transport name (“tcp_offload”)?

Also, would you prefer that we register the offload device driver
directly to blk_mq layer or through the tcp-offload layer?

>
> > +     /* Offload device specific driver context */
> > +     int num_hw_vectors;
> > +};
>
> As far as I can tell this is about queues, not "vectors" of some kind.

It's the num irqs which the device support. We can rename it "num_hw_queues".

>
> > +struct nvme_tcp_ofld_req {
> > +     struct nvme_request req;
> > +     struct nvme_command nvme_cmd;
> > +     struct list_head queue_entry;
> > +     struct nvme_tcp_ofld_queue *queue;
> > +
> > +     /* Offload device specific driver context */
> > +     void *private_data;
> > +
> > +     /* async flag is used to distinguish between async and IO flow
> > +      * in common send_req() of nvme_tcp_ofld_ops.
> > +      */
> > +     bool async;
> > +
> > +     void (*done)(struct nvme_tcp_ofld_req *req,
> > +                  union nvme_result *result,
> > +                  __le16 status);
>
> This always points to nvme_tcp_ofld_req_done, why the costly indirection?

Will be fixed.

>
> > +     /* Error callback function */
> > +     int (*report_err)(struct nvme_tcp_ofld_queue *queue);
> > +};
>
> This seems to always point to nvme_tcp_ofld_report_queue_err, why the
> indirection?

Will be fixed.