[ovs-dev] [PATCH v10 08/14] dp-packet: copy data from multi-seg. DPDK mbuf
Lam, Tiago
tiago.lam at intel.com
Fri Oct 5 14:50:54 UTC 2018
On 03/10/2018 19:26, Flavio Leitner wrote:
> On Fri, Sep 28, 2018 at 05:15:09PM +0100, 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>
>> Co-authored-by: Tiago Lam <tiago.lam at intel.com>
>>
>> Signed-off-by: Michael Qiu <qiudayu at chinac.com>
>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh at intel.com>
>> Signed-off-by: Tiago Lam <tiago.lam at intel.com>
>> Acked-by: Eelco Chaudron <echaudro at redhat.com>
>> ---
>> lib/dp-packet.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++---------
>> lib/dp-packet.h | 3 +++
>> lib/netdev-dpdk.c | 1 +
>> 3 files changed, 62 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 167bf43..806640b 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -48,6 +48,22 @@ 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->ol_flags = buf_src.ol_flags;
>> + buf_dst->packet_type = buf_src.packet_type;
>> + buf_dst->tx_offload = buf_src.tx_offload;
>
> Nit: use dereferencing on both sides.
>
> Also, since this is a simple function that only copies few fields, maybe it could
> be in the dp-packet.h, so that netdev-dpdk.c can inline it.
>
>> +}
>> +#else
>> +#define dp_packet_copy_mbuf_flags(arg1, arg2)
>
> Nit2: since this is very specific to mbuf, we don't need to expose it
> outside of DPDK.
>
>> +#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 +174,44 @@ 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 *b, size_t headroom) {
>> + struct dp_packet *new_buffer;
>> + uint32_t pkt_len = dp_packet_size(b);
>> +
>> + /* copy multi-seg data */
>> + if (b->source == DPBUF_DPDK && !rte_pktmbuf_is_contiguous(&b->mbuf)) {
>> + void *dst = NULL;
>> + struct rte_mbuf *mbuf = CONST_CAST(struct rte_mbuf *, &b->mbuf);
>> +
>> + new_buffer = dp_packet_new_with_headroom(pkt_len, headroom);
>> + dst = dp_packet_data(new_buffer);
>> + dp_packet_set_size(new_buffer, pkt_len);
>> +
>> + if (!rte_pktmbuf_read(mbuf, 0, pkt_len, dst)) {
>
> Here we leak new_buffer which is xmalloc'ed.
Fixed. Thanks!
>
>> + return NULL;
>> + }
>> + } else {
>> + new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(b),
>> + dp_packet_size(b),
>> + headroom);
>> + }
>> +
>> + /* Copy the following fields into the returned buffer: l2_pad_size,
>> + * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>> + memcpy(&new_buffer->l2_pad_size, &b->l2_pad_size,
>> + sizeof(struct dp_packet) -
>> + offsetof(struct dp_packet, l2_pad_size));
>> +
>> + dp_packet_copy_mbuf_flags(new_buffer, b);
>> + if (dp_packet_rss_valid(new_buffer)) {
>> + new_buffer->mbuf.hash.rss = b->mbuf.hash.rss;
>> + }
>> +
>> + return new_buffer;
>> +}
>> +#else
>> /* Creates and returns a new dp_packet whose data are copied from 'buffer'.
>> * The returned dp_packet will additionally have 'headroom' bytes of
>> * headroom. */
>> @@ -165,32 +219,25 @@ 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);
>>
>> new_buffer = dp_packet_clone_data_with_headroom(dp_packet_data(buffer),
>> - dp_packet_size(buffer),
>> - headroom);
>> + pkt_len, headroom);
>> +
>> /* Copy the following fields into the returned buffer: l2_pad_size,
>> * l2_5_ofs, l3_ofs, l4_ofs, cutlen, packet_type and md. */
>> memcpy(&new_buffer->l2_pad_size, &buffer->l2_pad_size,
>> sizeof(struct dp_packet) -
>> offsetof(struct dp_packet, l2_pad_size));
>>
>> -#ifdef DPDK_NETDEV
>> - new_buffer->mbuf.ol_flags = buffer->mbuf.ol_flags;
>> -#else
>> new_buffer->rss_hash_valid = buffer->rss_hash_valid;
>> -#endif
>> -
>> if (dp_packet_rss_valid(new_buffer)) {
>> -#ifdef DPDK_NETDEV
>> - new_buffer->mbuf.hash.rss = buffer->mbuf.hash.rss;
>> -#else
>> new_buffer->rss_hash = buffer->rss_hash;
>> -#endif
>> }
>>
>> return new_buffer;
>> }
>> +#endif
>
> Nit3: Looks like the above functions share some code which could be on
> another function or maybe move the copy specifics to a separate
> functions.
The "nits" sound good to me. I'll include them in the next iteration.
Tiago.
>
>
>> /* Creates and returns a new dp_packet that initially contains a copy of the
>> * 'size' bytes of data starting at 'data' with no headroom or tailroom. */
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index aab8f62..cbf002c 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -124,6 +124,9 @@ void dp_packet_init_dpdk(struct dp_packet *);
>> void dp_packet_init(struct dp_packet *, size_t);
>> void dp_packet_uninit(struct dp_packet *);
>>
>> +void dp_packet_copy_mbuf_flags(struct dp_packet *dst,
>> + const struct dp_packet *src);
>> +
>> struct dp_packet *dp_packet_new(size_t);
>> struct dp_packet *dp_packet_new_with_headroom(size_t, size_t headroom);
>> struct dp_packet *dp_packet_clone(const struct dp_packet *);
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 3b11546..8484239 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -2364,6 +2364,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet_batch *batch)
>> memcpy(rte_pktmbuf_mtod(pkts[txcnt], void *),
>> dp_packet_data(packet), size);
>> dp_packet_set_size((struct dp_packet *)pkts[txcnt], size);
>> + dp_packet_copy_mbuf_flags((struct dp_packet *)pkts[txcnt], packet);
>>
>> txcnt++;
>> }
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list