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

Eelco Chaudron echaudro at redhat.com
Tue Jun 26 08:51:59 UTC 2018



On 22 Jun 2018, at 21:03, Lam, Tiago wrote:

> 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.

Thanks for the clarification. Guess it would also mess up the fields 
just setup by dp_packet_use__() which I missed during review.

> 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.


More information about the dev mailing list