[ovs-dev] [PATCH v8 02/13] dp-packet: Init specific mbuf fields.

[Lam, Tiago]

On 18/06/2018 12:23, Eelco Chaudron wrote:
> 
> 
> On 11 Jun 2018, at 18:21, 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
>>
>> 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>
>> ---
>>  lib/dp-packet.h | 8 +++++---
>>  1 file changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 596cfe6..82e45ad 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -626,13 +626,15 @@ dp_packet_mbuf_rss_flag_reset(struct dp_packet 
>> *p OVS_UNUSED)
>>
>>  /* 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. */
>> + * The DPDK rte library will still otherwise manage the mbuf. */
>>  static inline void
>>  dp_packet_mbuf_init(struct dp_packet *p OVS_UNUSED)
>>  {
>>  #ifdef DPDK_NETDEV
>> -    p->mbuf.ol_flags = 0;
>> +    struct rte_mbuf *mbuf = &(p->mbuf);
>> +    mbuf->ol_flags = mbuf->tx_offload = mbuf->packet_type = 0;
>> +    mbuf->nb_segs = 1;
>> +    mbuf->next = NULL;
> 
> Are we sure this is all we need? Asking as rte_pktmbuf_reset() cleans of 
> some more field, so want to make sure we hit all the corner cases. 
> Unfortunately, we can not use it here due to the sanity check.
> 

In this case it doesn't seem to me that we need to replicate what DPDK
would do. Eventually, before sending this packet out (because this is a
DPBUF_MALLOC dp_packet), the values will be copied to an mbuf properly
initialised by DPDK, and we just need to make sure we are not copying
random values as it was happening before with `ol_falgs` and
`tx_offload`. For the use of DPDK's mbuf manipulation functions, such as
rte_pktmbuf_tailroom(), these flags are enough.

On a similar note, I don't think the comment above, "The DPDK rte
library will still otherwise manage the mbuf.", is relevant anymore
since DPDK won't manage this packet at all. I'll remove it for next
iteration.

Tiago.