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

[Stokes, Ian]

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