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

[Ilya Maximets]

On 02.11.2018 19:45, Stokes, Ian wrote:
>> 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.
>>
> 
> I think it would still make sense to apply this patch. As Tiago has said, I don’t see why we should handle clearing fields in a separate part for the code for the policer use case if they can be initialized here along with the fields that are already initialized.

In this case I do not understand why only these few fields initialized?
Why not other fields like 'vlan_tci' or 'packet_type' ? They are not used
in OVS and we're passing mbufs with uninitialized fields to the rte_meter
library.
And yes, there are fields that we just can not initialize, like 'pool'.

>>>
>>> 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.
> 
> I'd like to start applying some of patches in the multiseg series so that we can reduce the overall size of the series and so that the multiseg work (and the follow up TSO work) would have some soakage before the 2.11 release.
> 
> I understand your preference for reworking the mbuf related code but it would push the multiseg work out again, and may have fallout in terms of the mseg series being re-reviewed.
> 
> What I propose here is that the mbuf related rework you suggest be rebased after the multi seg series is applied.

IMHO, multiseg work is not ready still. dp-packet APIs still returns NULL pointers
and callers just ignores that fact, we're still passing mbufs with the
number of segments that could be not supported by the driver.
Profit of multisegs without TSO is very arguable, and TSO patch set contains
more questions than answers. At the same time code becomes highly unreadable
and hard to maintain.
This is not that I can call an improvement.

> 
> Thanks
> Ian
> 
>>>
>>>>
>>>> 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
>>>>