[ovs-dev] [PATCH v4 01/15] netdev-dpdk: Differentiate between mtu/mbuf size.

[Stokes, Ian]

> 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 <tiago.lam at intel.com>
> >
> > 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.

That sounds ok, will review in the next revision.

Thanks
Ian