On 18/06/2018 14:10, Eelco Chaudron wrote:
>
>
> On 11 Jun 2018, at 18:21, Tiago Lam wrote:
>
>> From: Michael Qiu <qiud...@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.kavan...@intel.com>
>>
>> Signed-off-by: Michael Qiu <qiud...@chinac.com>
>> Signed-off-by: Mark Kavanagh <mark.b.kavan...@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.
Tiago.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
