On 11/07/2018 12:40, Ian Stokes wrote: > On 7/10/2018 12:06 PM, Tiago Lam wrote: >> When configuring a mempool, in netdev_dpdk_mempool_configure(), the >> result of a call to dpdk_buf_size() is being used as the MTU. This, >> however, is not the MTU but a ROUND_UP aligned number based on the MTU, >> which could lead to the reuse of mempools even when the real MTUs >> between different ports differ but somehow round up to the same mbuf >> size. >> >> To support multi-segment mbufs, which is coming in later commits, this >> distinction is important. For multi-segment mbufs the mbuf size is >> always the same (multiple mbufs are chained together to hold the >> packet's data), which means that using the mbuf size as the mtu would >> lead to reusing always the same mempool, independent of the MTU set. >> >> To fix this, two fields are now passed to dpdk_mp_create(), the mtu and >> the mbuf_size, thus creating a distinction between the two. The latter >> is used for telling rte_pktmbuf_pool_create() the size of each mbuf, >> while the former is used for tracking purposes in struct dpdk_mp and as >> an input to create a unique hash for the mempool's name. >> >> Signed-off-by: Tiago Lam <[email protected]> > > Hi Tiago, > > I don't think these changes are acceptable as they break from the > expected behavior of the shared mempool model. > > The reason we introduced the shared mempool model was that it was used > from OVS 2.6 -> 2.9. > > A user with the same deployment moving between releases would not have > to re-calculate the memory they dimension for OVS DPDK. > > With these changes however they may have to. > > Consider the following example: > > A user adds a port with mtu 1500 and a second port with mtu 1800. > Assuming both ports are on the same socket, the same mempool would be > re-used for both ports in the current shared mempool model.
> However with this change, separate mempools would have to be created as > the MTU is different, potentially requiring an increase in the memory > provisioned by the user. It may not be possible for users to upgrade > from OVS 2.9 to 2.10 without re-dimension their memory with this. > Hi Ian, Thanks for the context here. It's a valid point. I did consider this, but wasn't aware that users / deployments were also taking this round up into account. In that case, this could certainly cause problems during upgrade. > We had a similar issue to this in OVS 2.9 release with the per port > memory model, it was flagged as a blocking factor for Red Hat and > Ericsson upgrades which led to the removal of the per port model in 2.9. > > One possible solution here is that multiseg is supported for the per > port memory model only? > It seems we can adapt this in a different way. I was trying to reuse the "mbuf_size" for both cases; In the per-port and shared mempool case, where it is being used as the MTU, and in the multi-segment case where it is being used for telling rte_pktmbuf_pool_create() the size of each mbuf. Hence why I thought it would be better to differentiate between the real MTU and the "mbuf_size". Instead, we can calculate two "mbuf_sizes" - the one the shared and per-port mempool models use for tracking the MTU changes, and in case multi-segment mbufs are in use, the "mbuf_size" for when creating the mempool. This should suffice. I'll update the series with this; This patch 01/15 will probably be dropped entirely, and patch 12/15 will likely be the one taking the changes mentioned above. I'll also update patches 05/15 and 11/15 to fix the Travis CI. Thanks, Tiago. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
