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

[Lam, Tiago]

On 01/11/2018 19:30, Stokes, Ian wrote:
>> Hi Tiago, Ian,
>>
>> On Tue, Oct 16, 2018 at 02:28:56PM +0000, Stokes, Ian 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>
>>>> Acked-by: Eelco Chaudron <echaudro at redhat.com>
>>>> Acked-by: Flavio Leitner <fbl at sysclose.org>
>>>
>>> Thanks Tiago,
>>>
>>> This will need a rebase. One minor comment below.
>>>> ---
>>>>  lib/dp-packet.h | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h index
>>>> ba91e58..b948fe1
>>>> 100644
>>>> --- a/lib/dp-packet.h
>>>> +++ b/lib/dp-packet.h
>>>> @@ -625,14 +625,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. */
>>>> + * from DPDK interfaces, when vswitchd is built with --with-dpdk.
>>>> + */
>>>>  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;
>>>
>>> Is it just for ease of access that you cast a pointer to mbuf?
>>>
>>> Just looking at the other rte specific functions implemented in the
>>> file, they access mbuf via p->mbuf rather than the cast. I would like
>>> to keep implementation uniform across functions if possible.
>>
>> This seems to be needed but not related to the multi seg, so I wonder if
>> this patch could be posted as a standalone fix.  The goal is to apply it
>> sooner and reduce the patchset size.
>>
>> What do you think?
>>
> 
> +1, I think patches 1-3 could be applied independent of the series itself.
> 
> There's been a few comments on the latest revision, Tiago if you get a chance to send another revision of these separate to the series we could look to merge as part of the pull request this week.
> 

Sounds good. Just sent a new series with the first three patches only -
it addresses the latest comments in v11 of the multi-segment mbuf
series. I've also kept the ACK's, let me know if that's not OK.

Thanks,
Tiago.

> Thanks
> Ian
>> Thanks,
>> fbl
>>
>>
>>>
>>> Thanks
>>> Ian
>>>
>>>> +    mbuf->next = NULL;
>>>>  #endif
>>>>  }
>>>>
>>>> --
>>>> 2.7.4
>