RE: [PATCH v7 01/23] net: Introduce direct data placement tcp offload

[Shai Malin <smalin@nvidia.com>]

On Tue, 25 Oct 2022 at 23:39, Jakub Kicinski <kuba@kernel.org> wrote:
> On Tue, 25 Oct 2022 16:59:36 +0300 Aurelien Aptel wrote:
> > diff --git a/include/linux/netdev_features.h b/include/linux/netdev_features.h
> > index 7c2d77d75a88..bf7391aa04c7 100644
> > --- a/include/linux/netdev_features.h
> > +++ b/include/linux/netdev_features.h
> > @@ -14,7 +14,7 @@ typedef u64 netdev_features_t;
> >  enum {
> >       NETIF_F_SG_BIT,                 /* Scatter/gather IO. */
> >       NETIF_F_IP_CSUM_BIT,            /* Can checksum TCP/UDP over IPv4. */
> > -     __UNUSED_NETIF_F_1,
> > +     NETIF_F_HW_ULP_DDP_BIT,         /* ULP direct data placement offload */
> 
> Why do you need a feature bit if there is a whole caps / limit querying
> mechanism?

The caps are used for the HW device to publish the supported 
capabilities/limitation, while the feature bit is used for the DDP 
enablement "per net-device".

Disabling will be required in case that another feature which is 
mutually exclusive to the DDP is needed (as an example in the mlx case, 
CQE compress which is controlled from ethtool).

> 
> >       NETIF_F_HW_CSUM_BIT,            /* Can checksum all the packets. */
> >       NETIF_F_IPV6_CSUM_BIT,          /* Can checksum TCP/UDP over IPV6 */
> >       NETIF_F_HIGHDMA_BIT,            /* Can DMA to high memory. */
> > @@ -168,6 +168,7 @@ enum {
> >  #define NETIF_F_HW_HSR_TAG_RM        __NETIF_F(HW_HSR_TAG_RM)
> >  #define NETIF_F_HW_HSR_FWD   __NETIF_F(HW_HSR_FWD)
> >  #define NETIF_F_HW_HSR_DUP   __NETIF_F(HW_HSR_DUP)
> > +#define NETIF_F_HW_ULP_DDP   __NETIF_F(HW_ULP_DDP)
> >
> >  /* Finds the next feature with the highest number of the range of start-1 till 0.
> >   */
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index eddf8ee270e7..84554f26ad6b 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1043,6 +1043,7 @@ struct dev_ifalias {
> >
> >  struct devlink;
> >  struct tlsdev_ops;
> > +struct ulp_ddp_dev_ops;
> 
> I thought forward declarations are not required for struct members,
> are they?

Right, thanks, we will remove it.

> 
> >  struct netdev_net_notifier {
> >       struct list_head list;
> > @@ -2096,6 +2097,10 @@ struct net_device {
> >       const struct tlsdev_ops *tlsdev_ops;
> >  #endif
> >
> > +#if IS_ENABLED(CONFIG_ULP_DDP)
> > +     const struct ulp_ddp_dev_ops *ulp_ddp_ops;
> > +#endif
> 
> It's somewhat unclear to me why we add ops to struct net_device,
> rather than to ops.. can you explain?

We were trying to follow the TLS design which is similar.
Can you please clarify what do you mean by "rather than to ops.."?

> 
> >       const struct header_ops *header_ops;
> >
> >       unsigned char           operstate;
> 
> > +#include <linux/netdevice.h>
> > +#include <net/inet_connection_sock.h>
> > +#include <net/sock.h>
> > +
> > +enum ulp_ddp_type {
> > +     ULP_DDP_NVME = 1,
> 
> I think the DDP and the NVME parts should have more separation.
> 
> Are you planning to implement pure TCP placement, without NIC trying
> to also "add value" by processing whatever TCP is carrying.

We are not planning to implement pure TCP placement.
As we will present in the netdev, this is doable only if the HW is L5 aware.

> 
> Can you split the DDP and NVME harder in the API, somehow?

We can simplify the layering by using union per ulp_ddp_type.
There are no nvme structures or definitions needed in ulp_ddp.

> 
> > +};
> > +
> > +enum ulp_ddp_offload_capabilities {
> > +     ULP_DDP_C_NVME_TCP = 1,
> > +     ULP_DDP_C_NVME_TCP_DDGST_RX = 2,
> > +};
> > +
> > +/**
> > + * struct ulp_ddp_limits - Generic ulp ddp limits: tcp ddp
> > + * protocol limits.
> > + * Protocol implementations must use this as the first member.
> > + * Add new instances of ulp_ddp_limits below (nvme-tcp, etc.).
> > + *
> > + * @type:            type of this limits struct
> > + * @offload_capabilities:bitmask of supported offload types
> > + * @max_ddp_sgl_len: maximum sgl size supported (zero means no limit)
> > + * @io_threshold:    minimum payload size required to offload
> > + * @buf:             protocol-specific limits struct (if any)
> > + */
> > +struct ulp_ddp_limits {
> 
> Why is this called limits not capabilities / caps?

We will change it to caps.

> 
> > +     enum ulp_ddp_type       type;
> > +     u64                     offload_capabilities;
> > +     int                     max_ddp_sgl_len;
> > +     int                     io_threshold;
> > +     unsigned char           buf[];
> 
> Just put a union of all the protos here.

Sure.

> 
> > +};
> > +
> > +/**
> > + * struct nvme_tcp_ddp_limits - nvme tcp driver limitations
> > + *
> > + * @lmt:             generic ULP limits struct
> > + * @full_ccid_range: true if the driver supports the full CID range
> > + */
> > +struct nvme_tcp_ddp_limits {
> > +     struct ulp_ddp_limits   lmt;
> > +
> > +     bool                    full_ccid_range;
> > +};
> 
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 0640453fce54..df37db420110 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -5233,6 +5233,10 @@ tcp_collapse(struct sock *sk, struct sk_buff_head
> *list, struct rb_root *root,
> >               memcpy(nskb->cb, skb->cb, sizeof(skb->cb));
> >  #ifdef CONFIG_TLS_DEVICE
> >               nskb->decrypted = skb->decrypted;
> > +#endif
> > +#ifdef CONFIG_ULP_DDP
> > +             nskb->ulp_ddp = skb->ulp_ddp;
> > +             nskb->ulp_crc = skb->ulp_crc;
> >  #endif
> >               TCP_SKB_CB(nskb)->seq = TCP_SKB_CB(nskb)->end_seq = start;
> >               if (list)
> > @@ -5266,6 +5270,10 @@ tcp_collapse(struct sock *sk, struct sk_buff_head
> *list, struct rb_root *root,
> >  #ifdef CONFIG_TLS_DEVICE
> >                               if (skb->decrypted != nskb->decrypted)
> >                                       goto end;
> > +#endif
> > +#ifdef CONFIG_ULP_DDP
> 
> no ifdef needed

Thanks, we will remove it.

> 
> > +                             if (skb_is_ulp_crc(skb) != skb_is_ulp_crc(nskb))
> > +                                     goto end;
> >  #endif
> >                       }
> >               }