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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <253edj2h20j.fsf@mtr-vdi-124.i-did-not-set--mail-host-address--so-tickle-me>
Date: Wed, 13 Sep 2023 12:10:36 +0300
From: Aurelien Aptel <aaptel@...dia.com>
To: Sagi Grimberg <sagi@...mberg.me>, linux-nvme@...ts.infradead.org,
 netdev@...r.kernel.org, hch@....de, kbusch@...nel.org, axboe@...com,
 chaitanyak@...dia.com, davem@...emloft.net, kuba@...nel.org
Cc: Boris Pismenny <borisp@...dia.com>, aurelien.aptel@...il.com,
 smalin@...dia.com, malin1024@...il.com, ogerlitz@...dia.com,
 yorayz@...dia.com, galshalom@...dia.com, mgurtovoy@...dia.com
Subject: Re: [PATCH v15 05/20] nvme-tcp: Add DDP offload control path

Sagi Grimberg <sagi@...mberg.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