[ovs-dev] [RFC patch v1] dp-packet: Refactor DPDK packet initialization.

[Chandran, Sugesh]

Hi Darrel,
Thank you for the patch.
Please find my comments below.

Regards
_Sugesh


> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Darrell Ball
> Sent: Tuesday, August 15, 2017 5:14 PM
> To: dlu998 at gmail.com; dev at openvswitch.org
> 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. So should that be ok to leave out this init ?


> 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 <dlu998 at gmail.com>
> ---
>  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev