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

[Stokes, Ian]

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

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

> 
>      /* 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)) {

>              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