On 24/03/2020 23:50, Ilya Maximets wrote:
> In case number of polling threads goes from exact number of Tx queues
> in port to higher value while set_tx_multiq() not implemented or not
> requesting reconfiguration, port will not be reconfigured and datapath
> will continue using static Tx queue ids leading to crash.
> 
> Ex.:
>  Assuming that port p0 supports up to 4 Tx queues and doesn't support
>  set_tx_multiq() method.  For example, netdev-afxdp could be the case,
>  because it could have multiple Tx queues, but doesn't have
>  set_tx_multiq() implementation because number of Tx queues always
>  equals to number of Rx queues.
> 
>  1. Configuring pmd-cpu-mask to have 3 pmd threads.
> 
>  2. Adding port p0 to OVS.
>     At this point wanted_txqs = 4 (3 for pmd threads + 1 for non-pmd).
>     Port reconfigured to have 4 Tx queues successfully.
>     dynamic_txqs = (4 < 4) = false;
> 
>  3. Configuring pmd-cpu-mask to have 10 pmd threads.
>     At this point wanted_txqs = 11 (10 for pmd threads + 1 for non-pmd).
>     Since set_tx_multiq() is not implemented, netdev doesn't request
>     reconfiguration and 'dynamic_txqs' remains in 'false' state.
> 
>  4. Since 'dynamic_txqs == false', dpif-netdev uses static Tx queue
>     ids that are in range [0, 10] while device only supports 4 leading
>     to unwanted behavior and crashes.
> 
> Fix that by marking for reconfiguration all the ports that will likely
> change their 'dynamic_txqs' value.
> 
> It looks like the issue could be reproduced only with afxdp ports,
> because all other non-dpdk ports ignores Tx queue ids and dpdk ports
> requests for reconfiguration on set_tx_multiq().
> 
> Reported-by: William Tu <u9012...@gmail.com>
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-dev/2020-March/368364.html
> Fixes: e32971b8ddb4 ("dpif-netdev: Centralized threads and queues handling 
> code.")
> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
> ---
> 
> Since the issue could be reproduced only with afxdp ports, this should
> be backported only down to 2.12.
> 
>  lib/dpif-netdev.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a798db45d..e456cc9be 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -4941,9 +4941,17 @@ reconfigure_datapath(struct dp_netdev *dp)
>  
>      /* Check for all the ports that need reconfiguration.  We cache this in
>       * 'port->need_reconfigure', because netdev_is_reconf_required() can
> -     * change at any time. */
> +     * change at 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) < wanted_txqs))) {
>              port->need_reconfigure = true;
>          }
>      }
> 

LGTM
Acked-by: Kevin Traynor <ktray...@redhat.com>

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to