[ovs-dev] [PATCH v2 2/3] dpif-netdev: Avoid port's reconfiguration on pmd-cpu-mask changes.

Stokes, Ian ian.stokes at intel.com
Thu Jun 15 11:37:30 UTC 2017


> >> -----Original Message-----
> >> From: Ilya Maximets [mailto:i.maximets at samsung.com]
> >> Sent: Thursday, June 15, 2017 9:45 AM
> >> To: Stokes, Ian <ian.stokes at intel.com>; dev at openvswitch.org; Darrell
> >> Ball <dball at vmware.com>
> >> Cc: Heetae Ahn <heetae82.ahn at samsung.com>; Daniele Di Proietto
> >> <diproiettod at ovn.org>; Ben Pfaff <blp at ovn.org>; Pravin Shelar
> >> <pshelar at ovn.org>; Loftus, Ciara <ciara.loftus at intel.com>; Kevin
> >> Traynor <ktraynor at redhat.com>
> >> Subject: Re: [PATCH v2 2/3] dpif-netdev: Avoid port's reconfiguration
> >> on pmd-cpu-mask changes.
> >>
> >> On 15.06.2017 11:02, Stokes, Ian wrote:
> >>>> Reconfiguration of HW NICs may lead to packet drops.
> >>>> In current model all physical ports will be reconfigured each time
> >>>> number of PMD threads changed. Since we not stopping threads on
> >>>> pmd-cpu-mask changes, this patch will help to further decrease
> >>>> port's downtime by setting the maximum possible number of wanted tx
> >>>> queues to avoid unnecessary reconfigurations.
> >>>
> >>> Hi Ilya,
> >>>
> >>> Thanks for the patch, in testing I can confirm it resolves the
> >>> issue. A
> >> few observations and comments below.
> >>
> >> Hi Ian. Thanks for testing.
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Ilya Maximets <i.maximets at samsung.com>
> >>>> ---
> >>>>  lib/dpif-netdev.c | 26 +++++++++++++++++++++-----
> >>>>  1 file changed, 21 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> >>>> 596d133..79db770
> >>>> 100644
> >>>> --- a/lib/dpif-netdev.c
> >>>> +++ b/lib/dpif-netdev.c
> >>>> @@ -3453,7 +3453,7 @@ reconfigure_datapath(struct dp_netdev *dp)  {
> >>>>      struct dp_netdev_pmd_thread *pmd;
> >>>>      struct dp_netdev_port *port;
> >>>> -    int wanted_txqs;
> >>>> +    int needed_txqs, wanted_txqs;
> >>>>
> >>>>      dp->last_reconfigure_seq = seq_read(dp->reconfigure_seq);
> >>>>
> >>>> @@ -3461,7 +3461,15 @@ reconfigure_datapath(struct dp_netdev *dp)
> >>>>       * on the system and the user configuration. */
> >>>>      reconfigure_pmd_threads(dp);
> >>>>
> >>>> -    wanted_txqs = cmap_count(&dp->poll_threads);
> >>>> +    /* We need 1 Tx queue for each thread to avoid locking, but we
> >>>> + will
> >>>> try
> >>>> +     * to allocate the maximum possible value to minimize the
> >>>> + number of
> >>>> port
> >>>> +     * reconfigurations. */
> >>>> +    needed_txqs = cmap_count(&dp->poll_threads);
> >>>> +    /* (n_cores + 1) is the maximum that we might need to have.
> >>>> +     * Additional queue is for non-PMD threads. */
> >>>> +    wanted_txqs = ovs_numa_get_n_cores();
> >>>> +    ovs_assert(wanted_txqs != OVS_CORE_UNSPEC);
> >>>> +    wanted_txqs++;
> >>>
> >>> So just after this line there is a call
> >>>
> >>>     HMAP_FOR_EACH (port, node, &dp->ports) {
> >>>         netdev_set_tx_multiq(port->netdev, wanted_txqs);
> >>>     }
> >>>
> >>> My initial concern with this patch was twofold:
> >>>
> >>> (i) What would happen if the number of cores was greater than the
> >>> number
> >> of supported queues.
> >>>
> >>> That's not an issue from what I can see as the requested_txqs will
> >>> be
> >> compared to what's supported by the device and the smaller of the two
> >> will be chosen in the eventual call to dpdk_eth_dev_init():
> >>>
> >>> n_txq = MIN(info.max_tx_queues, dev->up.n_txq);
> >>>
> >>> (ii) What happens when a device reports the incorrect number of max
> >>> tx
> >> queues? (can occur depending on the mode of the device).
> >>>
> >>> I think this case is ok as well as it's handled by the existing txq
> >> retry logic in dpdk_eth_dev_queue_setup().
> >>
> >> Actually these concerns are not about this patch. Such situations are
> >> possible on current master. But you're right, both of them handled
> >> properly with existing code according to conventions between
> >> dpif-netdev and netdev layers.
> >
> > Sure, they are not direct concerns as they could be reproduced on master
> as is, but moving to using core count can potentially cause the number of
> txqs requests to be larger than what's supported, for example with hyper
> threading a system can appear with 64 cores & will require 65 txqs queues,
> depending on the DPDK device and mode this can increase the chance of
> requesting more txqs than the device supports (instead of the current
> model of requesting the number of txqs as the number polling threads).
> >
> > Either way the code is in place to handle such a situation so I think
> it's fine.
> >>
> >>>>      /* The number of pmd threads might have changed, or a port can
> >>>> be
> >>>> new:
> >>>>       * adjust the txqs. */
> >>>> @@ -3474,9 +3482,17 @@ reconfigure_datapath(struct dp_netdev *dp)
> >>>>
> >>>>      /* Check for all the ports that need reconfiguration.  We
> >>>> cache this in
> >>>>       * 'port->reconfigure', because netdev_is_reconf_required()
> >>>> can change at
> >>>> -     * any time. */
> >>>> +     * any time.
> >>>> +     * Also mark for reconfiguration all ports which will likely
> >>>> + change
> >>>> their
> >>>> +     * 'dynamic_txqs' parameter. It's required to stop using them
> >> before
> >>>> +     * changing this setting and it's simpler to mark ports here
> >>>> + and
> >>>> allow
> >>>> +     * 'pmd_remove_stale_ports' to remove them from threads. There
> >>>> + will
> >>>> be
> >>>> +     * no actual reconfiguration in 'port_reconfigure' because it's
> >>>> +     * unnecessary.  */
> >>>>      HMAP_FOR_EACH (port, node, &dp->ports) {
> >>>> -        if (netdev_is_reconf_required(port->netdev)) {
> >>>> +        if (netdev_is_reconf_required(port->netdev)
> >>>> +            || (port->dynamic_txqs
> >>>> +                !=  netdev_n_txq(port->netdev) < needed_txqs)) {
> >>>
> >>> GCC caught the following
> >>>
> >>> lib/dpif-netdev.c:3448:48: error: suggest parentheses around
> >>> comparison
> >> in operand of '!=' [-Werror=parentheses]
> >>>                  !=  netdev_n_txq(port->netdev) < needed_txqs)) {
> >>
> >> It's unnecessary because equality operators has higher priority than
> >> relational, but I can add parentheses if compiler complains.
> >>
> >> What version of GCC you're using? I didn't see such warnings on my
> >> systems.
> >
> > gcc version 6.3.1 20161221 (Red Hat 6.3.1-1) (GCC)
> 
> Ok. I'm going to make v3 with additional parentheses and maybe enhanced
> comments in the first patch.
> 
> If you don't mind I'll add you to Tested-by tag in v3, since there will
> only be minor code change.
Sure, sounds good.

Thanks
Ian
> 
> >>
> >>>>              port->need_reconfigure = true;
> >>>>          }
> >>>>      }
> >>>> @@ -3510,7 +3526,7 @@ reconfigure_datapath(struct dp_netdev *dp)
> >>>>              seq_change(dp->port_seq);
> >>>>              port_destroy(port);
> >>>>          } else {
> >>>> -            port->dynamic_txqs = netdev_n_txq(port->netdev) <
> >>>> wanted_txqs;
> >>>> +            port->dynamic_txqs = netdev_n_txq(port->netdev) <
> >>>> + needed_txqs;
> >>>>          }
> >>>>      }
> >>>>
> >>>> --
> >>>> 2.7.4
> >
> >
> >


More information about the dev mailing list