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

[Jiri Pirko <jiri@resnulli.us>]

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,
>+};
>+

[...]