| Message ID | 1541149594-257478-3-git-send-email-tiago.lam@intel.com |
|---|---|
| State | Accepted |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | Misc fixes in dp_packet and netdev-dpdk. | expand |
On 02.11.2018 12:06, Tiago Lam 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 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(). 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. > > 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 | 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 >
On 02/11/2018 10:35, Ilya Maximets wrote: > On 02.11.2018 12:06, Tiago Lam 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 > > 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. > > 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. > >> >> 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 | 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 >>
> On 02/11/2018 10:35, Ilya Maximets wrote: > > On 02.11.2018 12:06, Tiago Lam 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 > > > > 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. > > > > 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. Thanks Ian > > > >> > >> 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 | 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 > >>
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@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@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 | 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 >>>>
On 02/11/2018 17:39, Ilya Maximets wrote: > 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@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'. 'packet_type' is being set to 0 already. These were the fields we identified as "essential" while testing the multi-segments patch series. To me, these make sense overall. Other fields may be used in some corner cases which we might find later on when integrating with more use cases. But not at the macro level or simple calculations that are performed on DPBUF_DPDK packets at the moment (like rte_pktmbuf_pkt_len()). Tiago. > >>>> >>>> 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@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 | 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 >>>>>
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