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 <[email protected]> >>>> >>>> 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 <[email protected]>: >>>> https://patchwork.ozlabs.org/patch/777570/ >>>> >>>> Co-authored-by: Tiago Lam <[email protected]> >>>> >>>> Signed-off-by: Mark Kavanagh <[email protected]> >>>> Signed-off-by: Tiago Lam <[email protected]> >>>> Acked-by: Eelco Chaudron <[email protected]> >>>> Acked-by: Flavio Leitner <[email protected]> >>> >>> 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
