On 26/06/2018 14:23, Eelco Chaudron wrote: > > > On 22 Jun 2018, at 21:02, Lam, Tiago wrote: > >> Hi Eelco, >> >> On 18/06/2018 12:18, Eelco Chaudron wrote: >>> >>> >>> On 11 Jun 2018, at 18:21, Tiago Lam wrote: >>> >> >> [snip] >> >>>> Performance notes >>>> ================= >>>> In order to test for regressions in performance, tests were run on >>>> top >>>> of master 88125d6 and v8 of this patchset, both with the >>>> multi-segment >>>> mbufs option enabled and disabled. >>>> >>>> VSperf was used to run the phy2phy_cont and pvp_cont tests with >>>> varying >>>> packet sizes of 64B, 1500B and 7000B, on a 10Gbps interface. >>>> >>>> Test | Size | Master | Multi-seg disabled | Multi-seg enabled >>>> ------------------------------------------------------------- >>>> p2p | 64 | ~22.7 | ~22.65 | ~18.3 >>>> p2p | 1500 | ~1.6 | ~1.6 | ~1.6 >>>> p2p | 7000 | ~0.36 | ~0.36 | ~0.36 >>>> pvp | 64 | ~6.7 | ~6.7 | ~6.3 >>>> pvp | 1500 | ~1.6 | ~1.6 | ~1.6 >>>> pvp | 7000 | ~0.36 | ~0.36 | ~0.36 >>>> >>>> Packet size is in bytes, while all packet rates are reported in mpps >>>> (aggregated). >>>> >>>> No noticeable regression has been observed (certainly everything is >>>> within the ± 5% margin of existing performance), aside from the 64B >>>> packet size case when multi-segment mbuf is enabled. This is >>>> expected, however, because of how Tx vectoriszed functions are >>>> incompatible with multi-segment mbufs on some PMDs. The PMD under >>>> use during these tests was the i40e (on a Intel X710 NIC), which >>>> indeed doesn't support vectorized Tx functions with multi-segment >>>> mbufs. >>> >>> I did some testing using my PVP framework, >>> https://github.com/chaudron/ovs_perf, and I see the same as above. I >>> also used an XL710 for these tests. >> >> Thanks for the review! And for confirming the results, that gives some >> assurance. >> >>> >>> I reviewed the complete patch-set and will reply on the individual >>> patches. >>> >>> //Eelco >>> >>> >> I thought it would be worth giving a more generic explanation here >> since >> some of the comments are around the same point. >> >> This series takes the approach of not allocating new mbufs in >> `DPBUF_DPDK` dp_packets. This is to avoid the case where a >> dp_packet_put() call, for example, allocates two mbufs to create >> enough >> space, and returns a pointer to the data which isn't contiguous in >> memory (because it is spread across two mbufs). Most of the code in >> OvS >> that uses the dp_packet API relies on the assumption that memory >> returned is contiguous in memory. >> >> To enforce this, dp_packet_resize__() doesn't allocate any new mbufs, >> and only tries to make enough room from the space it already has >> available (both in the tailroom and headroom). dp_packet_shift() also >> doesn't allow the possibility for the head to move past the first mbuf >> (when shifting right), or the tail to move past the last mbuf (if >> shifting left). >> >> This is also why some assumptions can be made in the implementation, >> such that the tailroom of a dp_packet will be the tailroom available >> in >> the last mbuf, since it shouldn't be possible to have a whole free >> mbuf >> after the tail. > > Thanks for explaining the details, maybe its good to explain some of > these details in dp_packet_resize__(). > > However, for the general function, I think you should not assume that > tailroom is only available in the last mbuf. I think you should stick as > much as possible to the existing mbuf API’s to avoid problems in the > future if people decide to do add “full” mbuf support. Most of these > shortcuts do not give any real performance advantages. >>> This is opposed to a `DPBUF_MALLOC` packet, which when `DPDK_NETDEV` >> is >> defined, an mbuf's allocated memory comes from the system. In many >> cases >> though, this is the type of packet used internally by OvS when doing >> manipulations, since any clone of the received `DPBUF_DPDK` dp_packet >> will be allocated in a `DPBUF_MALLOC` (done by dpdk_do_tx_copy()). >> >> I'll reply to the other emails individually and refer to this >> explanation when useful. > > Thanks, I’ve replied to all the comments in the individual mails. > > I’ll allocate some time to review your new revision once it’s > available. >
Thanks a bunch for the reviews, Eelco. This is much appreciated. I should be able to send the next iteration soon. To avoid the extra noise I've replied only to the comments which were more relevant in the series, and I agree and will work on the rest. Tiago. > Cheers, > > Eelco > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
