[ovs-dev] [PATCH] netdev-dpdk: reset packet_type for reused dp_packets

Zoltán Balogh zoltan.balogh at ericsson.com
Fri Sep 8 08:45:23 UTC 2017


Hi Darrell,

Thank you for your comments! I created a second version and sent it to the dev list:
https://patchwork.ozlabs.org/patch/811387/

Best regards,
Zoltan

> -----Original Message-----
> From: Darrell Ball [mailto:dball at vmware.com]
> Sent: Thursday, September 07, 2017 12:45 AM
> To: Zoltán Balogh <zoltan.balogh at ericsson.com>; 'dev at openvswitch.org' <dev at openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH] netdev-dpdk: reset packet_type for reused dp_packets
> 
> 
> 
> On 9/6/17, 5:12 AM, "ovs-dev-bounces at openvswitch.org on behalf of Zoltán Balogh" <ovs-dev-bounces at openvswitch.org on
> behalf of zoltan.balogh at ericsson.com> wrote:
> 
>     DPDK uses dp-packet pool for storing received packets. The pool is
>     reused by rxq_recv funcions of the DPDK netdevs. The datapath is
>     capable to modify the packet_type property of packets. For instance
>     when encapsulated L3 packets are received on a ptap gre port.
>     In this case the packet_type property of struct dp_packet can be
>     modified and later the same dp_packet with the modified packet_type
>     can be reused in the rxq_rec function, so it can contain corrupted
>     data.
> 
>     The dp_packet_batch_init_cutlen() in the rxq_recv functions iterates
>     over dp_packets and sets their cutlen. So I modified this function
>     to set packet_type to Ethernet for the dp_packets as well. I also
>     renamed this function because of the added functionality.
> 
>     The dp_packet_batch_init_cutlen() iterates over batch->count dp_packet.
>     Therefore setting of batch->count = nb_rx needs to be done before the
>     former function is invoked. This is an additional fix.
> 
>     Signed-off-by: Zoltán Balogh <zoltan.balogh at ericsson.com>
>     Signed-off-by: László Sürü <laszlo.suru at ericsson.com>
>     Co-authored-by: László Sürü <laszlo.suru at ericsson.com>
>     CC: Jan Scheurich <jan.scheurich at ericsson.com>
>     CC: Sugesh Chandran <sugesh.chandran at intel.com>
>     CC: Darrell Ball <dlu998 at gmail.com>
>     ---
>      lib/dp-packet.h   | 4 +++-
>      lib/netdev-dpdk.c | 5 +++--
>      2 files changed, 6 insertions(+), 3 deletions(-)
> 
>     diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>     index 046f3ab..0046d0a 100644
>     --- a/lib/dp-packet.h
>     +++ b/lib/dp-packet.h
>     @@ -805,12 +805,14 @@ 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)
>     +dp_packet_batch_init_transient_fields(struct dp_packet_batch *batch)
> 
> [Darrell]
> Can we just call this function dp_packet_batch_init_packet_fields() now that it has wider scope than just cutlen,
> as the name makes it clear that we are initing the individual packet fields of a batch, rather than the batch
> context fields ?
> ‘transient’ is maybe implied ?
> 
> 
>      {
>          struct dp_packet *packet;
> 
>          DP_PACKET_BATCH_FOR_EACH (packet, batch) {
>              dp_packet_reset_cutlen(packet);
>     +        /* Set packet_type to Ethernet. */
>     +        packet->packet_type = PACKET_TYPE_BE(OFPHTN_ONF, 0x0000);
> 
> 
> [Darrell] The original dp_packet init code for PTAP has
>  b->packet_type = htonl(PT_ETH);
> This form seems preferred and intuitive (comment would even be unnecessary); can we use this form ?
> 
> Also, the original dp_packet init code for PTAP was in dp_packet_init__ and then moved to dp_packet_init_dpdk
> I think we can just remove it from there, as it is redundant with the new code.
> 
> 
>          }
>      }
> 
>     diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>     index f58e9be..2422741 100644
>     --- a/lib/netdev-dpdk.c
>     +++ b/lib/netdev-dpdk.c
>     @@ -1644,8 +1644,9 @@ 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;
> 
> [Darrell] Although nothing to do with this patch, can we remove the cast ‘(int)’ above ?
> 
> 
>     +    dp_packet_batch_init_transient_fields(batch);
> 
> [Darrell]
> Good catch; this should work better than initializing 0 of the entries (
> 
>     +
>          return 0;
>      }
> 
>     @@ -1684,8 +1685,8 @@ 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;
>     +    dp_packet_batch_init_transient_fields(batch);
> 
> [Darrell]
> Same comment as above.
> 
>          return 0;
>      }
>     --
>     1.9.1
> 
>     _______________________________________________
>     dev mailing list
>     dev at openvswitch.org
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-
> 2Ddev&d=DwIFAw&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=zcjvUT1lz85oUJWStkTlRzIdXuQnJ4HL-
> n6oiK99iuM&s=KmEO2izD43oNVigXXu6TlPCLNruTmVtKMGdxVb6ANYs&e=
> 



More information about the dev mailing list