On 02/11/2018 10:35, Ilya Maximets wrote: > On 02.11.2018 12:06, Tiago Lam wrote: >> From: Mark Kavanagh <[email protected]> >> >> dp_packets are created using xmalloc(); in the case of OvS-DPDK, it's >> possible the the resultant mbuf portion of the dp_packet contains >> random data. For some mbuf fields, specifically those related to >> multi-segment mbufs and/or offload features, random values may cause >> unexpected behaviour, should the dp_packet's contents be later copied >> to a DPDK mbuf. It is critical therefore, that these fields should be >> initialized to 0. >> >> This patch ensures that the following mbuf fields are initialized to >> appropriate values on creation of a new dp_packet: >> - ol_flags=0 >> - nb_segs=1 >> - tx_offload=0 >> - packet_type=0 >> - next=NULL > > I don't understand why we need this, at least without multi-segs. > malloced packets never reaches dpdk library functions. We're always > re-allocating them. The only exception is the call to egress policer, > but we can clear some fields right before it inside dpdk_do_tx_copy().
These won't be objective conclusions, it will have to do with your preference vs mine. In my opinion, independent of multi-seg mbufs, it is best to initialize these field members to proper values, instead of carrying on with random / garbage data. Why do it in a specific place and then potentially forget about it in some other place later on? Tiago. > > We're only using ol_flags, but this field already cleared in current code. > > In general, I'd like to drop these 'mbuf' related references in generic > code at all like it done in my recent patch-set: > https://patchwork.ozlabs.org/patch/990181/ > > Best regards, Ilya Maximets. > >> >> Adapted from an idea by Michael Qiu <[email protected]>: >> https://patchwork.ozlabs.org/patch/777570/ >> >> Co-authored-by: Tiago Lam <[email protected]> >> >> Signed-off-by: Mark Kavanagh <[email protected]> >> Signed-off-by: Tiago Lam <[email protected]> >> Acked-by: Eelco Chaudron <[email protected]> >> Acked-by: Flavio Leitner <[email protected]> >> --- >> lib/dp-packet.h | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/lib/dp-packet.h b/lib/dp-packet.h >> index 5b4c9c7..fe34d04 100644 >> --- a/lib/dp-packet.h >> +++ b/lib/dp-packet.h >> @@ -498,14 +498,14 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet *p) >> p->mbuf.ol_flags &= ~PKT_RX_RSS_HASH; >> } >> >> -/* This initialization is needed for packets that do not come >> - * from DPDK interfaces, when vswitchd is built with --with-dpdk. >> - * The DPDK rte library will still otherwise manage the mbuf. >> - * We only need to initialize the mbuf ol_flags. */ >> +/* This initialization is needed for packets that do not come from DPDK >> + * interfaces, when vswitchd is built with --with-dpdk. */ >> static inline void >> dp_packet_mbuf_init(struct dp_packet *p) >> { >> - p->mbuf.ol_flags = 0; >> + p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0; >> + p->mbuf.nb_segs = 1; >> + p->mbuf.next = NULL; >> } >> >> static inline bool >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
