[ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with vhu client.

Kevin Traynor ktraynor at redhat.com
Wed Oct 11 13:37:17 UTC 2017


On 10/11/2017 02:04 PM, Fischetti, Antonio wrote:
> 
>> -----Original Message-----
>> From: Kevin Traynor [mailto:ktraynor at redhat.com]
>> Sent: Wednesday, October 11, 2017 11:40 AM
>> To: Fischetti, Antonio <antonio.fischetti at intel.com>; Róbert Mulik
>> <robert.mulik at ericsson.com>; dev at openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with
>> vhu client.
>>
>> On 10/11/2017 11:28 AM, Fischetti, Antonio wrote:
>>> Hi Robert,
>>> that's happening because the requested MTU is rounded up to the current
>> boundary.
>>> So if the current upper boundary is 2500 and we request 2000 =>
>>> 2000 is rounded up to 2500 and the same mempool is returned.
>>>
>>> I may be wrong but this seems the wanted behavior, maybe Kevin can shed some
>> light?
>>> I may have missed some detail as I didn't follow this implementation since
>> the
>>> very beginning.
>>>
>>
>> I think it's related to review comments I sent earlier today. mtu is
>> mtu, but a value that is rounded from it is used in calculating the size
>> of the mbuf. I suspect in this case, when the new mtu size results in
>> the same rounded value, the current mempool is being reused (which is
>> fine) 
> 
> [Antonio] exactly.
> 
>> but the EEXISTS error value returned from reconfigure means that
>> the change is not seen as successful and the old mtu value is restored
>> to ovsdb.
> 
> [Antonio] 
> I think this can be fixed by the following changes. 
> The new mtu value is stored when a pre-existing mempool is returned:
> __________________________________________________________________________
> @@ -631,6 +631,11 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
>                   rte_strerror(rte_errno));
>          return rte_errno;
>      } else if (mp_exists) {
> +        /* If a new MTU was requested and its rounded value is the same
> +         * that is currently used, then the existing mempool was returned.
> +         * Update the new MTU value. */
> +        dev->mtu = dev->requested_mtu;
> +        dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);

What about requested_socket_id, isn't that needed also?

>          return EEXIST;
>      } else {
>          dpdk_mp_put(dev->dpdk_mp);
> __________________________________________________________________________
> 
> Instead, inside netdev_dpdk_reconfigure() I think it should be correct to have
> 
>     err = netdev_dpdk_mempool_configure(dev);
>     if (err && err != EEXIST) {
>         goto out;
>     }
> > because as in the case of a new mempool just created as in the case of
EEXIST (=17)
> we don't want to execute "goto out" and fall through to do
> 
>     dev->rxq_size = dev->requested_rxq_size;
>     dev->txq_size = dev->requested_txq_size;
>     ...
> 
> With these code changes it seems to work, at the beginning I have MTU=1500 and I set it to 1000:
> 
> # ovs-vsctl list interface dpdk0 | grep mtu
> mtu                 : 1500
> mtu_request         : []
> 
> # ovs-vsctl set interface dpdk0 mtu_request=1000
> my log says
> netdev_dpdk|DBG|Requesting a mempool of 40992 mbufs for netdev dpdk0 with 1 Rx and 11 Tx queues, socket id:0.
> netdev_dpdk|DBG|A mempool with name ovs_62a2ca2f_0_2030_40992 already exists at 0x7fad9d77dd00.
> dpdk|INFO|PMD: ixgbe_dev_link_status_print(): Port 0: Link Up - speed 0 Mbps - half-duplex
> 
> # ovs-vsctl list interface dpdk0 | grep mtu
> mtu                 : 1000
> mtu_request         : 1000
> 
> Does that make sense?
> 

Looks ok to me.

Kevin.

> Thanks,
> -Antonio
> 
>>
>> Kevin.
>>
>>> Antonio
>>>
>>>> -----Original Message-----
>>>> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
>> bounces at openvswitch.org]
>>>> On Behalf Of Fischetti, Antonio
>>>> Sent: Wednesday, October 11, 2017 9:04 AM
>>>> To: Róbert Mulik <robert.mulik at ericsson.com>; dev at openvswitch.org
>>>> Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management
>> with
>>>> vhu client.
>>>>
>>>> Thanks Robert for reporting this and for all the clear details you provided.
>>>> I'll look into this and get back to you.
>>>>
>>>> Antonio
>>>>
>>>>> -----Original Message-----
>>>>> From: Róbert Mulik [mailto:robert.mulik at ericsson.com]
>>>>> Sent: Tuesday, October 10, 2017 4:19 PM
>>>>> To: Fischetti, Antonio <antonio.fischetti at intel.com>; dev at openvswitch.org
>>>>> Subject: RE: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management
>>>> with
>>>>> vhu client.
>>>>>
>>>>> Hi Antonio,
>>>>>
>>>>> Last week I run into this mempool issue during the development of a new
>>>>> feature. I have made a bugfix, but then we saw yours too, so I tested if it
>>>>> solves my problem. It did, but I realized another problem with it. The
>>>> mempool
>>>>> name generation is partly based on the MTU size, which is handled in 1024
>>>> bytes
>>>>> long ranges. For example MTU 1000 and 1500 are in the same range, 2000 and
>>>> 2500
>>>>> are in a different range. So I tested this patch and got the following.
>>>>>
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 2500
>>>>> mtu_request         : 2500
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 2500
>>>>> mtu_request         : 2000
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 1500
>>>>> mtu_request         : 1500
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 1500
>>>>> mtu_request         : 1000
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=2000
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 2000
>>>>> mtu_request         : 2000
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=1000
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 1000
>>>>> mtu_request         : 1000
>>>>> # ovs-vsctl set interface dpdk0 mtu_request=1500
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 1000
>>>>> mtu_request         : 1500
>>>>> # service openvswitch-switch restart
>>>>> # ovs-vsctl list interface dpdk0 |grep mtu
>>>>> mtu                 : 1500
>>>>> mtu_request         : 1500
>>>>>
>>>>>
>>>>> This was my setup:
>>>>>     Bridge br-prv
>>>>>         Port bond-prv
>>>>>             Interface "dpdk0"
>>>>>                 type: dpdk
>>>>>                 options: {dpdk-devargs="0000:05:00.0", n_rxq_desc="1024",
>>>>> n_txq_desc="1024"}
>>>>>     ovs_version: "2.8.90"
>>>>>
>>>>> And I used DPDK v17.08.
>>>>>
>>>>>
>>>>> So, as it can be see from the example above, with the patch applied when a
>>>> new
>>>>> mtu_request is in the same range as the previously set MTU, then it has no
>>>>> effect until service restart. The mtu_request has immediate effect when it
>> is
>>>>> in different range as the previously set MTU. Or did I miss something
>> during
>>>>> the testing?
>>>>>
>>>>> My patch what I used last week does the following. During reconfiguration
>> the
>>>>> mempool is always deleted before a new one is created. It solved the
>> problem
>>>>> without side effects, but it is not optimized (always recreates the mempool
>>>>> when this function is called).
>>>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>>>> index c60f46f..de38f95 100644
>>>>> --- a/lib/netdev-dpdk.c
>>>>> +++ b/lib/netdev-dpdk.c
>>>>> @@ -621,6 +621,7 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
>>>>>      uint32_t buf_size = dpdk_buf_size(dev->requested_mtu);
>>>>>      struct dpdk_mp *mp;
>>>>>
>>>>> +    dpdk_mp_put(dev->dpdk_mp);
>>>>>      mp = dpdk_mp_get(dev, FRAME_LEN_TO_MTU(buf_size));
>>>>>      if (!mp) {
>>>>>          VLOG_ERR("Failed to create memory pool for netdev "
>>>>> @@ -629,7 +630,6 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
>>>>>                   rte_strerror(rte_errno));
>>>>>          return rte_errno;
>>>>>      } else {
>>>>> -        dpdk_mp_put(dev->dpdk_mp);
>>>>>          dev->dpdk_mp = mp;
>>>>>          dev->mtu = dev->requested_mtu;
>>>>>          dev->socket_id = dev->requested_socket_id;
>>>>>
>>>>>
>>>>> What do you think about this solution?
>>>>>
>>>>> Regards,
>>>>> Robert
>>>> _______________________________________________
>>>> dev mailing list
>>>> dev at openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list