From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:04:41
From: Boris Pismenny <redacted>
Changes since v4:
=========================================
* Add transmit offload patches
* Use one feature bit for both receive and transmit offload
Changes since v3:
=========================================
* Use DDP_TCP ifdefs in iov_iter and skb iterators to minimize impact
when compiled out (Christoph)
* Simplify netdev references and reduce the use of
get_netdev_for_sock (Sagi)
* Avoid "static" in it's own line, move it one line down (Christoph)
* Pass (queue, skb, *offset) and retrieve the pdu_seq in
nvme_tcp_resync_response (Sagi)
* Add missing assignment of offloading_netdev to null in offload_limits
error case (Sagi)
* Set req->offloaded = false once -- the lifetime rules are:
set to false on cmd_setup / set to true when ddp setup succeeds (Sagi)
* Replace pr_info_ratelimited with dev_info_ratelimited (Sagi)
* Add nvme_tcp_complete_request and invoke it from two similar call
sites (Sagi)
* Introduce nvme_tcp_req_map_sg earlier in the series (Sagi)
* Add nvme_tcp_consume_skb and put into it a hunk from
nvme_tcp_recv_data to handle copy with and without offload
Changes since v2:
=========================================
* Use skb->ddp_crc for copy offload to avoid skb_condense
* Default mellanox driver support to no (experimental feature)
* In iov_iter use non-ddp functions for kvec and iovec
* Remove typecasting in nvme-tcp
Changes since v1:
=========================================
* Rework iov_iter copy skip if src==dst to be less intrusive (David Ahern)
* Add tcp-ddp documentation (David Ahern)
* Refactor mellanox driver patches into more patches (Saeed Mahameed)
* Avoid pointer casting (David Ahern)
* Rename nvme-tcp offload flags (Shai Malin)
* Update cover-letter according to the above
Changes since RFC v1:
=========================================
* Split mlx5 driver patches to several commits
* Fix nvme-tcp handling of recovery flows. In particular, move queue offlaod
init/teardown to the start/stop functions.
# Overview
=========================================
This series adds support for nvme-tcp receive and transmit offloads
which do not mandate the offload of the network stack to the device.
Instead, these work together with TCP to offload:
1. copy from SKB to the block layer buffers
2. CRC calculation and verification for received PDU
The series implements these as a generic offload infrastructure for storage
protocols, which we call TCP Direct Data Placement (TCP_DDP) and TCP DDP CRC,
respectively. We use this infrastructure to implement NVMe-TCP offload for copy
and CRC. Future implementations can reuse the same infrastructure for other
protcols such as iSCSI.
Note:
These offloads are similar in nature to the packet-based NIC TLS offloads,
which are already upstream (see net/tls/tls_device.c).
You can read more about TLS offload here:
https://www.kernel.org/doc/html/latest/networking/tls-offload.html
# Initialization and teardown:
=========================================
The offload for IO queues is initialized after the handshake of the
NVMe-TCP protocol is finished by calling `nvme_tcp_offload_socket`
with the tcp socket of the nvme_tcp_queue:
This operation sets all relevant hardware contexts in
hardware. If it fails, then the IO queue proceeds as usually with no offload.
If it succeeds then `nvme_tcp_setup_ddp` and `nvme_tcp_teardown_ddp` may be
called to perform copy offload, and crc offload will be used.
This initialization does not change the normal operation of nvme-tcp in any
way besides adding the option to call the above mentioned NDO operations.
For the admin queue, nvme-tcp does not initialize the offload.
Instead, nvme-tcp calls the driver to configure limits for the controller,
such as max_hw_sectors and max_segments; these must be limited to accomodate
potential HW resource limits, and to improve performance.
If some error occured, and the IO queue must be closed or reconnected, then
offload is teardown and initialized again. Additionally, we handle netdev
down events via the existing error recovery flow.
# Copy offload works as follows:
=========================================
The nvme-tcp layer calls the NIC drive to map block layer buffers to ccid using
`nvme_tcp_setup_ddp` before sending the read request. When the repsonse is
received, then the NIC HW will write the PDU payload directly into the
designated buffer, and build an SKB such that it points into the destination
buffer; this SKB represents the entire packet received on the wire, but it
points to the block layer buffers. Once nvme-tcp attempts to copy data from
this SKB to the block layer buffer it can skip the copy by checking in the
copying function (memcpy_to_page):
if (src == dst) -> skip copy
Finally, when the PDU has been processed to completion, the nvme-tcp layer
releases the NIC HW context be calling `nvme_tcp_teardown_ddp` which
asynchronously unmaps the buffers from NIC HW.
As the copy skip change is in a sensative function, we are careful to avoid
changing it. To that end, we create alternative skb copy and hash iterators
that skip copy/hash if (src == dst). Nvme-tcp is the first user for these.
# Asynchronous completion:
=========================================
The NIC must release its mapping between command IDs and the target buffers.
This mapping is released when NVMe-TCP calls the NIC
driver (`nvme_tcp_offload_socket`).
As completing IOs is performance criticial, we introduce asynchronous
completions for NVMe-TCP, i.e. NVMe-TCP calls the NIC, which will later
call NVMe-TCP to complete the IO (`nvme_tcp_ddp_teardown_done`).
An alternative approach is to move all the functions related to coping from
SKBs to the block layer buffers inside the nvme-tcp code - about 200 LOC.
# CRC receive offload works as follows:
=========================================
After offload is initialized, we use the SKB's ddp_crc bit to indicate that:
"there was no problem with the verification of all CRC fields in this packet's
payload". The bit is set to zero if there was an error, or if HW skipped
offload for some reason. If *any* SKB in a PDU has (ddp_crc != 1), then software
must compute the CRC, and check it. We perform this check, and
accompanying software fallback at the end of the processing of a received PDU.
# CRC transmit offload works as follows:
=========================================
The sending layer (e.g., nvme-tcp) sets the MSG_DDP_CRC when sending messages
down to TCP. Thereafter TCP will mark corresponding SKBs with the ddp_crc bit.
This ensures that CRC offload takes place for this packets, similarly to how
skb->decrtypted is used for TLS.
Additionally, nvme-tcp maintains a mapping between TCP sequence numbers
and PDU data which allows the driver to handle reordered/retransmitted
packet offload by resynchronizing the device's CRC state.
# SKB changes:
=========================================
The CRC offload requires an additional bit in the SKB, which is useful for
preventing the coalescing of SKB with different crc offload values. This bit
is similar in concept to the "decrypted" bit.
# Performance:
=========================================
The expected performance gain from this offload varies with the block size.
We perform a CPU cycles breakdown of the copy/CRC operations in nvme-tcp
fio random read workloads:
For 4K blocks we see up to 11% improvement for a 100% read fio workload,
while for 128K blocks we see upto 52%. If we run nvme-tcp, and skip these
operations, then we observe a gain of about 1.1x and 2x respectively.
# Resynchronization:
=========================================
The resynchronization flow is performed to reset the hardware tracking of
NVMe-TCP PDUs within the TCP stream. The flow consists of a request from
the driver, regarding a possible location of a PDU header. Followed by
a response from the nvme-tcp driver.
This flow is rare, and it should happen only after packet loss or
reordering events that involve nvme-tcp PDU headers.
# The patches are organized as follows:
=========================================
Patches 1,3 the infrastructure for all TCP DDP.
and TCP DDP CRC offloads, respectively.
Patch 2 the iov_iter change to skip copy if (src == dst).
Patch 4 exposes the get_netdev_for_sock function from TLS.
Patch 5 NVMe-TCP changes to call NIC driver on queue init/teardown.
Patches 6 NVMe-TCP changes to call NIC driver on IO operation.
setup/teardown, and support async completions.
Patches 7 NVMe-TCP changes to support CRC offload on receive.
Also, this patch moves CRC calculation to the end of PDU
in case offload requires software fallback.
Patches 8 NVMe-TCP handling of netdev events: stop the offload if
netdev is going down.
Patches 9-19 implement support for NVMe-TCP copy and CRC offload in
the mlx5 NIC driver as the first user.
Patches 20 Document TCP DDP offload.
Patches 21-24 Net core support for transmit offload
Patches 25-26 NVMe-TCP transmit offload support
Patches 27-36 Mellanox NVMe-TCP transmit offload support
Testing:
=========================================
This series was tested using fio with various configurations of IO sizes,
depths, MTUs, and with both the SPDK and kernel NVMe-TCP targets.
Also, we have used QEMU and gate-level simulation to verify these patches.
Future work:
=========================================
A follow-up series will introduce support for TLS in NVMe-TCP and combining the
two offloads.
Ben Ben-Ishay (8):
net/mlx5e: NVMEoTCP offload initialization
net/mlx5e: KLM UMR helper macros
net/mlx5e: NVMEoTCP use KLM UMRs
net/mlx5e: NVMEoTCP queue init/teardown
net/mlx5e: NVMEoTCP async ddp invalidation
net/mlx5e: NVMEoTCP ddp setup and resync
net/mlx5e: NVMEoTCP, data-path for DDP+DDGST offload
net/mlx5e: NVMEoTCP statistics
Ben Ben-ishay (2):
net/mlx5: Header file changes for nvme-tcp offload
net/mlx5: Add 128B CQE for NVMEoTCP offload
Boris Pismenny (8):
net: Introduce direct data placement tcp offload
iov_iter: DDP copy to iter/pages
net: skb copy(+hash) iterators for DDP offloads
net/tls: expose get_netdev_for_sock
nvme-tcp: Add DDP offload control path
nvme-tcp: Add DDP data-path
net/mlx5e: TCP flow steering for nvme-tcp
Documentation: add ULP DDP offload documentation
Or Gerlitz (1):
nvme-tcp: Deal with netdevice DOWN events
Yoray Zack (17):
nvme-tcp: RX DDGST offload
net: drop ULP DDP HW offload feature if no CSUM offload feature
net: Add ulp_ddp_pdu_info struct
net: Add to ulp_ddp support for fallback flow
net: Add MSG_DDP_CRC flag
nvme-tcp: TX DDGST offload
nvme-tcp: Mapping between Tx NVMEoTCP pdu and TCP sequence
mlx5e: make preparation in TLS code for NVMEoTCP CRC Tx offload
mlx5: Add sq state test bit for nvmeotcp
mlx5: Add support to NETIF_F_HW_TCP_DDP_CRC_TX feature
net/mlx5e: NVMEoTCP DDGST TX offload TIS
net/mlx5e: NVMEoTCP DDGST Tx offload queue init/teardown
net/mlx5e: NVMEoTCP DDGST TX BSF and PSV
net/mlx5e: NVMEoTCP DDGST TX Data path
net/mlx5e: NVMEoTCP DDGST TX handle OOO packets
net/mlx5e: NVMEoTCP DDGST TX offload optimization
net/mlx5e: NVMEoTCP DDGST TX statistics
Documentation/networking/index.rst | 1 +
Documentation/networking/ulp-ddp-offload.rst | 415 +++++
.../net/ethernet/mellanox/mlx5/core/Kconfig | 10 +
.../net/ethernet/mellanox/mlx5/core/Makefile | 2 +
drivers/net/ethernet/mellanox/mlx5/core/en.h | 36 +-
.../net/ethernet/mellanox/mlx5/core/en/fs.h | 4 +-
.../ethernet/mellanox/mlx5/core/en/params.c | 11 +-
.../ethernet/mellanox/mlx5/core/en/params.h | 3 +
.../net/ethernet/mellanox/mlx5/core/en/txrx.h | 20 +-
.../ethernet/mellanox/mlx5/core/en/xsk/rx.c | 1 +
.../ethernet/mellanox/mlx5/core/en/xsk/rx.h | 1 +
.../mellanox/mlx5/core/en_accel/en_accel.h | 22 +-
.../mellanox/mlx5/core/en_accel/fs_tcp.c | 10 +
.../mellanox/mlx5/core/en_accel/fs_tcp.h | 2 +-
.../mellanox/mlx5/core/en_accel/ktls_tx.c | 16 +-
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 1555 +++++++++++++++++
.../mellanox/mlx5/core/en_accel/nvmeotcp.h | 138 ++
.../mlx5/core/en_accel/nvmeotcp_rxtx.c | 264 +++
.../mlx5/core/en_accel/nvmeotcp_rxtx.h | 43 +
.../mlx5/core/en_accel/nvmeotcp_utils.h | 80 +
.../net/ethernet/mellanox/mlx5/core/en_main.c | 30 +-
.../net/ethernet/mellanox/mlx5/core/en_rx.c | 66 +-
.../ethernet/mellanox/mlx5/core/en_stats.c | 74 +
.../ethernet/mellanox/mlx5/core/en_stats.h | 47 +
.../net/ethernet/mellanox/mlx5/core/en_tx.c | 11 +
.../net/ethernet/mellanox/mlx5/core/en_txrx.c | 17 +
drivers/net/ethernet/mellanox/mlx5/core/fw.c | 6 +
drivers/nvme/host/tcp.c | 567 +++++-
include/linux/mlx5/device.h | 44 +-
include/linux/mlx5/mlx5_ifc.h | 104 +-
include/linux/mlx5/qp.h | 1 +
include/linux/netdev_features.h | 3 +-
include/linux/netdevice.h | 5 +
include/linux/skbuff.h | 13 +
include/linux/socket.h | 1 +
include/linux/uio.h | 17 +
include/net/inet_connection_sock.h | 4 +
include/net/sock.h | 23 +
include/net/ulp_ddp.h | 192 ++
lib/iov_iter.c | 55 +
net/Kconfig | 10 +
net/core/Makefile | 1 +
net/core/datagram.c | 48 +
net/core/dev.c | 2 +
net/core/skbuff.c | 8 +-
net/core/sock.c | 7 +
net/core/ulp_ddp.c | 235 +++
net/ethtool/common.c | 1 +
net/ipv4/tcp.c | 6 +
net/ipv4/tcp_input.c | 8 +
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/tcp_offload.c | 3 +
net/tls/tls_device.c | 20 +-
53 files changed, 4192 insertions(+), 74 deletions(-)
create mode 100644 Documentation/networking/ulp-ddp-offload.rst
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp_rxtx.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp_rxtx.h
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp_utils.h
create mode 100644 include/net/ulp_ddp.h
create mode 100644 net/core/ulp_ddp.c
--
2.24.1
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:04:50
From: Boris Pismenny <redacted>
This commit introduces direct data placement offload for TCP.
This capability is accompanied by new net_device operations that
configure hardware contexts. There is a context per socket, and a context per DDP
opreation. Additionally, a resynchronization routine is used to assist
hardware handle TCP OOO, and continue the offload.
Furthermore, we let the offloading driver advertise what is the max hw
sectors/segments.
Using this interface, the NIC hardware will scatter TCP payload directly
to the BIO pages according to the command_id.
To maintain the correctness of the network stack, the driver is expected
to construct SKBs that point to the BIO pages.
The SKB passed to the network stack from the driver
represents data as it is on the wire, while it is pointing
directly to data in destination buffers.
As a result, data from page frags should not be copied out to
the linear part. To avoid needless copies, such as when using
skb_condense, we mark the skb->ddp_crc bit. This bit will be
used to indicate both ddp and crc offload (next patch in series).
A follow-up patch will use this interface for DDP in NVMe-TCP.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
include/linux/netdev_features.h | 3 +-
include/linux/netdevice.h | 5 ++
include/linux/skbuff.h | 4 +
include/net/inet_connection_sock.h | 4 +
include/net/ulp_ddp.h | 136 +++++++++++++++++++++++++++++
net/Kconfig | 10 +++
net/core/skbuff.c | 8 +-
net/ethtool/common.c | 1 +
net/ipv4/tcp_input.c | 8 ++
net/ipv4/tcp_ipv4.c | 3 +
net/ipv4/tcp_offload.c | 3 +
11 files changed, 183 insertions(+), 2 deletions(-)
create mode 100644 include/net/ulp_ddp.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 */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 till 0.*/
@@ -0,0 +1,136 @@+/* SPDX-License-Identifier: GPL-2.0+*+*ulp_ddp.h+*Author:BorisPismenny<borisp@mellanox.com>+*Copyright(C)2021MellanoxTechnologies.+*/+#ifndef _ULP_DDP_H+#define _ULP_DDP_H++#include<linux/netdevice.h>+#include<net/inet_connection_sock.h>+#include<net/sock.h>++/* limits returned by the offload driver, zero means don't care */+structulp_ddp_limits{+intmax_ddp_sgl_len;+};++enumulp_ddp_type{+ULP_DDP_NVME=1,+};++/**+*structulp_ddp_config-Genericulpddpconfiguration:tcpddpIOqueue+*configimplementationsmustusethisasthefirstmember.+*Addnewinstancesofulp_ddp_configbelow(nvme-tcp,etc.).+*/+structulp_ddp_config{+enumulp_ddp_typetype;+unsignedcharbuf[];+};++/**+*structnvme_tcp_ddp_config-nvmetcpddpconfigurationforanIOqueue+*+*@pfv:pduversion(e.g.,NVME_TCP_PFV_1_0)+*@cpda:controllerpdudataalignmend(dwords,0'sbased)+*@dgst:digesttypesenabled.+*Thenetdevwilloffloadcrcifddp_crcissupported.+*@queue_size:numberofnvme-tcpIOqueueelements+*@queue_id:queueidentifier+*@cpu_io:cpucorerunningtheIOthreadforthisqueue+*/+structnvme_tcp_ddp_config{+structulp_ddp_configcfg;++u16pfv;+u8cpda;+u8dgst;+intqueue_size;+intqueue_id;+intio_cpu;+};++/**+*structulp_ddp_io-ulpddpconfigurationforanIOrequest.+*+*@command_id:identifieronthewireassociatedwiththesebuffers+*@nents:numberofentriesinthesg_table+*@sg_table:describingthebuffersforthisIOrequest+*@first_sgl:firstSGLinsg_table+*/+structulp_ddp_io{+u32command_id;+intnents;+structsg_tablesg_table;+structscatterlistfirst_sgl[SG_CHUNK_SIZE];+};++/* struct ulp_ddp_dev_ops - operations used by an upper layer protocol to configure ddp offload+*+*@ulp_ddp_limits:limitthenumberofscattergatherentriesperIO.+*thedevicedrivercanusethistolimittheresourcesallocatedperqueue.+*@ulp_ddp_sk_add:addoffloadforthequeuerepresenntedbythesocket+configpair.+*thisfunctionisusedtoconfigureeithercopy,crcorbothoffloads.+*@ulp_ddp_sk_del:removeoffloadfromthesocket,andreleaseanydevicerelatedresources.+*@ulp_ddp_setup:requestcopyoffloadforbuffersassociatedwithacommand_idinulp_ddp_io.+*@ulp_ddp_teardown:releaseoffloadresourcesassociationbetweenbuffersandcommand_idin+*ulp_ddp_io.+*@ulp_ddp_resync:respondtothedriver'sresync_request.Calledonlyifresyncissuccessful.+*/+structulp_ddp_dev_ops{+int(*ulp_ddp_limits)(structnet_device*netdev,+structulp_ddp_limits*limits);+int(*ulp_ddp_sk_add)(structnet_device*netdev,+structsock*sk,+structulp_ddp_config*config);+void(*ulp_ddp_sk_del)(structnet_device*netdev,+structsock*sk);+int(*ulp_ddp_setup)(structnet_device*netdev,+structsock*sk,+structulp_ddp_io*io);+int(*ulp_ddp_teardown)(structnet_device*netdev,+structsock*sk,+structulp_ddp_io*io,+void*ddp_ctx);+void(*ulp_ddp_resync)(structnet_device*netdev,+structsock*sk,u32seq);+};++#define ULP_DDP_RESYNC_REQ BIT(0)++/**+*structulp_ddp_ulp_ops-InterfacetoregisteruppperlayerDirectDataPlacement(DDP)TCPoffload+*/+structulp_ddp_ulp_ops{+/* NIC requests ulp to indicate if @seq is the start of a message */+bool(*resync_request)(structsock*sk,u32seq,u32flags);+/* NIC driver informs the ulp that ddp teardown is done - used for async completions*/+void(*ddp_teardown_done)(void*ddp_ctx);+};++/**+*structulp_ddp_ctx-Genericulpddpcontext:devicedriverperqueuecontextsmust+*usethisasthefirstmember.+*/+structulp_ddp_ctx{+enumulp_ddp_typetype;+unsignedcharbuf[];+};++staticinlinestructulp_ddp_ctx*ulp_ddp_get_ctx(conststructsock*sk)+{+structinet_connection_sock*icsk=inet_csk(sk);++return(__forcestructulp_ddp_ctx*)icsk->icsk_ulp_ddp_data;+}++staticinlinevoidulp_ddp_set_ctx(structsock*sk,void*ctx)+{+structinet_connection_sock*icsk=inet_csk(sk);++rcu_assign_pointer(icsk->icsk_ulp_ddp_data,ctx);+}++#endif //_ULP_DDP_H
@@ -454,4 +454,14 @@ config ETHTOOL_NETLINKnetlink.Itprovidesbetterextensibilityandsomenewfeatures,e.g.notificationmessages.+configULP_DDP+bool"ULP direct data placement offload"+defaultn+help+DirectDataPlacement(DDP)offloadenablesULP,suchas+NVMe-TCP/iSCSI,torequesttheNICtoplaceULPpayloaddata+ofacommandresponsedirectlyintokernelpageswhile+calculate/verifythedatadigestonULPPDUastheygothrough+theNIC.Thusavoidingthecostlyper-byteoverhead.+endif# if NET
@@ -6295,9 +6296,14 @@ EXPORT_SYMBOL(pskb_extract);*/voidskb_condense(structsk_buff*skb){+boolis_ddp=false;++#ifdef CONFIG_ULP_DDP+is_ddp=skb->ddp_crc;+#endifif(skb->data_len){if(skb->data_len>skb->end-skb->tail||-skb_cloned(skb))+skb_cloned(skb)||is_ddp)return;/* Nice, we can free page frag(s) right now */
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:05:03
From: Boris Pismenny <redacted>
When using direct data placement (DDP) the NIC writes some of the payload
directly to the destination buffer, and constructs SKBs such that they
point to this data. To skip copies when SKB data already resides in the
destination we use the newly introduced routines in this commit, which
check if (src == dst), and skip the copy when that's true.
As the current user for these routines is in the block layer (nvme-tcp),
then we only apply the change for bio_vec. Other routines use the normal
methods for copying.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
include/linux/uio.h | 17 ++++++++++++++
lib/iov_iter.c | 55 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:05:13
From: Boris Pismenny <redacted>
This commit introduces new functions to support direct data placement
(DDP) NIC offloads that avoid copying data from SKBs.
Later patches will use this for nvme-tcp DDP offload.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
include/linux/skbuff.h | 9 ++++++++
net/core/datagram.c | 48 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 57 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:05:47
From: Boris Pismenny <redacted>
get_netdev_for_sock is a utility that is used to obtain
the net_device structure from a connected socket.
Later patches will use this for nvme-tcp DDP and DDP DDGST offloads.
Signed-off-by: Boris Pismenny <redacted>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
---
include/net/sock.h | 17 +++++++++++++++++
net/tls/tls_device.c | 20 ++------------------
2 files changed, 19 insertions(+), 18 deletions(-)
@@ -107,22 +107,6 @@ static void tls_device_queue_ctx_destruction(struct tls_context *ctx)spin_unlock_irqrestore(&tls_device_lock,flags);}-/* We assume that the socket is already connected */-staticstructnet_device*get_netdev_for_sock(structsock*sk)-{-structdst_entry*dst=sk_dst_get(sk);-structnet_device*netdev=NULL;--if(likely(dst)){-netdev=netdev_sk_get_lowest_dev(dst->dev,sk);-dev_hold(netdev);-}--dst_release(dst);--returnnetdev;-}-staticvoiddestroy_record(structtls_record_info*record){inti;
@@ -1118,7 +1102,7 @@ int tls_set_device_offload(struct sock *sk, struct tls_context *ctx)if(skb)TCP_SKB_CB(skb)->eor=1;-netdev=get_netdev_for_sock(sk);+netdev=get_netdev_for_sock(sk,true);if(!netdev){pr_err_ratelimited("%s: netdev not found\n",__func__);rc=-EINVAL;
@@ -1194,7 +1178,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)if(ctx->crypto_recv.info.version!=TLS_1_2_VERSION)return-EOPNOTSUPP;-netdev=get_netdev_for_sock(sk);+netdev=get_netdev_for_sock(sk,true);if(!netdev){pr_err_ratelimited("%s: netdev not found\n",__func__);return-EINVAL;
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:06:11
From: Boris Pismenny <redacted>
This commit introduces direct data placement offload to NVME
TCP. There is a context per queue, which is established after the
handshake using the ulp_ddp_sk_add/del NDOs.
Additionally, a resynchronization routine is used to assist
hardware recovery from TCP OOO, and continue the offload.
Resynchronization operates as follows:
1. TCP OOO causes the NIC HW to stop the offload
2. NIC HW identifies a PDU header at some TCP sequence number,
and asks NVMe-TCP to confirm it.
This request is delivered from the NIC driver to NVMe-TCP by first
finding the socket for the packet that triggered the request, and
then finding the nvme_tcp_queue that is used by this routine.
Finally, the request is recorded in the nvme_tcp_queue.
3. When NVMe-TCP observes the requested TCP sequence, it will compare
it with the PDU header TCP sequence, and report the result to the
NIC driver (ulp_ddp_resync), which will update the HW,
and resume offload when all is successful.
Furthermore, we let the offloading driver advertise what is the max hw
sectors/segments via ulp_ddp_limits.
A follow-up patch introduces the data-path changes required for this
offload.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
drivers/nvme/host/tcp.c | 180 +++++++++++++++++++++++++++++++++++++++-
include/linux/skbuff.h | 4 +-
net/core/datagram.c | 4 +-
3 files changed, 182 insertions(+), 6 deletions(-)
@@ -219,6 +225,167 @@ static inline size_t nvme_tcp_pdu_last_send(struct nvme_tcp_request *req,returnnvme_tcp_pdu_data_left(req)<=len;}+#ifdef CONFIG_ULP_DDP++staticboolnvme_tcp_resync_request(structsock*sk,u32seq,u32flags);+staticconststructulp_ddp_ulp_opsnvme_tcp_ddp_ulp_ops={+.resync_request=nvme_tcp_resync_request,+};++staticintnvme_tcp_offload_socket(structnvme_tcp_queue*queue)+{+structnet_device*netdev=queue->ctrl->offloading_netdev;+structnvme_tcp_ddp_configconfig={};+intret;++if(!netdev||!(netdev->features&NETIF_F_HW_ULP_DDP))+return-EOPNOTSUPP;++config.cfg.type=ULP_DDP_NVME;+config.pfv=NVME_TCP_PFV_1_0;+config.cpda=0;+config.dgst=queue->hdr_digest?+NVME_TCP_HDR_DIGEST_ENABLE:0;+config.dgst|=queue->data_digest?+NVME_TCP_DATA_DIGEST_ENABLE:0;+config.queue_size=queue->queue_size;+config.queue_id=nvme_tcp_queue_id(queue);+config.io_cpu=queue->io_cpu;++dev_hold(netdev);/* put by unoffload_socket */+ret=netdev->ulp_ddp_ops->ulp_ddp_sk_add(netdev,+queue->sock->sk,+&config.cfg);+if(ret){+dev_put(netdev);+returnret;+}++inet_csk(queue->sock->sk)->icsk_ulp_ddp_ops=&nvme_tcp_ddp_ulp_ops;+if(netdev->features&NETIF_F_HW_ULP_DDP)+set_bit(NVME_TCP_Q_OFF_DDP,&queue->flags);++returnret;+}++staticvoidnvme_tcp_unoffload_socket(structnvme_tcp_queue*queue)+{+structnet_device*netdev=queue->ctrl->offloading_netdev;++if(!netdev){+dev_info_ratelimited(queue->ctrl->ctrl.device,"netdev not found\n");+return;+}++clear_bit(NVME_TCP_Q_OFF_DDP,&queue->flags);++netdev->ulp_ddp_ops->ulp_ddp_sk_del(netdev,queue->sock->sk);++inet_csk(queue->sock->sk)->icsk_ulp_ddp_ops=NULL;+dev_put(netdev);/* held by offload_socket */+}++staticintnvme_tcp_offload_limits(structnvme_tcp_queue*queue)+{+structnet_device*netdev=get_netdev_for_sock(queue->sock->sk,true);+structulp_ddp_limitslimits;+intret=0;++if(!netdev){+dev_info_ratelimited(queue->ctrl->ctrl.device,"netdev not found\n");+queue->ctrl->offloading_netdev=NULL;+return-ENODEV;+}++if((netdev->features&NETIF_F_HW_ULP_DDP)&&+netdev->ulp_ddp_ops&&+netdev->ulp_ddp_ops->ulp_ddp_limits)+ret=netdev->ulp_ddp_ops->ulp_ddp_limits(netdev,&limits);+else+ret=-EOPNOTSUPP;++if(!ret){+queue->ctrl->offloading_netdev=netdev;+dev_dbg_ratelimited(queue->ctrl->ctrl.device,+"netdev %s offload limits: max_ddp_sgl_len %d\n",+netdev->name,limits.max_ddp_sgl_len);+queue->ctrl->ctrl.max_segments=limits.max_ddp_sgl_len;+queue->ctrl->ctrl.max_hw_sectors=+limits.max_ddp_sgl_len<<(ilog2(SZ_4K)-9);+}else{+queue->ctrl->offloading_netdev=NULL;+}++/* release the device as no offload context is established yet. */+dev_put(netdev);++returnret;+}++staticvoidnvme_tcp_resync_response(structnvme_tcp_queue*queue,+structsk_buff*skb,unsignedintoffset)+{+u64pdu_seq=TCP_SKB_CB(skb)->seq+offset-queue->pdu_offset;+structnet_device*netdev=queue->ctrl->offloading_netdev;+u64pdu_val=(pdu_seq<<32)|ULP_DDP_RESYNC_REQ;+u64resync_val;+u32resync_seq;++resync_val=atomic64_read(&queue->resync_req);+/* Lower 32 bit flags. Check validity of the request */+if((resync_val&ULP_DDP_RESYNC_REQ)==0)+return;++/* Obtain and check requested sequence number: is this PDU header before the request? */+resync_seq=resync_val>>32;+if(before(pdu_seq,resync_seq))+return;++if(unlikely(!netdev)){+pr_info_ratelimited("%s: netdev not found\n",__func__);+return;+}++/**+*Theatomicoperationguraranteesthatwedon'tmissanyNICdriver+*resyncrequestssubmittedaftertheabovechecks.+*/+if(atomic64_cmpxchg(&queue->resync_req,pdu_val,+pdu_val&~ULP_DDP_RESYNC_REQ)!=atomic64_read(&queue->resync_req))+netdev->ulp_ddp_ops->ulp_ddp_resync(netdev,queue->sock->sk,pdu_seq);+}++staticboolnvme_tcp_resync_request(structsock*sk,u32seq,u32flags)+{+structnvme_tcp_queue*queue=sk->sk_user_data;++atomic64_set(&queue->resync_req,+(((uint64_t)seq<<32)|flags));++returntrue;+}++#else++staticintnvme_tcp_offload_socket(structnvme_tcp_queue*queue)+{+return-EINVAL;+}++staticvoidnvme_tcp_unoffload_socket(structnvme_tcp_queue*queue)+{}++staticintnvme_tcp_offload_limits(structnvme_tcp_queue*queue)+{+return-EINVAL;+}++staticvoidnvme_tcp_resync_response(structnvme_tcp_queue*queue,+structsk_buff*skb,unsignedintoffset)+{}++#endif+staticvoidnvme_tcp_init_iter(structnvme_tcp_request*req,unsignedintdir){
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:06:49
From: Boris Pismenny <redacted>
Introduce the NVMe-TCP DDP data-path offload.
Using this interface, the NIC hardware will scatter TCP payload directly
to the BIO pages according to the command_id in the PDU.
To maintain the correctness of the network stack, the driver is expected
to construct SKBs that point to the BIO pages.
The data-path interface contains two routines: tcp_ddp_setup/teardown.
The setup provides the mapping from command_id to the request buffers,
while the teardown removes this mapping.
For efficiency, we introduce an asynchronous nvme completion, which is
split between NVMe-TCP and the NIC driver as follows:
NVMe-TCP performs the specific completion, while NIC driver performs the
generic mq_blk completion.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
drivers/nvme/host/tcp.c | 150 ++++++++++++++++++++++++++++++++++++----
1 file changed, 138 insertions(+), 12 deletions(-)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:07:22
From: Yoray Zack <redacted>
Enable rx side of DDGST offload when supported.
At the end of the capsule, check if all the skb bits are
on, and if not recalculate the DDGST in SW and check it.
We reworked the receive-side DDGST calculation to always run
at the end, so as to keep a single flow for both offload and
non-offload. This change simplifies the code, but it may
degrade performance for non-offload DDGST calculation.
Signed-off-by: Yoray Zack <redacted>
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
---
drivers/nvme/host/tcp.c | 86 ++++++++++++++++++++++++++++++++++-------
1 file changed, 71 insertions(+), 15 deletions(-)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:08:18
From: Or Gerlitz <redacted>
For ddp setup/teardown and resync, the offloading logic
uses HW resources at the NIC driver such as SQ and CQ.
These resources are destroyed when the netdevice does down
and hence we must stop using them before the NIC driver
destroys them.
Use netdevice notifier for that matter -- offloaded connections
are stopped before the stack continues to call the NIC driver
close ndo.
We use the existing recovery flow which has the advantage
of resuming the offload once the connection is re-set.
This also buys us proper handling for the UNREGISTER event
b/c our offloading starts in the UP state, and down is always
there between up to unregister.
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Yoray Zack <redacted>
---
drivers/nvme/host/tcp.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:09:03
From: Ben Ben-ishay <redacted>
Add the necessary infrastructure for NVMEoTCP offload:
- Add nvmeocp_en + nvmeotcp_crc_en bit to the TIR for identify NVMEoTCP offload flow
And tag_buffer_id that will be used by the connected nvmeotcp_queues
- Add new CQE field that will be used to pass scattered data information to SW
- Add new capability to HCA_CAP that represnts the NVMEoTCP offload ability
Signed-off-by: Ben Ben-ishay <redacted>
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
include/linux/mlx5/device.h | 8 +++
include/linux/mlx5/mlx5_ifc.h | 101 +++++++++++++++++++++++++++++++++-
include/linux/mlx5/qp.h | 1 +
3 files changed, 107 insertions(+), 3 deletions(-)
@@ -1179,6 +1180,7 @@ enum mlx5_cap_type {MLX5_CAP_VDPA_EMULATION=0x13,MLX5_CAP_DEV_EVENT=0x14,MLX5_CAP_IPSEC,+MLX5_CAP_DEV_NVMEOTCP=0x19,MLX5_CAP_GENERAL_2=0x20,/* NUM OF CAP Types */MLX5_CAP_NUM
@@ -857,6 +870,27 @@ enum {#define MLX5_MINI_CQE_ARRAY_SIZE 8+staticinlineboolcqe_is_nvmeotcp_resync(structmlx5_cqe64*cqe)+{+return((cqe->nvmetcp>>6)&0x1);+}++staticinlineboolcqe_is_nvmeotcp_crcvalid(structmlx5_cqe64*cqe)+{+return((cqe->nvmetcp>>5)&0x1);+}++staticinlineboolcqe_is_nvmeotcp_zc(structmlx5_cqe64*cqe)+{+return((cqe->nvmetcp>>4)&0x1);+}++/* check if cqe is zc or crc or resync */+staticinlineboolcqe_is_nvmeotcp(structmlx5_cqe64*cqe)+{+return((cqe->nvmetcp>>4)&0x7);+}+staticinlineu8mlx5_get_cqe_format(structmlx5_cqe64*cqe){return(cqe->op_own>>2)&0x3;
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:13:14
From: Ben Ben-Ishay <redacted>
This commit introduce the initialization blocks for NVMEoTCP offload:
- Use 128B CQEs when NVME-TCP offload is enabled.
- Use a dedicated icosq for NVME-TCP work. This list of SQ is unique in the
sense that it is driven directly by the NVME-TCP layer to submit and
invalidate ddp requests.
- Query nvmeotcp capabilities
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
.../net/ethernet/mellanox/mlx5/core/Kconfig | 10 +
.../net/ethernet/mellanox/mlx5/core/Makefile | 2 +
drivers/net/ethernet/mellanox/mlx5/core/en.h | 11 +
.../ethernet/mellanox/mlx5/core/en/params.c | 11 +-
.../ethernet/mellanox/mlx5/core/en/params.h | 3 +
.../mellanox/mlx5/core/en_accel/en_accel.h | 9 +-
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 196 ++++++++++++++++++
.../mellanox/mlx5/core/en_accel/nvmeotcp.h | 117 +++++++++++
.../net/ethernet/mellanox/mlx5/core/en_main.c | 26 +++
.../net/ethernet/mellanox/mlx5/core/en_txrx.c | 17 ++
drivers/net/ethernet/mellanox/mlx5/core/fw.c | 6 +
11 files changed, 405 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp.h
@@ -3858,6 +3866,17 @@ static netdev_features_t mlx5e_fix_features(struct net_device *netdev,features&=~NETIF_F_RXHASH;if(netdev->features&NETIF_F_RXHASH)netdev_warn(netdev,"Disabling rxhash, not supported when CQE compress is active\n");++features&=~NETIF_F_HW_ULP_DDP;+if(netdev->features&NETIF_F_HW_ULP_DDP)+netdev_warn(netdev,"Disabling tcp-ddp offload, not supported when CQE compress is active\n");++}++if(netdev->features&NETIF_F_LRO){+features&=~NETIF_F_HW_ULP_DDP;+if(netdev->features&NETIF_F_HW_ULP_DDP)+netdev_warn(netdev,"Disabling tcp-ddp offload, not supported when LRO is active\n");}if(mlx5e_is_uplink_rep(priv)){
@@ -19,6 +20,121 @@ static const struct rhashtable_params rhash_queues = {.max_size=MAX_NVMEOTCP_QUEUES,};+staticvoid+fill_nvmeotcp_klm_wqe(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_umr_wqe*wqe,u16ccid,u32klm_entries,+u16klm_offset)+{+structscatterlist*sgl_mkey;+u32lkey,i;++lkey=queue->priv->mdev->mlx5e_res.mkey.key;+for(i=0;i<klm_entries;i++){+sgl_mkey=&queue->ccid_table[ccid].sgl[i+klm_offset];+wqe->inline_klms[i].bcount=cpu_to_be32(sgl_mkey->length);+wqe->inline_klms[i].key=cpu_to_be32(lkey);+wqe->inline_klms[i].va=cpu_to_be64(sgl_mkey->dma_address);+}++for(;i<ALIGN(klm_entries,KLM_ALIGNMENT);i++){+wqe->inline_klms[i].bcount=0;+wqe->inline_klms[i].key=0;+wqe->inline_klms[i].va=0;+}+}++staticvoid+build_nvmeotcp_klm_umr(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_umr_wqe*wqe,u16ccid,intklm_entries,+u32klm_offset,u32len)+{+u32id=queue->ccid_table[ccid].klm_mkey.key;+structmlx5_wqe_umr_ctrl_seg*ucseg=&wqe->uctrl;+structmlx5_wqe_ctrl_seg*cseg=&wqe->ctrl;+structmlx5_mkey_seg*mkc=&wqe->mkc;++u32sqn=queue->sq->icosq.sqn;+u16pc=queue->sq->icosq.pc;++cseg->opmod_idx_opcode=cpu_to_be32((pc<<MLX5_WQE_CTRL_WQE_INDEX_SHIFT)|+MLX5_OPCODE_UMR);+cseg->qpn_ds=cpu_to_be32((sqn<<MLX5_WQE_CTRL_QPN_SHIFT)|+MLX5E_KLM_UMR_DS_CNT(ALIGN(klm_entries,KLM_ALIGNMENT)));+cseg->general_id=cpu_to_be32(id);++if(!klm_offset){+ucseg->mkey_mask|=cpu_to_be64(MLX5_MKEY_MASK_XLT_OCT_SIZE|+MLX5_MKEY_MASK_LEN|MLX5_MKEY_MASK_FREE);+mkc->xlt_oct_size=cpu_to_be32(ALIGN(len,KLM_ALIGNMENT));+mkc->len=cpu_to_be64(queue->ccid_table[ccid].size);+}++ucseg->flags=MLX5_UMR_INLINE|MLX5_UMR_TRANSLATION_OFFSET_EN;+ucseg->xlt_octowords=cpu_to_be16(ALIGN(klm_entries,KLM_ALIGNMENT));+ucseg->xlt_offset=cpu_to_be16(klm_offset);+fill_nvmeotcp_klm_wqe(queue,wqe,ccid,klm_entries,klm_offset);+}++staticvoid+mlx5e_nvmeotcp_fill_wi(structmlx5e_nvmeotcp_queue*nvmeotcp_queue,+structmlx5e_icosq*sq,u32wqe_bbs,u16pi)+{+structmlx5e_icosq_wqe_info*wi=&sq->db.wqe_info[pi];++wi->num_wqebbs=wqe_bbs;+wi->wqe_type=MLX5E_ICOSQ_WQE_UMR_NVME_TCP;+}++staticvoid+post_klm_wqe(structmlx5e_nvmeotcp_queue*queue,+u16ccid,+u32klm_length,+u32*klm_offset)+{+structmlx5e_icosq*sq=&queue->sq->icosq;+u32wqe_bbs,cur_klm_entries;+structmlx5e_umr_wqe*wqe;+u16pi,wqe_sz;++cur_klm_entries=min_t(int,queue->max_klms_per_wqe,+klm_length-*klm_offset);+wqe_sz=MLX5E_KLM_UMR_WQE_SZ(ALIGN(cur_klm_entries,KLM_ALIGNMENT));+wqe_bbs=DIV_ROUND_UP(wqe_sz,MLX5_SEND_WQE_BB);+pi=mlx5e_icosq_get_next_pi(sq,wqe_bbs);+wqe=MLX5E_NVMEOTCP_FETCH_KLM_WQE(sq,pi);+mlx5e_nvmeotcp_fill_wi(queue,sq,wqe_bbs,pi);+build_nvmeotcp_klm_umr(queue,wqe,ccid,cur_klm_entries,*klm_offset,+klm_length);+*klm_offset+=cur_klm_entries;+sq->pc+=wqe_bbs;+sq->doorbell_cseg=&wqe->ctrl;+}++staticint+mlx5e_nvmeotcp_post_klm_wqe(structmlx5e_nvmeotcp_queue*queue,+u16ccid,+u32klm_length)+{+u32klm_offset=0,wqes,wqe_sz,max_wqe_bbs,i,room;+structmlx5e_icosq*sq=&queue->sq->icosq;++/* TODO: set stricter wqe_sz; using max for now */+wqes=DIV_ROUND_UP(klm_length,queue->max_klms_per_wqe);+wqe_sz=MLX5E_KLM_UMR_WQE_SZ(queue->max_klms_per_wqe);++max_wqe_bbs=DIV_ROUND_UP(wqe_sz,MLX5_SEND_WQE_BB);++room=mlx5e_stop_room_for_wqe(max_wqe_bbs)*wqes;+if(unlikely(!mlx5e_wqc_has_room_for(&sq->wq,sq->cc,sq->pc,room)))+return-ENOSPC;++for(i=0;i<wqes;i++)+post_klm_wqe(queue,ccid,klm_length,&klm_offset);++mlx5e_notify_hw(&sq->wq,sq->pc,sq->uar_map,sq->doorbell_cseg);+return0;+}+staticintmlx5e_nvmeotcp_offload_limits(structnet_device*netdev,structulp_ddp_limits*limits)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:18:05
From: Ben Ben-Ishay <redacted>
When nvme-tcp establishes new connections, we allocate a hardware
context to offload operations for this queue:
- Use a separate TIR to identify the queue and maintain the HW context
- Use a separate ICOSQ for maintain the HW context
- Use a separate tag buffer for buffer registration
- Maintain static and progress HW contexts by posting the proper
WQEs at creation time, or upon resync
Queue teardown will free the corresponding contexts.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
.../net/ethernet/mellanox/mlx5/core/en/txrx.h | 6 +
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 663 +++++++++++++++++-
.../mellanox/mlx5/core/en_accel/nvmeotcp.h | 4 +
.../mlx5/core/en_accel/nvmeotcp_utils.h | 68 ++
.../net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +
5 files changed, 723 insertions(+), 25 deletions(-)
@@ -400,8 +414,13 @@ mlx5e_nvmeotcp_post_klm_wqe(struct mlx5e_nvmeotcp_queue *queue,structmlx5e_icosq*sq=&queue->sq->icosq;/* TODO: set stricter wqe_sz; using max for now */-wqes=DIV_ROUND_UP(klm_length,queue->max_klms_per_wqe);-wqe_sz=MLX5E_KLM_UMR_WQE_SZ(queue->max_klms_per_wqe);+if(klm_length==0){+wqes=1;+wqe_sz=MLX5E_NVMEOTCP_STATIC_PARAMS_WQEBBS;+}else{+wqes=DIV_ROUND_UP(klm_length,queue->max_klms_per_wqe);+wqe_sz=MLX5E_KLM_UMR_WQE_SZ(queue->max_klms_per_wqe);+}max_wqe_bbs=DIV_ROUND_UP(wqe_sz,MLX5_SEND_WQE_BB);
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:22:19
From: Ben Ben-Ishay <redacted>
NVMEoTCP offload uses buffer registration for every NVME request to
perform direct data placement, The registration is done via KLM UMR
WQE's. The driver resync handler advertise the software resync response
via static params WQE.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 29 +++++++++++++++++++
1 file changed, 29 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:24:16
From: Ben Ben-Ishay <redacted>
This patch implements the data-path for direct data placement (DDP)
and DDGST offloads. NVMEoTCP DDP constructs an SKB from each CQE, while
pointing at NVME destination buffers. In turn, this enables the offload,
as the NVMe-TCP layer will skip the copy when src == dst.
Additionally, this patch adds support for DDGST (CRC32) offload.
HW will report DDGST offload only if it has not encountered an error
in the received packet. We pass this indication in skb->ddp_crc
up the stack to NVMe-TCP to skip computing the DDGST if all
corresponding SKBs were verified by HW.
This patch also handles context resynchronization requests made by
NIC HW. The resync request is passed to the NVMe-TCP layer
to be handled at a later point in time.
Finally, we also use the skb->ddp_crc bit to avoid skb_condense.
This is critical as every SKB that uses DDP has a hole that fits
perfectly with skb_condense's policy, but filling this hole is
counter-productive as the data there already resides in its
destination buffer.
Signed-off-by: Boris Pismenny <redacted>
Signed-off-by: Ben Ben-Ishay <redacted>
Signed-off-by: Or Gerlitz <redacted>
Signed-off-by: Yoray Zack <redacted>
---
.../net/ethernet/mellanox/mlx5/core/Makefile | 2 +-
drivers/net/ethernet/mellanox/mlx5/core/en.h | 1 +
.../ethernet/mellanox/mlx5/core/en/xsk/rx.c | 1 +
.../ethernet/mellanox/mlx5/core/en/xsk/rx.h | 1 +
.../mlx5/core/en_accel/nvmeotcp_rxtx.c | 248 ++++++++++++++++++
.../mlx5/core/en_accel/nvmeotcp_rxtx.h | 43 +++
.../net/ethernet/mellanox/mlx5/core/en_rx.c | 41 ++-
7 files changed, 330 insertions(+), 7 deletions(-)
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp_rxtx.c
create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_accel/nvmeotcp_rxtx.h
@@ -0,0 +1,248 @@+// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB+/* Copyright (c) 2021 Mellanox Technologies. */++#include"en_accel/nvmeotcp_rxtx.h"+#include"en_accel/nvmeotcp.h"+#include<linux/mlx5/mlx5_ifc.h>++#define MLX5E_TC_FLOW_ID_MASK 0x00ffffff+staticvoidnvmeotcp_update_resync(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_cqe128*cqe128)+{+conststructulp_ddp_ulp_ops*ulp_ops;+u32seq;++seq=be32_to_cpu(cqe128->resync_tcp_sn);+ulp_ops=inet_csk(queue->sk)->icsk_ulp_ddp_ops;+if(ulp_ops&&ulp_ops->resync_request)+ulp_ops->resync_request(queue->sk,seq,ULP_DDP_RESYNC_REQ);+}++staticvoidmlx5e_nvmeotcp_advance_sgl_iter(structmlx5e_nvmeotcp_queue*queue)+{+structnvmeotcp_queue_entry*nqe=&queue->ccid_table[queue->ccid];++queue->ccoff+=nqe->sgl[queue->ccsglidx].length;+queue->ccoff_inner=0;+queue->ccsglidx++;+}++staticinlinevoid+mlx5e_nvmeotcp_add_skb_frag(structnet_device*netdev,structsk_buff*skb,+structmlx5e_nvmeotcp_queue*queue,+structnvmeotcp_queue_entry*nqe,u32fragsz)+{+dma_sync_single_for_cpu(&netdev->dev,+nqe->sgl[queue->ccsglidx].offset+queue->ccoff_inner,+fragsz,DMA_FROM_DEVICE);+page_ref_inc(compound_head(sg_page(&nqe->sgl[queue->ccsglidx])));+// XXX: consider reducing the truesize, as no new memory is consumed+skb_add_rx_frag(skb,skb_shinfo(skb)->nr_frags,+sg_page(&nqe->sgl[queue->ccsglidx]),+nqe->sgl[queue->ccsglidx].offset+queue->ccoff_inner,+fragsz,+fragsz);+}++staticstructsk_buff*+mlx5_nvmeotcp_add_tail_nonlinear(structmlx5e_nvmeotcp_queue*queue,+structsk_buff*skb,skb_frag_t*org_frags,+intorg_nr_frags,intfrag_index)+{+structmlx5e_priv*priv=queue->priv;++while(org_nr_frags!=frag_index){+if(skb_shinfo(skb)->nr_frags>=MAX_SKB_FRAGS){+dev_kfree_skb_any(skb);+returnNULL;+}+skb_add_rx_frag(skb,skb_shinfo(skb)->nr_frags,+skb_frag_page(&org_frags[frag_index]),+skb_frag_off(&org_frags[frag_index]),+skb_frag_size(&org_frags[frag_index]),+skb_frag_size(&org_frags[frag_index]));+page_ref_inc(skb_frag_page(&org_frags[frag_index]));+frag_index++;+}+returnskb;+}++staticstructsk_buff*+mlx5_nvmeotcp_add_tail(structmlx5e_nvmeotcp_queue*queue,structsk_buff*skb,+intoffset,intlen)+{+structmlx5e_priv*priv=queue->priv;++if(skb_shinfo(skb)->nr_frags>=MAX_SKB_FRAGS){+dev_kfree_skb_any(skb);+returnNULL;+}+skb_add_rx_frag(skb,skb_shinfo(skb)->nr_frags,+virt_to_page(skb->data),+offset,+len,+len);+page_ref_inc(virt_to_page(skb->data));+returnskb;+}++staticvoidmlx5_nvmeotcp_trim_nonlinear(structsk_buff*skb,+skb_frag_t*org_frags,+int*frag_index,+intremaining)+{+unsignedintfrag_size;+intnr_frags;++/* skip @remaining bytes in frags */+*frag_index=0;+while(remaining){+frag_size=skb_frag_size(&skb_shinfo(skb)->frags[*frag_index]);+if(frag_size>remaining){+skb_frag_off_add(&skb_shinfo(skb)->frags[*frag_index],+remaining);+skb_frag_size_sub(&skb_shinfo(skb)->frags[*frag_index],+remaining);+remaining=0;+}else{+remaining-=frag_size;+skb_frag_unref(skb,*frag_index);+*frag_index+=1;+}+}++/* save original frags for the tail and unref */+nr_frags=skb_shinfo(skb)->nr_frags;+memcpy(&org_frags[*frag_index],&skb_shinfo(skb)->frags[*frag_index],+(nr_frags-*frag_index)*sizeof(skb_frag_t));+while(--nr_frags>=*frag_index)+skb_frag_unref(skb,nr_frags);++/* remove frags from skb */+skb_shinfo(skb)->nr_frags=0;+skb->len-=skb->data_len;+skb->truesize-=skb->data_len;+skb->data_len=0;+}++structsk_buff*+mlx5e_nvmeotcp_handle_rx_skb(structnet_device*netdev,structsk_buff*skb,+structmlx5_cqe64*cqe,u32cqe_bcnt,+boollinear)+{+intccoff,cclen,hlen,ccid,remaining,fragsz,to_copy=0;+structmlx5e_priv*priv=netdev_priv(netdev);+skb_frag_torg_frags[MAX_SKB_FRAGS];+structmlx5e_nvmeotcp_queue*queue;+structnvmeotcp_queue_entry*nqe;+intorg_nr_frags,frag_index;+structmlx5e_cqe128*cqe128;+u32queue_id;++queue_id=(be32_to_cpu(cqe->sop_drop_qpn)&MLX5E_TC_FLOW_ID_MASK);+queue=mlx5e_nvmeotcp_get_queue(priv->nvmeotcp,queue_id);+if(unlikely(!queue)){+dev_kfree_skb_any(skb);+returnNULL;+}++cqe128=container_of(cqe,structmlx5e_cqe128,cqe64);+if(cqe_is_nvmeotcp_resync(cqe)){+nvmeotcp_update_resync(queue,cqe128);+mlx5e_nvmeotcp_put_queue(queue);+returnskb;+}++/* If a resync occurred in the previous cqe,+*thecurrentcqe.crcvalidbitmaynotbevalid,+*sowewilltreatitas0+*/+if(unlikely(queue->after_resync_cqe)){+skb->ddp_crc=0;+queue->after_resync_cqe=0;+}else{+if(queue->crc_rx)+skb->ddp_crc=cqe_is_nvmeotcp_crcvalid(cqe);+else+skb->ddp_crc=cqe_is_nvmeotcp_zc(cqe);+}++if(!cqe_is_nvmeotcp_zc(cqe)){+mlx5e_nvmeotcp_put_queue(queue);+returnskb;+}++/* cc ddp from cqe */+ccid=be16_to_cpu(cqe128->ccid);+ccoff=be32_to_cpu(cqe128->ccoff);+cclen=be16_to_cpu(cqe128->cclen);+hlen=be16_to_cpu(cqe128->hlen);++/* carve a hole in the skb for DDP data */+if(linear){+skb_trim(skb,hlen);+}else{+org_nr_frags=skb_shinfo(skb)->nr_frags;+mlx5_nvmeotcp_trim_nonlinear(skb,org_frags,&frag_index,+cclen);+}++nqe=&queue->ccid_table[ccid];++/* packet starts new ccid? */+if(queue->ccid!=ccid||queue->ccid_gen!=nqe->ccid_gen){+queue->ccid=ccid;+queue->ccoff=0;+queue->ccoff_inner=0;+queue->ccsglidx=0;+queue->ccid_gen=nqe->ccid_gen;+}++/* skip inside cc until the ccoff in the cqe */+while(queue->ccoff+queue->ccoff_inner<ccoff){+remaining=nqe->sgl[queue->ccsglidx].length-queue->ccoff_inner;+fragsz=min_t(off_t,remaining,+ccoff-(queue->ccoff+queue->ccoff_inner));++if(fragsz==remaining)+mlx5e_nvmeotcp_advance_sgl_iter(queue);+else+queue->ccoff_inner+=fragsz;+}++/* adjust the skb according to the cqe cc */+while(to_copy<cclen){+if(skb_shinfo(skb)->nr_frags>=MAX_SKB_FRAGS){+dev_kfree_skb_any(skb);+mlx5e_nvmeotcp_put_queue(queue);+returnNULL;+}++remaining=nqe->sgl[queue->ccsglidx].length-queue->ccoff_inner;+fragsz=min_t(int,remaining,cclen-to_copy);++mlx5e_nvmeotcp_add_skb_frag(netdev,skb,queue,nqe,fragsz);+to_copy+=fragsz;+if(fragsz==remaining)+mlx5e_nvmeotcp_advance_sgl_iter(queue);+else+queue->ccoff_inner+=fragsz;+}++if(cqe_bcnt>hlen+cclen){+remaining=cqe_bcnt-hlen-cclen;+if(linear)+skb=mlx5_nvmeotcp_add_tail(queue,skb,+offset_in_page(skb->data)++hlen+cclen,+remaining);+else+skb=mlx5_nvmeotcp_add_tail_nonlinear(queue,skb,+org_frags,+org_nr_frags,+frag_index);+}++mlx5e_nvmeotcp_put_queue(queue);+returnskb;+}
@@ -0,0 +1,415 @@+.. SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)++=================================+ULP direct data placement offload+=================================++Overview+========++The Linux kernel ULP direct data placement (DDP) offload infrastructure+provides tagged request-response protocols, such as NVMe-TCP, the ability to+place response data directly in pre-registered buffers according to header+tags. DDP is particularly useful for data-intensive pipelined protocols whose+responses may be reordered.++For example, in NVMe-TCP numerous read requests are sent together and each+request is tagged using the PDU header CID field. Receiving servers process+requests as fast as possible and sometimes responses for smaller requests+bypasses responses to larger requests, i.e., read 4KB bypasses read 1GB.+Thereafter, clients correlate responses to requests using PDU header CID tags.+The processing of each response requires copying data from SKBs to read+request destination buffers; The offload avoids this copy. The offload is+oblivious to destination buffers which can reside either in userspace+(O_DIRECT) or in kernel pagecache.++Request TCP byte-stream:++..parsed-literal::++ +---------------+-------+---------------+-------+---------------+-------++| PDU hdr CID=1 | Req 1 | PDU hdr CID=2 | Req 2 | PDU hdr CID=3 | Req 3 |+ +---------------+-------+---------------+-------+---------------+-------+++Response TCP byte-stream:++..parsed-literal::++ +---------------+--------+---------------+--------+---------------+--------++| PDU hdr CID=2 | Resp 2 | PDU hdr CID=3 | Resp 3 | PDU hdr CID=1 | Resp 1 |+ +---------------+--------+---------------+--------+---------------+--------+++The driver builds SKB page fragments that point destination buffers.+Consequently, SKBs represent the original data on the wire, which enables+*transparent* inter-operation with the network stack.To avoid copies between +SKBs and destination buffers,the layer-5 protocol (L5P) will check +``if (src == dst)`` for SKB page fragments,success indicates that data is +already placed there by NIC hardware and copy should be skipped.++In addition, L5P might have DDGST that responsible for ensure no-error over the+network. If not offloded, ULP DDP might be not efiant as L5P will need to go +over the data and calculate it by himself, redundet DDP copy skip.+ULP DDP have support for Rx/Tx DDGST offload. On the recived side the NIC will +verify DDGST for recived pdus and update SKB->ddp_crc bit if so. +If all SKB constructing L5P pdu have ddp_crc on, L5P will skip on calculating +and verify the DDGST for the correspond pdu. On the Tx side, the NIC will be +responsible for calculating and fill the DDGST fields in the sent pdus.++Offloading does require NIC hardware to track L5P protocol framing, similarly+to RX TLS offload (see documentation at+:ref:`Documentation/networking/tls-offload.rst <tls_offload>`). NIC hardware+will parse PDU headers extract fields such as operation type, length, ,tag+identifier, etc. and offload only segments that correspond to tags registered+with the NIC, see the :ref:`buf_reg` section.++Device configuration+====================++During driver initialization the device sets the ``NETIF_F_HW_ULP_DDP`` and+feature and installs its+:c:type:`struct ulp_ddp_ops <ulp_ddp_ops>`+pointer in the :c:member:`ulp_ddp_ops` member of the+:c:type:`struct net_device <net_device>`.++Later, after the L5P completes its handshake offload is installed on the socket.+If offload installation fails, then the connection is handled by software as if+offload was not attempted. Offload installation should configure++To request offload for a socket `sk`, the L5P calls :c:member:`ulp_ddp_sk_add`:++..code-block:: c++ int (*ulp_ddp_sk_add)(struct net_device *netdev,+ struct sock *sk,+ struct ulp_ddp_config *config);++The function return 0 for success. In case of failure, L5P software should+fallback to normal non-offloaded operation. The `config` parameter indicates+the L5P type and any metadata relevant for that protocol. For example, in+NVMe-TCP the following config is used:++..code-block:: c++ /**+* struct nvme_tcp_ddp_config - nvme tcp ddp configuration for an IO queue+ *+* @pfv: pdu version (e.g., NVME_TCP_PFV_1_0)+* @cpda: controller pdu data alignment (dwords, 0's based)+* @dgst: digest types enabled.+* The netdev will offload crc if ddp_crc is supported.+* @queue_size: number of nvme-tcp IO queue elements+* @queue_id: queue identifier+* @cpu_io: cpu core running the IO thread for this queue+ */+ struct nvme_tcp_ddp_config {+ struct ulp_ddp_config cfg;++ u16 pfv;+ u8 cpda;+ u8 dgst;+ int queue_size;+ int queue_id;+ int io_cpu;+ };++When offload is not needed anymore, e.g., the socket is being released, the L5P+calls :c:member:`ulp_ddp_sk_del` to release device contexts:++..code-block:: c++ void (*ulp_ddp_sk_del)(struct net_device *netdev,+ struct sock *sk);++TX+--++To request Tx offload for a socket `sk`, the L5P calls+:c:member:`ulp_ddp_int_tx_offload`:+..code-block:: c+ int ulp_ddp_init_tx_offload(struct sock *sk);++When Tx offload is not needed anymore, e.g., the socket is being released, +the L5P calls :c:member:`ulp_ddp_release_tx_offload` to release device +contexts:++..code-block:: c+ void ulp_ddp_release_tx_offload(struct sock *sk);++Normal operation+================++At the very least, the device maintains the following state for each connection:++* 5-tuple+* expected TCP sequence number+* mapping between tags and corresponding buffers+* current offset within PDU, PDU length, current PDU tag++NICs should not assume any correlation between PDUs and TCP packets. Assuming+that TCP packets arrive in-order, offload will place PDU payload directly+inside corresponding registered buffers. No packets are to be delayed by NIC+offload. If offload is not possible, than the packet is to be passed as-is to+software. To perform offload on incoming packets without buffering packets in+the NIC, the NIC stores some inter-packet state, such as partial PDU headers.++RX data-path+------------++After the device validates TCP checksums, it can perform DDP offload. The+packet is steered to the DDP offload context according to the 5-tuple.+Thereafter, the expected TCP sequence number is checked against the packet's+TCP sequence number. If there's a match, then offload is performed: PDU payload+is DMA written to corresponding destination buffer according to the PDU header+tag. The data should be DMAed only once, and the NIC receive ring will only+store the remaining TCP and PDU headers.++We remark that a single TCP packet may have numerous PDUs embedded inside. NICs+can choose to offload one or more of these PDUs according to various+trade-offs. Possibly, offloading such small PDUs is of little value, and it is+better to leave it to software.++Upon receiving a DDP offloaded packet, the driver reconstructs the original SKB+using page frags, while pointing to the destination buffers whenever possible.+This method enables seamless integration with the network stack, which can+inspect and modify packet fields transparently to the offload.++TX data-path+------------++In DDGST Tx offload the DDGST calculation isn't performed in the ULP (L5P).+Instead packets reach a device driver, the driver will mark the packets +for DDGST offload based on the socket the packet is attached to, +and send them to the device for DDGST calculation and transmission.++Both the device and the driver maintain expected TCP sequence numbers+due to the possibility of retransmissions and the lack of software fallback+once the packet reaches the device.+For segments passed in order, the driver marks the packets with+a connection identifier and hands them to the device. +The device identifies the packet as requiring DDGST offload handling and +confirms the sequence number matches its expectation. The device performs+DDGST calculation of the pdu data. +It replaces the PDU DDGST and TCP checksum with correct values.++.._buf_reg:++Destination buffer registration+-------------------------------++To register the mapping between tags and destination buffers for a socket+`sk`, the L5P calls :c:member:`ulp_ddp_setup` of :c:type:`struct ulp_ddp_ops+<ulp_ddp_ops>`:++..code-block:: c++ int (*ulp_ddp_setup)(struct net_device *netdev,+ struct sock *sk,+ struct ulp_ddp_io *io);+++The `io` provides the buffer via scatter-gather list (`sg_table`) and+corresponding tag (`command_id`):++..code-block:: c+ /**+* struct ulp_ddp_io - tcp ddp configuration for an IO request.+ *+* @command_id: identifier on the wire associated with these buffers+* @nents: number of entries in the sg_table+* @sg_table: describing the buffers for this IO request+* @first_sgl: first SGL in sg_table+ */+ struct ulp_ddp_io {+ u32 command_id;+ int nents;+ struct sg_table sg_table;+ struct scatterlist first_sgl[SG_CHUNK_SIZE];+ };++After the buffers have been consumed by the L5P, to release the NIC mapping of+buffers the L5P calls :c:member:`ulp_ddp_teardown` of :c:type:`struct+ulp_ddp_ops <ulp_ddp_ops>`:++..code-block:: c++ int (*ulp_ddp_teardown)(struct net_device *netdev,+ struct sock *sk,+ struct ulp_ddp_io *io,+ void *ddp_ctx);++`ulp_ddp_teardown` receives the same `io` context and an additional opaque+`ddp_ctx` that is used for asynchronous teardown, see the :ref:`async_release`+section.++.._async_release:++Asynchronous teardown+---------------------++To teardown the association between tags and buffers and allow tag reuse NIC HW+is called by the NIC driver during `ulp_ddp_teardown`. This operation may be+performed either synchronously or asynchronously. In asynchronous teardown,+`ulp_ddp_teardown` returns immediately without unmapping NIC HW buffers. Later,+when the unmapping completes by NIC HW, the NIC driver will call up to L5P+using :c:member:`ddp_teardown_done` of :c:type:`struct ulp_ddp_ulp_ops`:++..code-block:: c++ void (*ddp_teardown_done)(void *ddp_ctx);++The `ddp_ctx` parameter passed in `ddp_teardown_done` is the same on provided+in `ulp_ddp_teardown` and it is used to carry some context about the buffers+and tags that are released.++Resync handling+===============++RX+--+In presence of packet drops or network packet reordering, the device may lose+synchronization between the TCP stream and the L5P framing, and require a+resync with the kernel's TCP stack. When the device is out of sync, no offload+takes place, and packets are passed as-is to software. (resync is very similar+to TLS offload (see documentation at+:ref:`Documentation/networking/tls-offload.rst <tls_offload>`)++If only packets with L5P data are lost or reordered, then resynchronization may+be avoided by NIC HW that keeps tracking PDU headers. If, however, PDU headers+are reordered, then resynchronization is necessary.++To resynchronize hardware during traffic, we use a handshake between hardware+and software. The NIC HW searches for a sequence of bytes that identifies L5P+headers (i.e., magic pattern). For example, in NVMe-TCP, the PDU operation+type can be used for this purpose. Using the PDU header length field, the NIC+HW will continue to find and match magic patterns in subsequent PDU headers. If+the pattern is missing in an expected position, then searching for the pattern+starts anew.++The NIC will not resume offload when the magic pattern is first identified.+Instead, it will request L5P software to confirm that indeed this is a PDU+header. To request confirmation the NIC driver calls up to L5P using+:c:member:`*resync_request` of :c:type:`struct ulp_ddp_ulp_ops`:++..code-block:: c++ bool (*resync_request)(struct sock *sk, u32 seq, u32 flags);++The `seq` field contains the TCP sequence of the last byte in the PDU header.+L5P software will respond to this request after observing the packet containing+TCP sequence `seq` in-order. If the PDU header is indeed there, then L5P+software calls the NIC driver using the :c:member:`ulp_ddp_resync` function of+the :c:type:`struct ulp_ddp_ops <ulp_ddp_ops>` inside the :c:type:`struct+net_device <net_device>` while passing the same `seq` to confirm it is a PDU+header.++..code-block:: c++ void (*ulp_ddp_resync)(struct net_device *netdev,+ struct sock *sk, u32 seq);+++TX+--++Segments transmitted from an offloaded socket can get out of sync+in similar ways to the receive side-retransmissions - local drops+are possible, though network reorders are not. There is currently+one mechanism for dealing with out of order segments.++Offload state rebuilding+~~~~~~~~~~~~~~~~~~~~~~~++Whenever an out of order segment is transmitted the driver provides+the device with enough information to perform DDGST offload.+This means most likely that the part of the pdu preceding the current+segment has to be passed to the device as part of the packet context,+together with its TCP sequence number. The device can then initialize its +offload state, process and discard the preceding data (to be able to insert +the DDGST value) and move onto handling the actual packet.+For doing so, L5P should map PDU and TCP sequnce number using ++Depending on the implementation the driver can either ask for a continuation +with the offload state and the new sequence number (next expected segment is +the one after the out of order one), or continue with the previous stream +state - assuming that the out of order segment was just a retransmission.+The former is simpler, and does not require retransmission detection therefore +it is the recommended method until such time it is proven inefficient.++For doing so, L5P should map PDU and TCP sequnce number using:++..code-block:: c+ int ulp_ddp_map_pdu_info(struct sock *sk, u32 start_seq, void *hdr,+ u32 hdr_len, u32 data_len, struct request *req);+ void ulp_ddp_close_pdu_info(struct sock *sk);++While the driver can recived pdu information from ulp by calling:+..code-block:: c + struct ulp_ddp_pdu_info *ulp_ddp_get_pdu_info(struct sock *sk, u32 seq);++Statistics+==========++Per L5P protocol, the following NIC driver must report statistics for the above+netdevice operations and packets processed by offload. For example, NVMe-TCP+offload reports:++*``rx_nvmeotcp_queue_init`` - number of NVMe-TCP Rx offload contexts created.+*``rx_nvmeotcp_queue_teardown`` - number of NVMe-TCP Rx offload contexts+ destroyed.+*``rx_nvmeotcp_ddp_setup`` - number of DDP buffers mapped.+*``rx_nvmeotcp_ddp_setup_fail`` - number of DDP buffers mapping that failed.+*``rx_nvmeoulp_ddp_teardown`` - number of DDP buffers unmapped.+*``rx_nvmeotcp_drop`` - number of packets dropped in the driver due to fatal+ errors.+*``rx_nvmeotcp_resync`` - number of packets with resync requests.+*``rx_nvmeotcp_offload_packets`` - number of packets that used offload.+*``rx_nvmeotcp_offload_bytes`` - number of bytes placed in DDP buffers.+*``tx_nvmeotcp_offload_packets`` - number of Tx packets that used + DDGST offload.+*``tx_nvmeotcp_offload_bytes`` - number of Tx bytes that used + DDGST offload. +*``tx_nvmeotcp_ooo`` - number of TX Out of order packets.+*``tx_nvmeotcp_dump_packets`` - number of Dump packets sent to the NIC for+ sync on OOO context.+*``tx_nvmeotcp_dump_bytes`` - number of Dump bytes sent to the NIC for + sync on OOO context.+*``tx_nvmeotcp_resync`` - number of resync opertion due to out of order + packets +*``tx_nvmeotcp_ctx`` - number of NVMe-TCP Tx offload contexts created.+*``tx_nvmeotcp_resync_fail``: number of resync opertion that failed.+*``tx_nvmeotcp_no_need_offload`` - number of OOO packet that not needed+ offlaod +*``tx_nvmeotcp_no_pdu_info`` - number of OOO packet that dontt have + pdu-info. ++NIC requirements+================++NIC hardware should meet the following requirements to provide this offload:++* Offload must never buffer TCP packets.+* Offload must never modify TCP packet headers.+* Offload must never reorder TCP packets within a flow.+* Offload must never drop TCP packets.+* Offload must not depend on any TCP fields beyond the+ 5-tuple and TCP sequence number.++Error handling+==============++TX+--++Packets may be redirected or rerouted by the stack to a different+device than the selected ULP DDP offload device. The stack will handle+such condition using the :c:func:`sk_validate_xmit_skb` helper+(ULP DDP code installs :c:func:`ulp_ddp_validate_xmit_skb` at this hook).+Offload maintains information about all pdu until the data is fully +acknowledged, so if skbs reach the wrong device they can be handled +by software fallback.++Any ULP DDP device handling error on the transmission side must result+in the packet being dropped. For example if a packet got out of order+due to a bug in the stack or the device, reached the device and can't+be DDGST ofloaded, such packet must be dropped.+
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:34:10
From: Yoray Zack <redacted>
This struct is mapping between pdu's pages to TCP sequence number.
The use case for this mapping is in tx offload,
when the NIC needs to send a retransmitted packet.
The NIC, in this case, might need the complete pdu,
which is sent before (i.e for computing the CRC for this pdu).
Using this mapping, the SW can send the NIC enough context
to offload the packet.
Signed-off-by: Yoray Zack <redacted>
---
include/net/ulp_ddp.h | 55 +++++++++++++-
net/core/Makefile | 1 +
net/core/ulp_ddp.c | 166 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 219 insertions(+), 3 deletions(-)
create mode 100644 net/core/ulp_ddp.c
@@ -10,6 +10,7 @@#include<linux/netdevice.h>#include<net/inet_connection_sock.h>#include<net/sock.h>+#include<net/tcp.h>/* limits returned by the offload driver, zero means don't care */structulp_ddp_limits{
@@ -67,6 +68,26 @@ struct ulp_ddp_io {structscatterlistfirst_sgl[SG_CHUNK_SIZE];};+/**+*structulp_ddp_pdu_info-pduinfofortcpddpcrcTxoffload.+*+*@end_seq:tcpseqofthelastbyteinthepdu.+*@start_seq:tcpseqofthefirstbyteinthepdu.+*@data_len:pdudatasize(inbytes).+*@hdr_len:thesize(inbytes)ofthepduheader.+*@hdr:pduheader.+*@req:theulprequestfortheoriginalpdu.+*/+structulp_ddp_pdu_info{+structlist_headlist;+u32end_seq;+u32start_seq;+u32data_len;+u32hdr_len;+void*hdr;+structrequest*req;+};+/* struct ulp_ddp_dev_ops - operations used by an upper layer protocol to configure ddp offload**@ulp_ddp_limits:limitthenumberofscattergatherentriesperIO.
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:37:15
From: Yoray Zack <redacted>
Add ddp_ddgest_falback(), and ddp_get_pdu_info function to ulp.
During DDP CRC Tx offload, the HW is responsible for calculate the crc,
and therefore the SW not calculates it.
If the HW changes for some reason,
the SW should fallback from the offload and calculate the crc.
This is checking in the ulp_ddp_validate_skb and if need fallback it do it.
Signed-off-by: Yoray Zack <redacted>
---
include/net/ulp_ddp.h | 7 +++++
net/core/ulp_ddp.c | 69 +++++++++++++++++++++++++++++++++++++++++++
2 files changed, 76 insertions(+)
@@ -86,6 +87,7 @@ struct ulp_ddp_pdu_info {u32hdr_len;void*hdr;structrequest*req;+__le32ddgst;};/* struct ulp_ddp_dev_ops - operations used by an upper layer protocol to configure ddp offload
@@ -129,6 +131,8 @@ struct ulp_ddp_ulp_ops {bool(*resync_request)(structsock*sk,u32seq,u32flags);/* NIC driver informs the ulp that ddp teardown is done - used for async completions*/void(*ddp_teardown_done)(void*ddp_ctx);+/* NIC request ulp to calculate the ddgst and store it in pdu_info->ddgst */+void(*ddp_ddgst_fallback)(structulp_ddp_pdu_info*pdu_info);};/**
@@ -164,3 +164,72 @@ struct ulp_ddp_pdu_info *ulp_ddp_get_pdu_info(struct sock *sk, u32 seq)returninfo;}EXPORT_SYMBOL(ulp_ddp_get_pdu_info);+staticvoidulp_ddp_ddgst_recalc(conststructulp_ddp_ulp_ops*ulp_ops,+structulp_ddp_pdu_info*pdu_info)+{+if(pdu_info->ddgst)+return;++ulp_ops->ddp_ddgst_fallback(pdu_info);+}++staticstructsk_buff*ulp_ddp_fallback_skb(structulp_ddp_ctx*ctx,+structsk_buff*skb,+structsock*sk)+{+conststructulp_ddp_ulp_ops*ulp_ops=inet_csk(sk)->icsk_ulp_ddp_ops;+intdatalen=skb->len-(skb_transport_offset(skb)+tcp_hdrlen(skb));+structulp_ddp_pdu_info*pdu_info=NULL;+intddgst_start,ddgst_offset,ddgst_len;+u32seq=ntohl(tcp_hdr(skb)->seq);+u32end_skb_seq=seq+datalen;+u32first_seq=seq;++if(!(ulp_ops&&ulp_ops->ddp_ddgst_fallback))+returnskb;++again:+/* check if we can't use the last pdu_info+*Reasonswecan'tuseit:+*1.firsttimeandthenpdu_infoisNULL.+*2.seqdoesn'tMaptothispdu_info(outofbounds).+*/+if(!pdu_info||!between(seq,pdu_info->start_seq,pdu_info->end_seq-1)){+pdu_info=ulp_ddp_get_pdu_info(sk,seq);+if(!pdu_info)+returnskb;+}++ddgst_start=pdu_info->end_seq-ctx->ddgst_len;++//check if this skb contains ddgst field+if(between(ddgst_start,seq,end_skb_seq-1)&&pdu_info->data_len){+ulp_ddp_ddgst_recalc(ulp_ops,pdu_info);+ddgst_offset=ddgst_start-first_seq+skb_headlen(skb);+ddgst_len=min_t(int,ctx->ddgst_len,end_skb_seq-ddgst_start);+skb_store_bits(skb,ddgst_offset,&pdu_info->ddgst,ddgst_len);+}++//check if there is more PDU's in this skb+if(between(pdu_info->end_seq,seq+1,end_skb_seq-1)){+seq=pdu_info->end_seq;+gotoagain;+}++returnskb;+}++structsk_buff*ulp_ddp_validate_xmit_skb(structsock*sk,+structnet_device*dev,+structsk_buff*skb)+{+structulp_ddp_ctx*ctx=ulp_ddp_get_ctx(sk);++if(!ctx)+returnskb;++if(dev==ctx->netdev)+returnskb;++returnulp_ddp_fallback_skb(ctx,skb,sk);+}EXPORT_SYMBOL(ulp_ddp_validate_xmit_skb);
@@ -310,6 +310,7 @@ struct ucred {*plaintextandrequireencryption*/+#define MSG_DDP_CRC 0x200000 /* Skb pdu need crc offload */#define MSG_ZEROCOPY 0x4000000 /* Use user data in kernel path */#define MSG_FASTOPEN 0x20000000 /* Send data in TCP SYN */#define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exec for file
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:46:46
From: Yoray Zack <redacted>
This commit maintains a mapping from TCP sequence number to NVMEoTCP pdus,
for DDGST tx offload using the ULP_DDP API.
When send a pdu it save the req in ulp_ddp_pdu_info struct.
This mapping is used:
1. When packet is retransmitted, If this packet contain NVMEoTCP DDGST,
The NIC might needs all the pdu again for computing the DDGST.
2. If packet is offloaded but will not go to the offloaded netdev,
Then SW will need to be able to fallback and compute the DDGST.
Add founction nvme_tcp_ddgest_fallback(pdu_info):
caclulate the data digest for ulp_ddp_pdu_info
(requested by the netdev).
Signed-off-by: Yoray Zack <redacted>
---
drivers/nvme/host/tcp.c | 93 ++++++++++++++++++++++++++++++++++++++++-
1 file changed, 91 insertions(+), 2 deletions(-)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:50:56
From: Yoray Zack <redacted>
NVMEoTCP CRC Tx offload is similar to TLS Tx offload,
and uses DUMP wqe as well.
To avoid duplicate functions the following changes were added:
1. Add DUMP_WQE.type field (=TLS or NVMEoTCP).
2. change in mlx5e_ktls_tx_handle_resync_dump_comp
to handle also NVMEoTCP Tx DUMP WQE.
Signed-off-by: Yoray Zack <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en/txrx.h | 5 +++++
.../ethernet/mellanox/mlx5/core/en_accel/ktls_tx.c | 12 ++++++++++--
2 files changed, 15 insertions(+), 2 deletions(-)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 11:54:46
From: Yoray Zack <redacted>
Add to the sq state a bit indicate if their is an nvmeotcp crc tx offload.
Signed-off-by: Yoray Zack <redacted>
---
drivers/net/ethernet/mellanox/mlx5/core/en.h | 1 +
drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 2 ++
2 files changed, 3 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 12:06:30
From: Yoray Zack <redacted>
This commit add support for DDGST TX offload to the mlx5e_nvmeotcp_queue_init/teardown function.
If enable, mlx5e_nvmeotcp_queue_init will call mlx5e_nvmeotcp_queue_tx_init to handle TX offload init.
Add to mlx5e NVMEoTCP queue is responsible for:
- Create a separate TIS to identify the queue and maintain the HW context
- Update ulp_ddp_ctx params.
Signed-off-by: Yoray Zack <redacted>
---
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 47 +++++++++++++++++++
.../mellanox/mlx5/core/en_accel/nvmeotcp.h | 12 ++++-
2 files changed, 58 insertions(+), 1 deletion(-)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 12:14:22
From: Yoray Zack <redacted>
This patch handles only the good flow for the DDGST Tx offload
skb and wqe.
Later patches will handle bad flow (OOO packets)
1. add mlx5e_nvmeotcp_handle_tx_skb function(skb,...):
check if the skb can be offlaoded.
this function track the tcp_seq of the skb,
and check if this is the next tcp_seq.
and if so, send this skb with DDGST Tx offload.
2. add mlx5e_nvmeotcp_handle_tx_wqe function :
send the wqe with the correct tis number for the offload.
Signed-off-by: Yoray Zack <redacted>
---
.../mellanox/mlx5/core/en_accel/en_accel.h | 13 ++++
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 63 +++++++++++++++++++
.../mellanox/mlx5/core/en_accel/nvmeotcp.h | 6 ++
.../net/ethernet/mellanox/mlx5/core/en_tx.c | 11 ++++
4 files changed, 93 insertions(+)
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 12:17:32
From: Yoray Zack <redacted>
When the driver indicate an OOO NVMEoTCP Tx packet it starts OOO flow:
1. Get pdu_info from nvme-tcp.
2. Send indication to NIC (set psv)- NIC will rebuild the parse machine.
3. Send the data the NIC needs for computing the DDGST using DUMP wqes.
Signed-off-by: Yoray Zack <redacted>
---
.../net/ethernet/mellanox/mlx5/core/en/txrx.h | 2 +-
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 281 +++++++++++++++++-
2 files changed, 280 insertions(+), 3 deletions(-)
@@ -1160,6 +1230,202 @@ void mlx5e_nvmeotcp_tx_post_param_wqes(struct mlx5e_txqsq *sq, struct sock *sk,mlx5e_nvmeotcp_tx_post_progress_params(ctx,sq,tcp_sk(sk)->copied_seq,false);}+enummlx5e_nvmeotcp_resync_retval{+MLX5E_NVMEOTCP_RESYNC_DONE,+MLX5E_NVMEOTCP_RESYNC_FAIL,+MLX5E_NVMEOTCP_RESYNC_SKIP,+};++static+intmlx5e_nvmeotcp_resync_frag(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,structsk_buff*skb,+inti,skb_frag_t*frag,u32seq)+{+unsignedintorig_fsz,frag_offset=0,n=0;+enummlx5e_dump_wqe_typetype=MLX5E_DUMP_WQE_NVMEOTCP;++orig_fsz=skb_frag_size(frag);++do{+boolfence=!(i||frag_offset);+unsignedintfsz;++n++;+fsz=min_t(unsignedint,sq->hw_mtu,orig_fsz-frag_offset);+skb_frag_size_set(frag,fsz);+if(nvmeotcp_post_resync_dump(sq,frag,queue->tisn,fence,type)){+page_ref_add(compound_head(skb_frag_page(frag)),n-1);+return-1;+}++skb_frag_off_add(frag,fsz);+frag_offset+=fsz;+}while(frag_offset<orig_fsz);++page_ref_add(compound_head(skb_frag_page(frag)),n);++return0;+}++staticintmlx5e_nvmeotcp_resync_hdr(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,u32seq,+structsk_buff*skb,intremaining,+structulp_ddp_pdu_info*pdu_info)+{+skb_frag_tpdu_frag;+intsize=min_t(int,remaining,pdu_info->hdr_len);++__skb_frag_set_page(&pdu_frag,virt_to_page(pdu_info->hdr));+skb_frag_off_set(&pdu_frag,offset_in_page(pdu_info->hdr));+skb_frag_size_set(&pdu_frag,size);++returnmlx5e_nvmeotcp_resync_frag(queue,sq,skb,0,&pdu_frag,seq);+}++staticvoidmlx5e_nvmeotcp_init_iter(structiov_iter*iter,structbio*bio)+{+unsignedintbio_size;+structbio_vec*vec;+intnsegs;++vec=__bvec_iter_bvec(bio->bi_io_vec,bio->bi_iter);+nsegs=bio_segments(bio);+bio_size=bio->bi_iter.bi_size;+iov_iter_bvec(iter,1,vec,nsegs,bio_size);+iter->iov_offset=0;+}++staticintmlx5e_nvmeotcp_resync_data(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,u32seq,+structsk_buff*skb,intremaining,+structulp_ddp_pdu_info*pdu_info)+{+structrequest*req=pdu_info->req;+structbio*bio=req->bio;+structiov_iteriter;+intdata_remaining;+size_tdata_sent=0;++mlx5e_nvmeotcp_init_iter(&iter,bio);++data_remaining=min_t(int,remaining,pdu_info->data_len);++while(data_remaining>0){+skb_frag_tfrag;+size_tsize=min_t(size_t,+iter.bvec->bv_len-iter.iov_offset+,data_remaining);++__skb_frag_set_page(&frag,iter.bvec->bv_page);+skb_frag_off_set(&frag,iter.bvec->bv_offset+iter.iov_offset);+skb_frag_size_set(&frag,size);+data_remaining-=size;++if(mlx5e_nvmeotcp_resync_frag(queue,sq,skb,1,&frag,seq))+gotoerr_out;++if(!data_remaining)+break;++data_sent+=size;+iov_iter_advance(&iter,size);+if(!iov_iter_count(&iter)&&data_sent<pdu_info->data_len){+bio=bio->bi_next;+mlx5e_nvmeotcp_init_iter(&iter,bio);+}+}++return0;+err_out:+return-1;+}++staticintmlx5e_nvmeotcp_resync_crc(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,u32seq,+structsk_buff*skb,intremaining,+structulp_ddp_pdu_info*pdu_info)+{+skb_frag_tcrc_frag;+u32dummy_ddigest=0;++__skb_frag_set_page(&crc_frag,virt_to_page(&dummy_ddigest));+skb_frag_off_set(&crc_frag,offset_in_page(&dummy_ddigest));+skb_frag_size_set(&crc_frag,remaining);+returnmlx5e_nvmeotcp_resync_frag(queue,sq,skb,1,&crc_frag,seq);+}++/* for a pdu info mapping [--------seq----] capsule+*******sendtoHW[-------|seq*******************/+static+boolmlx5e_nvmeotcp_resync_cap(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,structsk_buff*skb,+structulp_ddp_pdu_info*pdu_info,+u32seq)+{+intremaining=seq-pdu_info->start_seq;+intret;++ret=mlx5e_nvmeotcp_resync_hdr(queue,sq,seq,skb,remaining,+pdu_info);+if(unlikely(ret))+gotoerr_out;++remaining-=pdu_info->hdr_len;+if(remaining<=0)+gotoout;++ret=mlx5e_nvmeotcp_resync_data(queue,sq,seq,skb,remaining,+pdu_info);+if(unlikely(ret))+gotoerr_out;++remaining-=pdu_info->data_len;+if(remaining<=0)+gotoout;++ret=mlx5e_nvmeotcp_resync_crc(queue,sq,seq,skb,remaining,+pdu_info);+if(unlikely(ret))+gotoerr_out;+out:+returntrue;+err_out:+returnfalse;+}++staticenummlx5e_nvmeotcp_resync_retval+mlx5e_nvmeotcp_handle_ooo_skb(structmlx5e_nvmeotcp_queue*queue,+structmlx5e_txqsq*sq,structsk_buff*skb,+u32seq,intdatalen)+{+structulp_ddp_pdu_info*pdu_info=NULL;++/* ask for pdu_info that includes the tcp_seq */+pdu_info=ulp_ddp_get_pdu_info(skb->sk,seq);++if(!pdu_info)+returnMLX5E_NVMEOTCP_RESYNC_SKIP;++/*update NIC about resync - he will rebuild parse machine+*sendpsvwithsmallfence+*/+mlx5e_nvmeotcp_tx_post_progress_params(queue,sq,pdu_info->start_seq,true);++if(seq==pdu_info->start_seq||seq==pdu_info->end_seq){+mlx5e_nvmeotcp_tx_post_fence_nop(sq);+returnMLX5E_NVMEOTCP_RESYNC_DONE;+}++/* post dump wqes -+*transfertheneededdatatoNICHWusingDUMPWQEwithdata[*,^]+*savedinpdu_info+*/+if(unlikely(!mlx5e_nvmeotcp_resync_cap(queue,sq,skb,pdu_info,seq)))+returnMLX5E_NVMEOTCP_RESYNC_FAIL;++returnMLX5E_NVMEOTCP_RESYNC_DONE;+}+staticinlineboolmlx5e_is_sk_tx_device_offloaded(structsock*sk){/* Return True after smp_store_release assing in
From: Boris Pismenny <borisp@nvidia.com> Date: 2021-07-22 12:20:46
From: Yoray Zack <redacted>
Stop offloading NVMEoTCP OOO packets which aren't contains the pdu DDGST field.
When the driver indicate an OOO NVMEoTCP packet he check if it contains
NVMEoTCP PDU DDGST field, if so he offloads it, otherwise he isn't.
Signed-off-by: Yoray Zack <redacted>
---
.../mellanox/mlx5/core/en_accel/nvmeotcp.c | 23 +++++++++++++++++++
1 file changed, 23 insertions(+)
@@ -1393,6 +1393,20 @@ bool mlx5e_nvmeotcp_resync_cap(struct mlx5e_nvmeotcp_queue *queue,returnfalse;}+static+boolmlx5e_nvmeotcp_check_if_need_offload(structmlx5e_nvmeotcp_queue*queue,+u32end_seq,u32start_seq)+{+if(!queue->end_seq_hint)+returnfalse;++/* check if skb end after pdu crc start */+if(before(queue->end_seq_hint,end_seq)||before(start_seq,queue->start_pdu_hint))+returnfalse;++returntrue;+}+staticenummlx5e_nvmeotcp_resync_retvalmlx5e_nvmeotcp_handle_ooo_skb(structmlx5e_nvmeotcp_queue*queue,structmlx5e_txqsq*sq,structsk_buff*skb,
@@ -1400,12 +1414,21 @@ mlx5e_nvmeotcp_handle_ooo_skb(struct mlx5e_nvmeotcp_queue *queue,{structulp_ddp_pdu_info*pdu_info=NULL;+if(mlx5e_nvmeotcp_check_if_need_offload(queue,seq+datalen,seq)){+returnMLX5E_NVMEOTCP_RESYNC_SKIP;+/* ask for pdu_info that includes the tcp_seq */pdu_info=ulp_ddp_get_pdu_info(skb->sk,seq);if(!pdu_info)returnMLX5E_NVMEOTCP_RESYNC_SKIP;+queue->end_seq_hint=pdu_info->end_seq-4;+queue->start_pdu_hint=pdu_info->start_seq;+/* check if this packet contain crc - if so offload else no */+if(mlx5e_nvmeotcp_check_if_need_offload(queue,seq+datalen,seq)){+returnMLX5E_NVMEOTCP_RESYNC_SKIP;+/*update NIC about resync - he will rebuild parse machine*sendpsvwithsmallfence*/
@@ -1413,21 +1415,29 @@ mlx5e_nvmeotcp_handle_ooo_skb(struct mlx5e_nvmeotcp_queue *queue,u32seq,intdatalen){structulp_ddp_pdu_info*pdu_info=NULL;+structmlx5e_sq_stats*stats=sq->stats;+stats->nvmeotcp_ooo++;if(mlx5e_nvmeotcp_check_if_need_offload(queue,seq+datalen,seq)){+stats->nvmeotcp_no_need_offload++;returnMLX5E_NVMEOTCP_RESYNC_SKIP;+}/* ask for pdu_info that includes the tcp_seq */pdu_info=ulp_ddp_get_pdu_info(skb->sk,seq);-if(!pdu_info)+if(!pdu_info){+stats->nvmeotcp_no_pdu_info++;returnMLX5E_NVMEOTCP_RESYNC_SKIP;+}queue->end_seq_hint=pdu_info->end_seq-4;queue->start_pdu_hint=pdu_info->start_seq;/* check if this packet contain crc - if so offload else no */if(mlx5e_nvmeotcp_check_if_need_offload(queue,seq+datalen,seq)){+stats->nvmeotcp_no_need_offload++;returnMLX5E_NVMEOTCP_RESYNC_SKIP;+}/*update NIC about resync - he will rebuild parse machine*sendpsvwithsmallfence
@@ -405,6 +416,18 @@ struct mlx5e_sq_stats {u64tls_skip_no_sync_data;u64tls_drop_no_sync_data;u64tls_drop_bypass_req;+#endif+#ifdef CONFIG_MLX5_EN_NVMEOTCP+u64nvmeotcp_offload_packets;+u64nvmeotcp_offload_bytes;+u64nvmeotcp_ooo;+u64nvmeotcp_resync;+u64nvmeotcp_dump_packets;+u64nvmeotcp_dump_bytes;+u64nvmeotcp_ctx;+u64nvmeotcp_resync_fail;+u64nvmeotcp_no_need_offload;+u64nvmeotcp_no_pdu_info;#endif/* less likely accessed in data path */u64csum_none;
This checks only the second, third, and remaining skbs.
+ goto end;
#endif
}
}
tcp_collapse() is copying data from small skbs to pack it to bigger
skb (one page of payload), in case
of memory emergency/pressure (socket queues are full)
If your changes are trying to avoid 'needless' copies, maybe you
should reconsider and let the emergency packing be done.
If the copy is not _possible_, you should rephrase your changelog to
clearly state the kernel _cannot_ access this memory in any way.
This checks only the second, third, and remaining skbs.
Right, as we handle the head skb above. Could you clarify?
quoted
+ goto end;
#endif
}
}
tcp_collapse() is copying data from small skbs to pack it to bigger
skb (one page of payload), in case
of memory emergency/pressure (socket queues are full)
If your changes are trying to avoid 'needless' copies, maybe you
should reconsider and let the emergency packing be done.
If the copy is not _possible_, you should rephrase your changelog to
clearly state the kernel _cannot_ access this memory in any way.
The issue is that skb_condense also gets called on many skbs in
tcp_add_backlog and it will identify skbs that went through DDP as ideal
for packing, even though they are not small and packing is
counter-productive as data already resides in its destination.
As mentioned above, it is possible to copy, but it is counter-productive
in this case. If there was a real need to access this memory, then it is
allowed.
This checks only the second, third, and remaining skbs.
Right, as we handle the head skb above. Could you clarify?
I was simply saying you missed the first skb.
quoted
quoted
+ goto end;
#endif
}
}
tcp_collapse() is copying data from small skbs to pack it to bigger
skb (one page of payload), in case
of memory emergency/pressure (socket queues are full)
If your changes are trying to avoid 'needless' copies, maybe you
should reconsider and let the emergency packing be done.
If the copy is not _possible_, you should rephrase your changelog to
clearly state the kernel _cannot_ access this memory in any way.
The issue is that skb_condense also gets called on many skbs in
tcp_add_backlog and it will identify skbs that went through DDP as ideal
for packing, even though they are not small and packing is
counter-productive as data already resides in its destination.
As mentioned above, it is possible to copy, but it is counter-productive
in this case. If there was a real need to access this memory, then it is
allowed.
Standard GRO packets from high perf drivers have no room in their
skb->head (ie skb_tailroom() should be 0)
If you have a driver using GRO and who pulled some payload in
skb->head, it is already too late for DDP.
So I think you are trying to add code in TCP that should not be
needed. Perhaps mlx5 driver is doing something it should not ?
(If this is ' copybreak' this has been documented as being
suboptimal, transports have better strategies)
Secondly, tcp_collapse() should absolutely not be called under regular
workloads.
Trying to optimize this last-resort thing is a lost cause:
If an application is dumb enough to send small packets back-to-back,
it should be fixed (sender side has this thing called autocork, for
applications that do not know about MSG_MORE or TC_CORK.)
(tcp_collapse is a severe source of latencies)
This looks completely bogus to any casual read, so please document why
it makes sense. And no, a magic, unexplained ddp in the name does not
count as explanation at all. Please think about a more useful name.
Can this ever write to user page? If yes it needs a flush_dcache_page.
Last but not least: kmap_atomic is deprecated except for the very
rate use case where it is actually called from atomic context. Please
use kmap_local_page instead.
This checks only the second, third, and remaining skbs.
Right, as we handle the head skb above. Could you clarify?
I was simply saying you missed the first skb.
But, the first SKB got handled in the change above. The code here is the
same for TLS, if it is wrong, then we already have an issue here.
quoted
quoted
quoted
+ goto end;
#endif
}
}
tcp_collapse() is copying data from small skbs to pack it to bigger
skb (one page of payload), in case
of memory emergency/pressure (socket queues are full)
If your changes are trying to avoid 'needless' copies, maybe you
should reconsider and let the emergency packing be done.
If the copy is not _possible_, you should rephrase your changelog to
clearly state the kernel _cannot_ access this memory in any way.
The issue is that skb_condense also gets called on many skbs in
tcp_add_backlog and it will identify skbs that went through DDP as ideal
for packing, even though they are not small and packing is
counter-productive as data already resides in its destination.
As mentioned above, it is possible to copy, but it is counter-productive
in this case. If there was a real need to access this memory, then it is
allowed.
Standard GRO packets from high perf drivers have no room in their
skb->head (ie skb_tailroom() should be 0)
If you have a driver using GRO and who pulled some payload in
skb->head, it is already too late for DDP.
So I think you are trying to add code in TCP that should not be
needed. Perhaps mlx5 driver is doing something it should not ?
(If this is ' copybreak' this has been documented as being
suboptimal, transports have better strategies)
Secondly, tcp_collapse() should absolutely not be called under regular
workloads.
Trying to optimize this last-resort thing is a lost cause:
If an application is dumb enough to send small packets back-to-back,
it should be fixed (sender side has this thing called autocork, for
applications that do not know about MSG_MORE or TC_CORK.)
(tcp_collapse is a severe source of latencies)
Sorry. My response above was about skb_condense which I've confused with
tcp_collapse.
In tcp_collapse, we could allow the copy, but the problem is CRC, which
like TLS's skb->decrypted marks that the data passed the digest
validation in the NIC. If we allow collapsing SKBs with mixed marks, we
will need to force software copy+crc verification. As TCP collapse is
indeed rare and the offload is opportunistic in nature, we can make this
change and submit another version, but I'm confused; why was it OK for
TLS, while it is not OK for DDP+CRC?
From: Eric Dumazet <edumazet@google.com> Date: 2021-07-22 13:45:37
On Thu, Jul 22, 2021 at 3:33 PM Boris Pismenny [off-list ref] wrote:
Sorry. My response above was about skb_condense which I've confused with
tcp_collapse.
In tcp_collapse, we could allow the copy, but the problem is CRC, which
like TLS's skb->decrypted marks that the data passed the digest
validation in the NIC. If we allow collapsing SKBs with mixed marks, we
will need to force software copy+crc verification. As TCP collapse is
indeed rare and the offload is opportunistic in nature, we can make this
change and submit another version, but I'm confused; why was it OK for
TLS, while it is not OK for DDP+CRC?
Ah.... I guess I was focused on the DDP part, while all your changes
are really about the CRC part.
Perhaps having an accessor to express the CRC status (and not be
confused by the DDP part)
could help the intent of the code.
From: Boris Pismenny <hidden> Date: 2021-07-22 14:03:26
On 22/07/2021 16:39, Eric Dumazet wrote:
On Thu, Jul 22, 2021 at 3:33 PM Boris Pismenny [off-list ref] wrote:
quoted
Sorry. My response above was about skb_condense which I've confused with
tcp_collapse.
In tcp_collapse, we could allow the copy, but the problem is CRC, which
like TLS's skb->decrypted marks that the data passed the digest
validation in the NIC. If we allow collapsing SKBs with mixed marks, we
will need to force software copy+crc verification. As TCP collapse is
indeed rare and the offload is opportunistic in nature, we can make this
change and submit another version, but I'm confused; why was it OK for
TLS, while it is not OK for DDP+CRC?
Ah.... I guess I was focused on the DDP part, while all your changes
are really about the CRC part.
Perhaps having an accessor to express the CRC status (and not be
confused by the DDP part)
could help the intent of the code.
An accessor function sounds like a great idea for readability, thanks Eric!
We will re-spin the series and add it to v6.
From: Eric Dumazet <edumazet@google.com> Date: 2021-07-22 14:24:23
On Thu, Jul 22, 2021 at 1:06 PM Boris Pismenny [off-list ref] wrote:
From: Yoray Zack <redacted>
if the msg sent with this flag, turn up skb->ddp_crc bit.
Signed-off-by: Yoray Zack <redacted>
Ok, but why ?
How would you document in linux manpages the purpose of this flag ?
How can applications be sure the kernel they run on is actually
supporting this flag ?
Maybe it is a hint only ?
This looks completely bogus to any casual read, so please document why
it makes sense. And no, a magic, unexplained ddp in the name does not
count as explanation at all. Please think about a more useful name.
This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same. These are
then used by nvme-tcp (see skb_ddp_copy_datagram_iter in nvme-tcp) which
receives SKBs from the NIC that already placed data in its destination,
and this is the source for the name Direct Data Placement. I'd gladly
take suggestions for better names, but this is the best we came up with
so far.
The reason we are doing it is to avoid modifying memcpy_to_page itself,
but rather allow users (e.g., nvme-tcp) to access this functionality
directly.
Can this ever write to user page? If yes it needs a flush_dcache_page.
Yes, will add.
Last but not least: kmap_atomic is deprecated except for the very
rate use case where it is actually called from atomic context. Please
use kmap_local_page instead.
What is the point of this stub? To me it looks extremely dangerous.
As above, we use the same logic as in hash_and_copy_to_iter. The purpose
is again to eventually avoid the copy in case the source and destination
buffers are one and the same.
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-07-22 21:00:53
On Thu, Jul 22, 2021 at 02:02:51PM +0300, Boris Pismenny wrote:
From: Boris Pismenny <redacted>
When using direct data placement (DDP) the NIC writes some of the payload
directly to the destination buffer, and constructs SKBs such that they
point to this data. To skip copies when SKB data already resides in the
destination we use the newly introduced routines in this commit, which
check if (src == dst), and skip the copy when that's true.
As the current user for these routines is in the block layer (nvme-tcp),
then we only apply the change for bio_vec. Other routines use the normal
methods for copying.
Please, take a look at -rc1 and see the changes in lib/iov_iter.c in there.
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-23 05:03:25
On Thu, Jul 22, 2021 at 11:23:38PM +0300, Boris Pismenny wrote:
This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same.
Now why can't we just make that change to the generic routine?
If we can't, why do they not have a saner name documenting what they
actually do?
From: Al Viro <viro@zeniv.linux.org.uk> Date: 2021-07-23 05:26:32
On Fri, Jul 23, 2021 at 07:03:02AM +0200, Christoph Hellwig wrote:
On Thu, Jul 22, 2021 at 11:23:38PM +0300, Boris Pismenny wrote:
quoted
This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same.
Now why can't we just make that change to the generic routine?
Doable... replace memcpy(base, addr + off, len) with
base != addr + off && memcpy(base, addr + off, len)
in _copy_to_iter() and be done with that...
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-23 05:57:02
On Thu, Jul 22, 2021 at 02:02:49PM +0300, Boris Pismenny wrote:
From: Boris Pismenny <redacted>
Changes since v4:
=========================================
Explaining what the series does should go before the changelog.
* Add transmit offload patches
But to be honest, the previous one was already mostly unreviewable,
but this is now far beyond this. Please try to get anything that
is generally useful first in smaller series and the come back with
a somewhat reviewable series. That also means that at least for the
code I care about (nvme) the patches should be grouped together,
and actually provide meaningful functionality in each patch. Right
now even trying to understand what you add to the nvme code requires
me to jump all over a gigantic series.
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-23 06:07:06
On Thu, Jul 22, 2021 at 02:02:53PM +0300, Boris Pismenny wrote:
From: Boris Pismenny <redacted>
get_netdev_for_sock is a utility that is used to obtain
the net_device structure from a connected socket.
Later patches will use this for nvme-tcp DDP and DDP DDGST offloads.
Signed-off-by: Boris Pismenny <redacted>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
I don't think this should be an inline. Please move it to net/core/dev.c,
andd add an EXPORT_SYMBOL_GPL and a kerneldoc comment.
From: Christoph Hellwig <hch@lst.de> Date: 2021-07-23 06:09:39
On Thu, Jul 22, 2021 at 02:03:12PM +0300, Boris Pismenny wrote:
/* NIC driver informs the ulp that ddp teardown is done - used for async completions*/
void (*ddp_teardown_done)(void *ddp_ctx);
+ /* NIC request ulp to calculate the ddgst and store it in pdu_info->ddgst */
+ void (*ddp_ddgst_fallback)(struct ulp_ddp_pdu_info *pdu_info);
Overly long line. More importantly this whole struct should probably
use a kerneldoc comment anyway.
} EXPORT_SYMBOL(ulp_ddp_get_pdu_info);
+ if (!pdu_info || !between(seq, pdu_info->start_seq, pdu_info->end_seq - 1)) {
More overly lone lines. Please make sure to stick to 80 character lines
unless you have a really good to go over that.
+/**
+ * struct ulp_ddp_pdu_info - pdu info for tcp ddp crc Tx offload.
+ *
+ * @end_seq: tcp seq of the last byte in the pdu.
+ * @start_seq: tcp seq of the first byte in the pdu.
+ * @data_len: pdu data size (in bytes).
+ * @hdr_len: the size (in bytes) of the pdu header.
+ * @hdr: pdu header.
+ * @req: the ulp request for the original pdu.
+ */
+struct ulp_ddp_pdu_info {
+ struct list_head list;
+ u32 end_seq;
+ u32 start_seq;
+ u32 data_len;
+ u32 hdr_len;
+ void *hdr;
+ struct request *req;
Not sure what ddp does with this, but it shouldn't accept struct
request what-so-ever.
But to be honest, the previous one was already mostly unreviewable,
but this is now far beyond this. Please try to get anything that
is generally useful first in smaller series and the come back with
a somewhat reviewable series. That also means that at least for the
code I care about (nvme) the patches should be grouped together,
and actually provide meaningful functionality in each patch. Right
now even trying to understand what you add to the nvme code requires
me to jump all over a gigantic series.
I agree as well. It is difficult to review.
The order should be:
1. ulp_ddp interface
2. nvme-tcp changes
3. mlx5e changes
Also even beyond grouping patches together I have 2 requests:
1. Please consolidate ddp routines under a single ifdef (also minimize
the ifdef in call-sites).
2. When consolidating functions, try to do this as prep patches
documenting in the change log that it is preparing to add ddp. Its
difficult digesting both at times.
From: Or Gerlitz <hidden> Date: 2021-08-04 13:27:06
On Fri, Jul 23, 2021 at 9:09 AM Christoph Hellwig [off-list ref] wrote:
On Thu, Jul 22, 2021 at 02:02:53PM +0300, Boris Pismenny wrote:
quoted
From: Boris Pismenny <redacted>
get_netdev_for_sock is a utility that is used to obtain
the net_device structure from a connected socket.
Later patches will use this for nvme-tcp DDP and DDP DDGST offloads.
Signed-off-by: Boris Pismenny <redacted>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
I don't think this should be an inline. Please move it to net/core/dev.c,
andd add an EXPORT_SYMBOL_GPL and a kerneldoc comment.
From: Or Gerlitz <hidden> Date: 2021-08-04 13:51:53
On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg [off-list ref] wrote:
[.. ] It is difficult to review.
The order should be:
1. ulp_ddp interface
2. nvme-tcp changes
3. mlx5e changes
.. and this is exactly how the series is organized, for v6 we will drop the
TX offload part and stick to completing the review on the RX offload part.
Also even beyond grouping patches together I have 2 requests:
1. Please consolidate ddp routines under a single ifdef (also minimize
the ifdef in call-sites).
ok, will make an effort to be better in that respect
2. When consolidating functions, try to do this as prep patches
documenting in the change log that it is preparing to add ddp. Its
difficult digesting both at times.
to clarify, you would like patch #5 "nvme-tcp: Add DDP offload control path"
to only add the call sites and if-not-deffed implementation for the added knobs:
nvme_tcp_offload_socket
nvme_tcp_unoffload_socket
nvme_tcp_offload_limits
nvme_tcp_resync_response
and a 2nd patch to add the if-yes-deffed implementation?
This makes sense, however IMHO repeating this prep exercise for
the data-path patch (#6 "nvme-tcp: Add DDP data-path") doesn't
seem to provide notable value b/c you will only see two call sites
for the two added empty knobs:
nvme_tcp_setup_ddp
nvme_tcp_teardown_ddp
but whatever you prefer, so.. let us know
From: Or Gerlitz <hidden> Date: 2021-08-04 14:15:01
On Fri, Jul 23, 2021 at 8:30 AM Al Viro [off-list ref] wrote:
On Fri, Jul 23, 2021 at 07:03:02AM +0200, Christoph Hellwig wrote:
quoted
On Thu, Jul 22, 2021 at 11:23:38PM +0300, Boris Pismenny wrote:
quoted
quoted
This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same.
quoted
Now why can't we just make that change to the generic routine?
Doable... replace memcpy(base, addr + off, len) with
base != addr + off && memcpy(base, addr + off, len)
in _copy_to_iter() and be done with that...
Guys,
AFAIR we did the adding ddp_ prefix exercise to the copy functions call chain
ddp_hash_and_copy_to_iter
-> ddp_copy_to_iter
-> _ddp_copy_to_iter
-> ddp_memcpy_to_page
to address feedback given on earlier versions of the series. So let's
decide please.. are we all set to remove the ddp_ prefixed calls and just
plant the new check (plus a nice comment!) as Al suggested?
re the comments given on ddp_memcpy_to_page, upstream move
to just call memcpy, so we need not have it anyway, will be fixed in v6
if we remain with ddp_ call chain or becomes irrelevant if we drop it.
On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg [off-list ref] wrote:
quoted
[.. ] It is difficult to review.
The order should be:
1. ulp_ddp interface
2. nvme-tcp changes
3. mlx5e changes
.. and this is exactly how the series is organized, for v6 we will drop the
TX offload part and stick to completing the review on the RX offload part.
quoted
Also even beyond grouping patches together I have 2 requests:
1. Please consolidate ddp routines under a single ifdef (also minimize
the ifdef in call-sites).
ok, will make an effort to be better in that respect
quoted
2. When consolidating functions, try to do this as prep patches
documenting in the change log that it is preparing to add ddp. Its
difficult digesting both at times.
to clarify, you would like patch #5 "nvme-tcp: Add DDP offload control path"
to only add the call sites and if-not-deffed implementation for the added knobs:
nvme_tcp_offload_socket
nvme_tcp_unoffload_socket
nvme_tcp_offload_limits
nvme_tcp_resync_response
and a 2nd patch to add the if-yes-deffed implementation?
This makes sense, however IMHO repeating this prep exercise for
the data-path patch (#6 "nvme-tcp: Add DDP data-path") doesn't
seem to provide notable value b/c you will only see two call sites
for the two added empty knobs:
nvme_tcp_setup_ddp
nvme_tcp_teardown_ddp
but whatever you prefer, so.. let us know
I was more referring to routines that now grew the ddp path
and changed in the same time like:
nvme_tcp_complete_request
nvme_tcp_consume_skb
etc..
From: Or Gerlitz <hidden> Date: 2021-08-10 13:30:27
On Wed, Aug 4, 2021 at 5:13 PM Or Gerlitz [off-list ref] wrote:
On Fri, Jul 23, 2021 at 8:30 AM Al Viro [off-list ref] wrote:
quoted
On Fri, Jul 23, 2021 at 07:03:02AM +0200, Christoph Hellwig wrote:
quoted
On Thu, Jul 22, 2021 at 11:23:38PM +0300, Boris Pismenny wrote:
quoted
quoted
quoted
This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same.
quoted
quoted
Now why can't we just make that change to the generic routine?
quoted
Doable... replace memcpy(base, addr + off, len) with
base != addr + off && memcpy(base, addr + off, len)
in _copy_to_iter() and be done with that...
Guys,
AFAIR we did the adding ddp_ prefix exercise to the copy functions call chain
ddp_hash_and_copy_to_iter
-> ddp_copy_to_iter
-> _ddp_copy_to_iter
-> ddp_memcpy_to_page
to address feedback given on earlier versions of the series. So let's
decide please.. are we all set to remove the ddp_ prefixed calls and just
plant the new check (plus a nice comment!) as Al suggested?
So we are okay going for the minimal approach / direction suggested by
Al of adding a (base != addr + offset) check before the memcpy call.
This will also simplify the changes to the nvme-tcp driver. Please
speak if you want the ddp_ prefix approach to remain.
Or.
re the comments given on ddp_memcpy_to_page, upstream move
to just call memcpy, so we need not have it anyway, will be fixed in v6
if we remain with ddp_ call chain or becomes irrelevant if we drop it.
From: Or Gerlitz <hidden> Date: 2021-08-10 13:38:14
On Fri, Aug 6, 2021 at 10:46 PM Sagi Grimberg [off-list ref] wrote:
On 8/4/21 6:51 AM, Or Gerlitz wrote:
quoted
On Fri, Jul 23, 2021 at 10:59 PM Sagi Grimberg [off-list ref] wrote:
quoted
quoted
[.. ] It is difficult to review.
The order should be:
1. ulp_ddp interface
2. nvme-tcp changes
3. mlx5e changes
.. and this is exactly how the series is organized, for v6 we will drop the
TX offload part and stick to completing the review on the RX offload part.
quoted
Also even beyond grouping patches together I have 2 requests:
1. Please consolidate ddp routines under a single ifdef (also minimize
the ifdef in call-sites).
ok, will make an effort to be better in that respect
quoted
2. When consolidating functions, try to do this as prep patches
documenting in the change log that it is preparing to add ddp. Its
difficult digesting both at times.
to clarify, you would like patch #5 "nvme-tcp: Add DDP offload control path"
to only add the call sites and if-not-deffed implementation for the added knobs:
nvme_tcp_offload_socket
nvme_tcp_unoffload_socket
nvme_tcp_offload_limits
nvme_tcp_resync_response
and a 2nd patch to add the if-yes-deffed implementation?
This makes sense, however IMHO repeating this prep exercise for
the data-path patch (#6 "nvme-tcp: Add DDP data-path") doesn't
seem to provide notable value b/c you will only see two call sites
for the two added empty knobs:
nvme_tcp_setup_ddp
nvme_tcp_teardown_ddp
but whatever you prefer, so.. let us know
I was more referring to routines that now grew the ddp path
and changed in the same time like:
nvme_tcp_complete_request
not sure to follow on this one.. It's added on patch #6 "nvme-tcp: Add
DDP data-path"
and then used twice in the same patch replacing calls to nvme_try_complete_req
and then to nvme_complete_rq -- so how want this be broken to prep/usage?
nvme_tcp_consume_skb
this routine was born due to the ddp_ prefix addition to the iov copy
iter functions, which we are now removing due to feedback from Al