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

Fischetti, Antonio antonio.fischetti at intel.com
Wed Oct 11 14:21:47 UTC 2017



> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor at redhat.com]
> Sent: Wednesday, October 11, 2017 2:37 PM
> 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 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?

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-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