[ovs-dev] [PATCH v12 2/3] dp-packet: Init specific mbuf fields.

[Lam, Tiago]

On 02/11/2018 10:35, Ilya Maximets wrote:
> On 02.11.2018 12:06, Tiago Lam wrote:
>> From: Mark Kavanagh <mark.b.kavanagh at intel.com>
>>
>> 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 <qiudayu at chinac.com>:
>> https://patchwork.ozlabs.org/patch/777570/
>>
>> Co-authored-by: Tiago Lam <tiago.lam at intel.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>
>> Acked-by: Flavio Leitner <fbl at sysclose.org>
>> ---
>>  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
>>