[PATCH v15 05/20] nvme-tcp: Add DDP offload control path

[Aurelien Aptel]

Sagi Grimberg <sagi at grimberg.me> writes:
>> +     if (test_bit(NVME_TCP_Q_OFF_DDP, &queue->flags))
>> +             nvme_tcp_unoffload_socket(queue);
>> +#ifdef CONFIG_ULP_DDP
>> +     if (nvme_tcp_admin_queue(queue) && queue->ctrl->ddp_netdev) {
>> +             /* put back ref from get_netdev_for_sock() */
>> +             dev_put(queue->ctrl->ddp_netdev);
>> +             queue->ctrl->ddp_netdev = NULL;
>> +     }
>> +#endif
>
> Lets avoid spraying these ifdefs in the code.
> the ddp_netdev struct member can be lifted out of the ifdef I think
> because its only controller-wide.
>

Ok, we will remove the ifdefs.

>> +#ifdef CONFIG_ULP_DDP
>> +             /*
>> +              * Admin queue takes a netdev ref here, and puts it
>> +              * when the queue is stopped in __nvme_tcp_stop_queue().
>> +              */
>> +             ctrl->ddp_netdev = get_netdev_for_sock(queue->sock->sk);
>> +             if (ctrl->ddp_netdev) {
>> +                     if (nvme_tcp_ddp_query_limits(ctrl)) {
>> +                             nvme_tcp_ddp_apply_limits(ctrl);
>> +                     } else {
>> +                             dev_put(ctrl->ddp_netdev);
>> +                             ctrl->ddp_netdev = NULL;
>> +                     }
>> +             } else {
>> +                     dev_info(nctrl->device, "netdev not found\n");
>
> Would prefer to not print offload specific messages in non-offload code
> paths. at best, dev_dbg.

Sure, we will switch to dev_dbg.

> If the netdev is derived by the sk, why does the interface need a netdev
> at all? why not just pass sk and derive the netdev from the sk behind
> the interface?
>
> Or is there a case that I'm not seeing here?

If we derive the netdev from the socket, it would be too costly to call
get_netdev_for_sock() which takes a lock on the data path.

We could store it in the existing sk->ulp_ddp_ctx, assigning it in
sk_add and accessing it in sk_del/setup/teardown/resync.
But we would run into the problem of not being sure
get_netdev_for_sock() returned the same device in query_limits() and
sk_add() because we did not keep a pointer to it.

We believe it would be more complex to deal with these problems than to
just keep a reference to the netdev in the nvme-tcp controller.

Thanks