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

[Aurelien Aptel <aaptel@nvidia.com>]

Hi,

The mlx5 driver allocates linear and non-linear SKBs depending on
certain condition, MTU size being one of them. The choice is a trade-off
based on many parameters and performances.

Your suggested change affects the non-linear code path.  If we increase
the MTU size and use non-linear SKB, we don't even require your change
at the moment as the tailroom is already too small to be condensed.

The problematic part is the linear SKB code path. With a default MTU
size of 1500, the driver allocates a linear SKB. We receive the CQE from
the NIC and we allocate a linear SKB in mlx5e_build_linear_skb().

static inline                                                 
struct sk_buff *mlx5e_build_linear_skb(struct mlx5e_rq *rq, void *va,
                                       u32 frag_size, u16 headroom,
                                       u32 cqe_bcnt, u32 metasize)
{
	struct sk_buff *skb = napi_build_skb(va, frag_size);
	...
	skb_reserve(skb, headroom);
	skb_put(skb, cqe_bcnt);
        ...
}

The CQE might hold multiple NVME PDUs. It is not just a single NVME pdu
with offloaded data. After the data, there can be the CRC, and other
complete or incomplete NVME pdus.

Later the mlx5 offload code, we have
mlx5e_nvmeotcp_rebuild_rx_skb_linear() which will rearrange the SKB to
use the previously registered buffers the NVME-TCP layer gave us.

So mlx5e_nvmeotcp_rebuild_rx_skb_linear() takes a SKB like this:

    skb
    head   data                                            tail   end
    |       |                                               |      |
    v       v  <----------- cqe_bcnt -------------------->  v      v
    +-------+----hdr-----+-offloaded-data-+-next-nvme-pdu---+------+
                                          ^
                                         rest
       frag_list = n/a (linear)

And turns it to this:   

    skb
    head   data         tail                                      end
    |       |            |                                         |
    v       v            v                                         v
    +-------+----hdr-----+-offloaded-data-+-next-nvme-pdu----------+
                                          ^
                                         rest
       frag_list[0] = nvme-tcp buffer
       frag_list[1] = rest

And then passes the skb up the network stack. In the big picture the
complete length of the skb does not change.

If this skb is then condensed, it is going to put back the frag_list at
end the tail which we don't want.

We could do what you suggested (adapted to linear), move skb->data
forward by increasing the headroom resulting in a smaller tailroom, but
we need at least cqe_bcnt bytes, but having that much also allows
condensing, which we *don't* want.

struct sk_buff *mlx5e_build_linear_skb(struct mlx5e_rq *rq, void *va,
                                       u32 frag_size, u16 headroom,
                                       u32 cqe_bcnt, u32 metasize)
{
 	struct sk_buff *skb = napi_build_skb(va, frag_size);
 	...
+	if (is_ddp) {
+		headroom = skb_tailroom(skb) - cqe_bcnt;
+	}

 	skb_reserve(skb, headroom);
 	skb_put(skb, cqe_bcnt);
        ...
}

If we leave an even smaller tailroom, then we lose the data after the PDU.

So we are stuck, this is why we need the condense bit.

Thanks