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....@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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to