-----Original Message-----
From: <[email protected]> on behalf of "Chandran, Sugesh"
<[email protected]>
Date: Wednesday, August 16, 2017 at 6:55 AM
To: Darrell Ball <[email protected]>, "[email protected]"
<[email protected]>
Subject: Re: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet
initialization.
Hi Darrel,
Thank you for the patch.
Please find my comments below.
Regards
_Sugesh
> -----Original Message-----
> From: [email protected] [mailto:ovs-dev-
> [email protected]] On Behalf Of Darrell Ball
> Sent: Tuesday, August 15, 2017 5:14 PM
> To: [email protected]; [email protected]
> Subject: [ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet
> initialization.
>
> DPDK uses dp-packet pools and manages the mbuf portion of each packet.
> When a pool is created, partial initialization is also done on the OVS
portion
> (i.e. non-mbuf). Since packet memory is reused, this is not very useful
for
> transient fields and is also misleading. Furthermore, some of these
transient
> fields are properly initialized for DPDK packets entering OVS anyways,
which
> is the only reasonable way to do this.
> Another field, cutlen, is initialized in this manner in the pool and
intended to
> be reset when cutlen is applied on sending the packet out. However, if
> cutlen context is set but the packet is not sent out for some reason,
then the
> packet header would be corrupted in the memory pool. It is better to just
> reset the cutlen in the packets when received. I did not detect a
degradation
> in performance, however, I would be willing to have some degradation,
since
[Sugesh] I also did some testing in our lab with your patch and didn't find
any degradation as such.
The approach looks fine to me. Only one concern is about init the cutlen
for all the packet structure.
It looks to me bit of overhead even I don't see any performance degradation
as such in my testing.
Similarly the md strcture of dp_packet doesn't init completely due to the
performance reason.
My understanding is the dp_packet can have corrupted uninitialized data and
it is not necessary to init
all the fields.
[Darrell] In general, it goes without saying that not everything needs init; it
is case by case.
So should that be ok to leave out this init ?
[Darrell]: cutlen corrupts packets (although there is a legitimate use case)
so we need to extra careful here.
I think cutlen needs init because we cannot track all future
error corner cases where it is not reset in
the dp-packet header.
Since both of us don’t see any degradation by initing it, I
would like to keep the initialization. I would even keep it
with some small degradation, since it is that important.
> this is a proper way to handle this. In addition to initializing cutlen
in received
> packets, the other OVS transient fields are removed from the DPDK pool
> initialization.
>
> Signed-off-by: Darrell Ball <[email protected]>
> ---
> lib/dp-packet.c | 16 +++++++++-------
> lib/dp-packet.h | 10 ++++++++++
> lib/netdev-dpdk.c | 2 ++
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c index c1f43f3..312f63a
100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -92,16 +92,18 @@ dp_packet_use_const(struct dp_packet *b, const
> void *data, size_t size)
> dp_packet_set_size(b, size);
> }
>
> -/* Initializes 'b' as an empty dp_packet that contains the 'allocated'
bytes of
> - * memory starting at 'base'. DPDK allocated dp_packet and *data is
> allocated
> - * from one continous memory region, so in memory data start right after
> - * dp_packet. Therefore there is special method to free this type of
> - * buffer. dp_packet base, data and size are initialized by dpdk rcv()
so no
> - * need to initialize those fields. */
> +/* Initializes 'b' as an empty dp_packet that contains the 'allocated'
bytes.
> + * DPDK allocated dp_packet and *data is allocated from one continous
> +memory
> + * region as part of memory pool, so in memory data start right after
> + * dp_packet. Therefore, there is a special method to free this type
> +of
> + * buffer. Here, non-transient ovs dp-packet fields are initialized
> +for
> + * packets that are part of a DPDK memory pool. */
> void
> dp_packet_init_dpdk(struct dp_packet *b, size_t allocated) {
> - dp_packet_init__(b, allocated, DPBUF_DPDK);
> + dp_packet_set_allocated(b, allocated);
> + b->source = DPBUF_DPDK;
> + b->packet_type = htonl(PT_ETH);
> }
>
> /* Initializes 'b' as an empty dp_packet with an initial capacity of
'size'
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index bb3f9db..cbeb831
100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -801,6 +801,16 @@ dp_packet_delete_batch(struct dp_packet_batch
> *batch, bool may_steal) }
>
> static inline void
> +dp_packet_batch_init_cutlen(struct dp_packet_batch *batch) {
> + struct dp_packet *packet;
> +
> + DP_PACKET_BATCH_FOR_EACH (packet, batch) {
> + dp_packet_reset_cutlen(packet);
> + }
> +}
> +
> +static inline void
> dp_packet_batch_apply_cutlen(struct dp_packet_batch *batch) {
> if (batch->trunc) {
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..92453b2
> 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1644,6 +1644,7 @@ netdev_dpdk_vhost_rxq_recv(struct netdev_rxq
> *rxq,
> nb_rx, dropped);
> rte_spinlock_unlock(&dev->stats_lock);
>
> + dp_packet_batch_init_cutlen(batch);
> batch->count = (int) nb_rx;
> return 0;
> }
> @@ -1683,6 +1684,7 @@ netdev_dpdk_rxq_recv(struct netdev_rxq *rxq,
> struct dp_packet_batch *batch)
> rte_spinlock_unlock(&dev->stats_lock);
> }
>
> + dp_packet_batch_init_cutlen(batch);
> batch->count = nb_rx;
>
> return 0;
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> [email protected]
>
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=8F2TyXQLRSV1ShhsQ4-5RM9Y-bLOTVfMzlTnkPlQ90Y&s=mXjrw5j6ezo9KBACYhQb5jqIk3IM61O_xEhDXQ0qZTE&e=
_______________________________________________
dev mailing list
[email protected]
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=8F2TyXQLRSV1ShhsQ4-5RM9Y-bLOTVfMzlTnkPlQ90Y&s=mXjrw5j6ezo9KBACYhQb5jqIk3IM61O_xEhDXQ0qZTE&e=
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
