[ovs-dev] [RFC PATCH V4 3/9] dp-packet: fix put_uninit for multi-seg mbufs

[Chandran, Sugesh]

Regards
_Sugesh


> -----Original Message-----
> From: Kavanagh, Mark B
> Sent: Friday, December 8, 2017 12:02 PM
> To: dev at openvswitch.org; qiudayu at chinac.com
> Cc: Stokes, Ian <ian.stokes at intel.com>; Loftus, Ciara <ciara.loftus at intel.com>;
> santosh.shukla at caviumnetworks.com; Chandran, Sugesh
> <sugesh.chandran at intel.com>; Kavanagh, Mark B
> <mark.b.kavanagh at intel.com>
> Subject: [ovs-dev][RFC PATCH V4 3/9] dp-packet: fix put_uninit for multi-seg
> mbufs
> 
> dp_packet_put_uninit(dp_packet, size) appends 'size' bytes to the tail of a
> dp_packet. In the case of multi-segment mbufs, it is the data length of the last
> mbuf in the mbuf chain that should be adjusted by 'size' bytes.
> 
> In its current implementation, dp_packet_put_uninit() adjusts the dp_packet's
> size via a call to dp_packet_set_size(); however, this adjusts the data length of
> the first mbuf in the chain, which is incorrect in the case of multi-segment
> mbufs. Instead, traverse the mbuf chain to locate the final mbuf of said chain,
> and update its data_len[1]. To finish, increase the packet length of the entire
> mbuf[2] by 'size'.
> 
> [1] In the case of a single-segment mbuf, this is the mbuf itself.
> [2] This is stored in the first mbuf of an mbuf chain.
> 
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
> ---
>  lib/dp-packet.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index 443c225..ad71486 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -322,7 +322,27 @@ dp_packet_put_uninit(struct dp_packet *b, size_t size)
>      void *p;
>      dp_packet_prealloc_tailroom(b, size);
>      p = dp_packet_tail(b);
> +#ifdef DPDK_NETDEV
> +    if (b->source == DPBUF_DPDK) {
> +        struct rte_mbuf *buf = &(b->mbuf);
> +        /* In the case of multi-segment mbufs, the data length of the last mbuf
> +         * should be adjusted by 'size' bytes. A call to dp_packet_size() would
> +         * adjust the data length of the first mbuf in the segment, so we avoid
> +         * invoking same; as a result, the packet length of the entire mbuf
> +         * chain (stored in the first mbuf of said chain) must be adjusted here
> +         * instead.
> +         */
> +        while (buf->next) {
> +            buf = buf->next;
> +        }
> +        buf->data_len += size;
[Sugesh] Just to confirm, should we have enough room in buf to accommodate the new size? If not, what will happen?
Will it allocate a new mbuf at last? Do you think it's a valid case to happen? My understanding is always mbuf data are over-provisioned previously.
The multiple mbuf case, we are allocating the exact size of packet. Wondering if it make any issues? Please ignore this if its not a valid case at all.
 
> +        b->mbuf.pkt_len += size;
> +    } else {
> +#endif
>      dp_packet_set_size(b, dp_packet_size(b) + size);
> +#ifdef DPDK_NETDEV
> +    }
> +#endif
>      return p;
>  }
> 
> --
> 1.9.3