Re: [PATCH v17 01/20] net: Introduce direct data placement tcp offload

Jiri Pirko
2023-10-24T14:18:47Z

Tue, Oct 24, 2023 at 02:54:26PM CEST, aaptel@nvidia.com wrote:
>From: Boris Pismenny <borisp@nvidia.com>

[...]

>@@ -2134,6 +2146,9 @@ struct net_device {
> netdev_features_t mpls_features;
> netdev_features_t gso_partial_features;
>
>+#ifdef CONFIG_ULP_DDP
>+ struct ulp_ddp_netdev_caps ulp_ddp_caps;

Why can't you have this inside the driver? You have set_caps/get_stats
ops. Try to avoid netdev struct pollution.

>+#endif
> unsigned int min_mtu;
> unsigned int max_mtu;
> unsigned short type;

[...]

>+/**
>+ * struct netlink_ulp_ddp_stats - ULP DDP offload statistics
>+ * @rx_nvmeotcp_sk_add: number of sockets successfully prepared for offloading.
>+ * @rx_nvmeotcp_sk_add_fail: number of sockets that failed to be prepared
>+ * for offloading.
>+ * @rx_nvmeotcp_sk_del: number of sockets where offloading has been removed.
>+ * @rx_nvmeotcp_ddp_setup: number of NVMeTCP PDU successfully prepared for
>+ * Direct Data Placement.
>+ * @rx_nvmeotcp_ddp_setup_fail: number of PDUs that failed DDP preparation.
>+ * @rx_nvmeotcp_ddp_teardown: number of PDUs done with DDP.
>+ * @rx_nvmeotcp_drop: number of PDUs dropped.
>+ * @rx_nvmeotcp_resync: number of resync.
>+ * @rx_nvmeotcp_packets: number of offloaded PDUs.
>+ * @rx_nvmeotcp_bytes: number of offloaded bytes.
>+ */
>+struct netlink_ulp_ddp_stats {

There is nothing "netlink" about this. Just stats. Exposed over netlink,
yes, but that does not need the prefix.

>+ u64 rx_nvmeotcp_sk_add;
>+ u64 rx_nvmeotcp_sk_add_fail;
>+ u64 rx_nvmeotcp_sk_del;
>+ u64 rx_nvmeotcp_ddp_setup;
>+ u64 rx_nvmeotcp_ddp_setup_fail;
>+ u64 rx_nvmeotcp_ddp_teardown;
>+ u64 rx_nvmeotcp_drop;
>+ u64 rx_nvmeotcp_resync;
>+ u64 rx_nvmeotcp_packets;
>+ u64 rx_nvmeotcp_bytes;
>+
>+ /*
>+ * add new stats at the end and keep in sync with
>+ * Documentation/netlink/specs/ulp_ddp.yaml
>+ */
>+};

[...]

>+++ b/include/net/ulp_ddp_caps.h
>@@ -0,0 +1,42 @@
>+/* SPDX-License-Identifier: GPL-2.0
>+ *
>+ * ulp_ddp.h
>+ * Author: Aurelien Aptel <aaptel@nvidia.com>
>+ * Copyright (c) 2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>+ */
>+#ifndef _ULP_DDP_CAPS_H
>+#define _ULP_DDP_CAPS_H
>+
>+#include <linux/types.h>
>+
>+enum {
>+ ULP_DDP_C_NVME_TCP_BIT,
>+ ULP_DDP_C_NVME_TCP_DDGST_RX_BIT,
>+
>+ /*
>+ * add capabilities above and keep in sync with
>+ * Documentation/netlink/specs/ulp_ddp.yaml

Wait what? Why do you need this at all? Just use the uapi enum.

>+ */
>+ ULP_DDP_C_COUNT,
>+};
>+ …