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

[Kavanagh, Mark B]

>From: Chandran, Sugesh
>Sent: Friday, December 8, 2017 6:00 PM
>To: Kavanagh, Mark B <mark.b.kavanagh at intel.com>; 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
>Subject: RE: [ovs-dev][RFC PATCH V4 3/9] dp-packet: fix put_uninit for multi-
>seg mbufs
>
>
>
>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?

dp_packet_prealloc_tailroom() checks that there is enough tailroom in the mbuf to increase the size accordingly, and reallocates the dp_packet if not (if the dp_packet's memory source is not DPDK).
However, if the packet memory for the dp_packet comes from DPDK, then dp_packet_resize() triggers an abort(); this behavior already exists in OvS, and this patchset does not change that.

Your question has flagged to me however, that some additional functions may need to be modified to accommodate multi-segment mbufs (dp_packet_end(), dp_packet_tail(), for instance).

Thanks,
Mark

>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