> -----Original Message----- > From: Kevin Traynor [mailto:ktray...@redhat.com] > Sent: Wednesday, October 11, 2017 2:37 PM > To: Fischetti, Antonio <antonio.fische...@intel.com>; Róbert Mulik > <robert.mu...@ericsson.com>; d...@openvswitch.org > Subject: Re: [ovs-dev] [PATCH v3 1/5] netdev-dpdk: fix mempool management with > vhu client. > > On 10/11/2017 02:04 PM, Fischetti, Antonio wrote: > > > >> -----Original Message----- > >> From: Kevin Traynor [mailto:ktray...@redhat.com] > >> Sent: Wednesday, October 11, 2017 11:40 AM > >> To: Fischetti, Antonio <antonio.fische...@intel.com>; Róbert Mulik > >> <robert.mu...@ericsson.com>; d...@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?
Yes will add it. Thanks Kevin and Robert, I'll rework it and post a v5. I'll take into account also the other comments by Kevin on v4 review. -Antonio > > > 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-boun...@openvswitch.org [mailto:ovs-dev- > >> boun...@openvswitch.org] > >>>> On Behalf Of Fischetti, Antonio > >>>> Sent: Wednesday, October 11, 2017 9:04 AM > >>>> To: Róbert Mulik <robert.mu...@ericsson.com>; d...@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.mu...@ericsson.com] > >>>>> Sent: Tuesday, October 10, 2017 4:19 PM > >>>>> To: Fischetti, Antonio <antonio.fische...@intel.com>; > >>>>> d...@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 > >>>> d...@openvswitch.org > >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev