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

Message ID 1539188552-129083-3-git-send-email-tiago.lam@intel.com
State Changes Requested
Delegated to: Ian Stokes
Headers show
Series
  • Support multi-segment mbufs
Related show

Commit Message

Lam, Tiago Oct. 10, 2018, 4:22 p.m.
From: Mark Kavanagh <mark.b.kavanagh@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@chinac.com>:
https://patchwork.ozlabs.org/patch/777570/

Co-authored-by: Tiago Lam <tiago.lam@intel.com>

Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
Signed-off-by: Tiago Lam <tiago.lam@intel.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/dp-packet.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Comments

Ian Stokes Oct. 16, 2018, 2:28 p.m. | #1
> From: Mark Kavanagh <mark.b.kavanagh@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@chinac.com>:
> https://patchwork.ozlabs.org/patch/777570/
> 
> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
> 
> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Acked-by: Flavio Leitner <fbl@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.

Thanks
Ian

> +    mbuf->next = NULL;
>  #endif
>  }
> 
> --
> 2.7.4
Flavio Leitner Nov. 1, 2018, 7:19 p.m. | #2
Hi Tiago, Ian,

On Tue, Oct 16, 2018 at 02:28:56PM +0000, Stokes, Ian wrote:
> > From: Mark Kavanagh <mark.b.kavanagh@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@chinac.com>:
> > https://patchwork.ozlabs.org/patch/777570/
> > 
> > Co-authored-by: Tiago Lam <tiago.lam@intel.com>
> > 
> > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> > Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > Acked-by: Flavio Leitner <fbl@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?

Thanks,
fbl


> 
> Thanks
> Ian
> 
> > +    mbuf->next = NULL;
> >  #endif
> >  }
> > 
> > --
> > 2.7.4
Ian Stokes Nov. 1, 2018, 7:30 p.m. | #3
> Hi Tiago, Ian,
> 
> On Tue, Oct 16, 2018 at 02:28:56PM +0000, Stokes, Ian wrote:
> > > From: Mark Kavanagh <mark.b.kavanagh@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@chinac.com>:
> > > https://patchwork.ozlabs.org/patch/777570/
> > >
> > > Co-authored-by: Tiago Lam <tiago.lam@intel.com>
> > >
> > > Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
> > > Signed-off-by: Tiago Lam <tiago.lam@intel.com>
> > > Acked-by: Eelco Chaudron <echaudro@redhat.com>
> > > Acked-by: Flavio Leitner <fbl@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.

Thanks
Ian
> Thanks,
> fbl
> 
> 
> >
> > Thanks
> > Ian
> >
> > > +    mbuf->next = NULL;
> > >  #endif
> > >  }
> > >
> > > --
> > > 2.7.4
Lam, Tiago Nov. 2, 2018, 9:10 a.m. | #4
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@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@chinac.com>:
>>>> https://patchwork.ozlabs.org/patch/777570/
>>>>
>>>> Co-authored-by: Tiago Lam <tiago.lam@intel.com>
>>>>
>>>> Signed-off-by: Mark Kavanagh <mark.b.kavanagh@intel.com>
>>>> Signed-off-by: Tiago Lam <tiago.lam@intel.com>
>>>> Acked-by: Eelco Chaudron <echaudro@redhat.com>
>>>> Acked-by: Flavio Leitner <fbl@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
>

Patch

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;
+    mbuf->next = NULL;
 #endif
 }