Re: [ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.
On 01/11/2018 19:30, Stokes, Ian wrote:
>> Hi Tiago, Ian,
>>
>> On Tue, Oct 16, 2018 at 02:28:56PM +, Stokes, Ian wrote:
From: Mark Kavanagh
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 :
https://patchwork.ozlabs.org/patch/777570/
Co-authored-by: Tiago Lam
Signed-off-by: Mark Kavanagh
Signed-off-by: Tiago Lam
Acked-by: Eelco Chaudron
Acked-by: Flavio Leitner
>>>
>>> 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
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.
> Hi Tiago, Ian,
>
> On Tue, Oct 16, 2018 at 02:28:56PM +, Stokes, Ian wrote:
> > > From: Mark Kavanagh
> > >
> > > 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 :
> > > https://patchwork.ozlabs.org/patch/777570/
> > >
> > > Co-authored-by: Tiago Lam
> > >
> > > Signed-off-by: Mark Kavanagh
> > > Signed-off-by: Tiago Lam
> > > Acked-by: Eelco Chaudron
> > > Acked-by: Flavio Leitner
> >
> > 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.
Hi Tiago, Ian,
On Tue, Oct 16, 2018 at 02:28:56PM +, Stokes, Ian wrote:
> > From: Mark Kavanagh
> >
> > 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 :
> > https://patchwork.ozlabs.org/patch/777570/
> >
> > Co-authored-by: Tiago Lam
> >
> > Signed-off-by: Mark Kavanagh
> > Signed-off-by: Tiago Lam
> > Acked-by: Eelco Chaudron
> > Acked-by: Flavio Leitner
>
> 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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.
> From: Mark Kavanagh
>
> 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 :
> https://patchwork.ozlabs.org/patch/777570/
>
> Co-authored-by: Tiago Lam
>
> Signed-off-by: Mark Kavanagh
> Signed-off-by: Tiago Lam
> Acked-by: Eelco Chaudron
> Acked-by: Flavio Leitner
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
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v11 02/14] dp-packet: Init specific mbuf fields.
From: Mark Kavanagh
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 :
https://patchwork.ozlabs.org/patch/777570/
Co-authored-by: Tiago Lam
Signed-off-by: Mark Kavanagh
Signed-off-by: Tiago Lam
Acked-by: Eelco Chaudron
Acked-by: Flavio Leitner
---
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;
+mbuf->next = NULL;
#endif
}
--
2.7.4
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
