[ovs-dev] [PATCH v8 10/13] dp-packet: copy data from multi-seg. DPDK mbuf

Eelco Chaudron echaudro at redhat.com
Tue Jun 26 13:00:20 UTC 2018



On 22 Jun 2018, at 21:05, Lam, Tiago wrote:

> On 18/06/2018 14:10, Eelco Chaudron wrote:
>>
>>
>> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
>>
>>> From: Michael Qiu <qiudayu at chinac.com>
>>>
>>> When doing packet clone, if packet source is from DPDK driver,
>>> multi-segment must be considered, and copy the segment's data one by
>>> one.
>>>
>>> Also, lots of DPDK mbuf's info is missed during a copy, like packet
>>> type, ol_flags, etc.  That information is very important for DPDK to
>>> do
>>> packets processing.
>>>
>>> Co-authored-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
>>>
>>> Signed-off-by: Michael Qiu <qiudayu at chinac.com>
>>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
>>> ---
>>>  lib/dp-packet.c   | 76
>>> +++++++++++++++++++++++++++++++++++++++++++++++--------
>>>  lib/dp-packet.h   |  3 +++
>>>  lib/netdev-dpdk.c |  1 +
>>>  3 files changed, 69 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>>> index d0fab94..2e65b82 100644
>>> --- a/lib/dp-packet.c
>>> +++ b/lib/dp-packet.c
>>> @@ -48,6 +48,23 @@ dp_packet_use__(struct dp_packet *b, void *base,
>>> size_t allocated,
>>>      dp_packet_set_size(b, 0);
>>>  }
>>>
>>> +#ifdef DPDK_NETDEV
>>> +void
>>> +dp_packet_copy_mbuf_flags(struct dp_packet *dst, const struct
>>> dp_packet *src)
>>> +{
>>> +    ovs_assert(dst != NULL && src != NULL);
>>> +    struct rte_mbuf *buf_dst = &(dst->mbuf);
>>> +    struct rte_mbuf buf_src = src->mbuf;
>>> +
>>> +    buf_dst->nb_segs = buf_src.nb_segs;
>>> +    buf_dst->ol_flags = buf_src.ol_flags;
>>> +    buf_dst->packet_type = buf_src.packet_type;
>>> +    buf_dst->tx_offload = buf_src.tx_offload;
>>> +}
>>> +#else
>>> +#define dp_packet_copy_mbuf_flags(arg1, arg2)
>>> +#endif
>>> +
>>>  /* Initializes 'b' as an empty dp_packet that contains the
>>> 'allocated' bytes of
>>>   * memory starting at 'base'.  'base' should be the first byte of a
>>> region
>>>   * obtained from malloc().  It will be freed (with free()) if 'b' is
>>> resized or
>>> @@ -158,6 +175,50 @@ dp_packet_clone(const struct dp_packet *buffer)
>>>      return dp_packet_clone_with_headroom(buffer, 0);
>>>  }
>>>
>>> +#ifdef DPDK_NETDEV
>>> +struct dp_packet *
>>> +dp_packet_clone_with_headroom(const struct dp_packet *buffer,
>>> +                              size_t headroom) {
>>> +    struct dp_packet *new_buffer;
>>> +    uint32_t pkt_len = dp_packet_size(buffer);
>>> +
>>> +    /* copy multi-seg data */
>>> +    if (buffer->source == DPBUF_DPDK && buffer->mbuf.nb_segs > 1) {
>>> +        uint32_t offset = 0;
>>> +        void *dst = NULL;
>>> +        struct rte_mbuf *tmbuf = CONST_CAST(struct rte_mbuf *,
>>> +                                                &(buffer->mbuf));
>>> +
>>> +        new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
>>> +        dp_packet_set_size(new_buffer, pkt_len + headroom);
>>> +        dst = dp_packet_tail(new_buffer);
>>> +
>>
>> The dst is not pointing to the first data, but to the end of the data,
>> as dp_packet_set_size() has been called.  Why not just call dst =
>> dp_packet_put_uninit().
>
> I'm taking your suggestion below, so this will be removed.
>
>>
>>> +        while (tmbuf) {
>>> +            rte_memcpy((char *)dst + offset,
>>> +                       rte_pktmbuf_mtod(tmbuf, void *),
>>> tmbuf->data_len);
>>> +            offset += tmbuf->data_len;
>>> +            tmbuf = tmbuf->next;
>>
>> Why not using rte_pktmbuf_read() here with a check its not a contiguous
>> read?
>>
>
> Good point! We can use `rte_pktmbuf_read()` to read the data to
> `new_buffer`. It should be OK since in this clause we have always more
> than 1 mbuf in the chain.

Please add a check to be sure!


More information about the dev mailing list