[ovs-dev] [PATCH v4 3/3] netdev-dpdk: Enable vhost-tx-retries config.

Ian Stokes ian.stokes at intel.com
Wed Jul 3 16:07:39 UTC 2019


On 7/2/2019 1:32 AM, Kevin Traynor wrote:
> vhost tx retries can provide some mitigation against
> dropped packets due to a temporarily slow guest/limited queue
> size for an interface, but on the other hand when a system
> is fully loaded those extra cycles retrying could mean
> packets are dropped elsewhere.
>  > Up to now max vhost tx retries have been hardcoded, which meant
> no tuning and no way to disable for debugging to see if extra
> cycles spent retrying resulted in rx drops on some other
> interface.
> 
> Add an option to change the max retries, with a value of
> 0 effectively disabling vhost tx retries.
> 

Thanks for the patch Kevin. Tests ok for me. I realize your on leave but 
there's a few minor comments below. If there are no objection by those 
who have either acked or are reviewing I can add them on commit.

> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
> Acked-by: Eelco Chaudron <echaudro at redhat.com>
> Acked-by: Flavio Leitner <fbl at sysclose.org>
> ---
>   Documentation/topics/dpdk/vhost-user.rst | 28 +++++++++++++++++
>   lib/netdev-dpdk.c                        | 39 +++++++++++++++++++++---
>   vswitchd/vswitch.xml                     | 12 ++++++++

Probably a minor entry in NEWS for this additional feature needed also. 
As Kevin is on leave this can be added on commit if there is no objection.

>   3 files changed, 75 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst
> index 368f44520..724aa62f6 100644
> --- a/Documentation/topics/dpdk/vhost-user.rst
> +++ b/Documentation/topics/dpdk/vhost-user.rst
> @@ -351,4 +351,29 @@ The default value is ``false``.
>   .. _dpdk-testpmd:
>   
> +vhost-user-client tx retries config
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +For vhost-user-client interfaces, the max amount of retries can be changed from
> +the default 8 by setting ``tx-retries-max``.
> +
> +The minimum is 0 which means there will be no retries and if any packets in
> +each batch cannot be sent immediately they will be dropped. The maximum is 32,
> +which would mean that after the first packet(s) in the batch was sent there
> +could be a maximum of 32 more retries.
> +
> +Retries can help with avoiding packet loss when temporarily unable to send to a
> +vhost interface because the virtqueue is full. However, spending more time
> +retrying to send to one interface, will reduce the time available for rx/tx and
> +processing packets on other interfaces, so some tuning may be required for best
> +performance.
> +
> +Tx retries max can be set for vhost-user-client ports::
> +
> +    $ ovs-vsctl set Interface vhost-client-1 options:tx-retries-max=0

Minor, the commit message enables vhost-tx-retries, but the config 
option is tx-retries-max. I think vhost-tx-retries comes from the 
previous revision of the patch. This can be changed on commit if there 
is no objection.

Regards
Ian

> +
> +.. note::
> +
> +  Configurable vhost tx retries are not supported with vhost-user ports.
> +
>   DPDK in the Guest
>   -----------------
> @@ -496,4 +521,7 @@ packets can be sent, it may mean the guest is not accepting packets, so there
>   are no (more) retries.
>   
> +For information about configuring the maximum amount of tx retries for
> +vhost-user-client interfaces see `vhost-user-client tx retries config`_.
> +
>   .. note::
>   
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index d3e02d389..b8592962f 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -165,5 +165,10 @@ typedef uint16_t dpdk_port_t;
>   #define DPDK_PORT_ID_FMT "%"PRIu16
>   
> -#define VHOST_ENQ_RETRY_NUM 8
> +/* Minimum amount of vhost tx retries, effectively a disable. */
> +#define VHOST_ENQ_RETRY_MIN 0
> +/* Maximum amount of vhost tx retries. */
> +#define VHOST_ENQ_RETRY_MAX 32
> +/* Legacy default value for vhost tx retries. */
> +#define VHOST_ENQ_RETRY_DEF 8
>   #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>   
> @@ -418,5 +423,7 @@ struct netdev_dpdk {
>           /* True if vHost device is 'up' and has been reconfigured at least once */
>           bool vhost_reconfigured;
> -        /* 3 pad bytes here. */
> +
> +        atomic_uint8_t vhost_tx_retries_max;
> +        /* 2 pad bytes here. */
>       );
>   
> @@ -1262,4 +1269,6 @@ vhost_common_construct(struct netdev *netdev)
>       }
>   
> +    atomic_store_relaxed(&dev->vhost_tx_retries_max, VHOST_ENQ_RETRY_DEF);
> +
>       return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>                               DPDK_DEV_VHOST, socket_id);
> @@ -1922,4 +1931,5 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
>       struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>       const char *path;
> +    int max_tx_retries, cur_max_tx_retries;
>   
>       ovs_mutex_lock(&dev->mutex);
> @@ -1938,4 +1948,17 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
>           }
>       }
> +
> +    max_tx_retries = smap_get_int(args, "tx-retries-max",
> +                                  VHOST_ENQ_RETRY_DEF);
> +    if (max_tx_retries < VHOST_ENQ_RETRY_MIN
> +        || max_tx_retries > VHOST_ENQ_RETRY_MAX) {
> +        max_tx_retries = VHOST_ENQ_RETRY_DEF;
> +    }
> +    atomic_read_relaxed(&dev->vhost_tx_retries_max, &cur_max_tx_retries);
> +    if (max_tx_retries != cur_max_tx_retries) {
> +        atomic_store_relaxed(&dev->vhost_tx_retries_max, max_tx_retries);
> +        VLOG_INFO("Max Tx retries for vhost device '%s' set to %d",
> +                  dev->up.name, max_tx_retries);
> +    }
>       ovs_mutex_unlock(&dev->mutex);
>   
> @@ -2351,4 +2374,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>       unsigned int dropped = 0;
>       int i, retries = 0;
> +    int max_retries = VHOST_ENQ_RETRY_MIN;
>       int vid = netdev_dpdk_get_vid(dev);
>   
> @@ -2380,9 +2404,16 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>               /* Prepare for possible retry.*/
>               cur_pkts = &cur_pkts[tx_pkts];
> +            if (OVS_UNLIKELY(cnt && !retries)) {
> +                /*
> +                 * Read max retries as there are packets not sent
> +                 * and no retries have already occurred.
> +                 */
> +                atomic_read_relaxed(&dev->vhost_tx_retries_max, &max_retries);
> +            }
>           } else {
>               /* No packets sent - do not retry.*/
>               break;
>           }
> -    } while (cnt && (retries++ < VHOST_ENQ_RETRY_NUM));
> +    } while (cnt && (retries++ < max_retries));
>   
>       rte_spinlock_unlock(&dev->tx_q[qid].tx_lock);
> @@ -2391,5 +2422,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>       netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts,
>                                            cnt + dropped);
> -    dev->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM);
> +    dev->tx_retries += MIN(retries, max_retries);
>       rte_spinlock_unlock(&dev->stats_lock);
>   
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 69fce7ffb..5b4c13b64 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -3120,4 +3120,16 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>         </column>
>   
> +      <column name="options" key="tx-retries-max"
> +              type='{"type": "integer", "minInteger": 0, "maxInteger": 32}'>
> +        <p>
> +          The value specifies the maximum amount of vhost tx retries that can
> +          be made while trying to send a batch of packets to an interface.
> +          Only supported by dpdkvhostuserclient interfaces.
> +        </p>
> +        <p>
> +          Default value is 8.
> +        </p>
> +      </column>
> +
>         <column name="options" key="n_rxq_desc"
>                 type='{"type": "integer", "minInteger": 1, "maxInteger": 4096}'>
> 



More information about the dev mailing list