[ovs-dev] [PATCH v2 4/4] netdev-dpdk: Enable vhost-tx-retries config.
Kevin Traynor
ktraynor at redhat.com
Wed Jun 26 15:10:06 UTC 2019
On 26/06/2019 15:51, Flavio Leitner wrote:
> On Tue, Jun 25, 2019 at 03:57:24PM +0100, 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 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.
>>
>> Signed-off-by: Kevin Traynor <ktraynor at redhat.com>
>> ---
>> Documentation/topics/dpdk/vhost-user.rst | 26 ++++++++++++++++++++++
>> lib/netdev-dpdk.c | 28 +++++++++++++++++++++---
>> vswitchd/vswitch.xml | 10 +++++++++
>> 3 files changed, 61 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/vhost-user.rst b/Documentation/topics/dpdk/vhost-user.rst
>> index a25a64237..efee19b62 100644
>> --- a/Documentation/topics/dpdk/vhost-user.rst
>> +++ b/Documentation/topics/dpdk/vhost-user.rst
>> @@ -392,4 +392,30 @@ The default value is ``false``.
>> .. _dpdk-testpmd:
>>
>> +vhost-user-client tx retries
>> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> +
>> +For vhost-user-client interfaces, the max amount of retries can be changed from
>> +the default 8 by setting ``vhost-tx-retries``.
>> +
>> +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 31,
>> +which would mean that after the first packet(s) in the batch was sent there
>> +could be up to a separate retry for each of the remaining packets, until they
>> +are all sent or no packet was sent during a retry.
>> +
>> +Retries can help with avoiding packet loss when temporarily unable to a vhost
>
> ... unable to <send> to a vhost interface?
>
ack
>
>> +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 can be set for vhost-user-client ports::
>> +
>> + $ ovs-vsctl set Interface vhost-client-1 options:vhost-tx-retries=0
>> +
>> +.. note::
>> +
>> + Configurable vhost tx retries are not supported with vhost-user ports.
>> +
>> DPDK in the Guest
>> -----------------
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 65161deaf..97a90d4a5 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -160,4 +160,5 @@ typedef uint16_t dpdk_port_t;
>>
>> #define VHOST_ENQ_RETRY_NUM 8
>> +#define VHOST_MAX_ENQ_RETRY_NUM 31
>> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>
> Maybe:
>
> /* One retry for each packet in the batch */
> #define VHOST_ENQ_RETRY_MAX ( NETDEV_MAX_BURST - 1 )
> /* Minimum value to not retry */
> #define VHOST_ENQ_RETRY_MIN 0
> /* Arbitrary, debatable, though reasonable legacy value */
> #define VHOST_ENQ_RETRY_DEF 8
>
> and fix accordingly below...
>
Sounds good.
>
>>
>> @@ -409,5 +410,6 @@ 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;
>> + /* 2 pad bytes here. */
>> );
>>
>> @@ -1240,4 +1242,6 @@ vhost_common_construct(struct netdev *netdev)
>> }
>>
>> + atomic_store_relaxed(&dev->vhost_tx_retries, VHOST_ENQ_RETRY_NUM);
>> +
>> return common_construct(netdev, DPDK_ETH_PORT_ID_INVALID,
>> DPDK_DEV_VHOST, socket_id);
>> @@ -1899,4 +1903,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);
>> @@ -1915,4 +1920,16 @@ netdev_dpdk_vhost_client_set_config(struct netdev *netdev,
>> }
>> }
>> +
>> + max_tx_retries = smap_get_int(args, "vhost-tx-retries",
>> + VHOST_ENQ_RETRY_NUM);
>> + if (max_tx_retries < 0 || max_tx_retries > VHOST_MAX_ENQ_RETRY_NUM) {
>> + max_tx_retries = VHOST_ENQ_RETRY_NUM;
>> + }
>> + atomic_read_relaxed(&dev->vhost_tx_retries, &cur_max_tx_retries);
>> + if (max_tx_retries != cur_max_tx_retries) {
>> + atomic_store_relaxed(&dev->vhost_tx_retries, 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);
>>
>> @@ -2323,4 +2340,5 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>> int i, retries = 0;
>> int vid = netdev_dpdk_get_vid(dev);
>> + int max_retries = 0;
>
> Perhaps declare retries and max_retries close to each other?
>
sure
>>
>> qid = dev->tx_q[qid % netdev->n_txq].map;
>> @@ -2351,9 +2369,12 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>> /* Prepare for possible retry.*/
>> cur_pkts = &cur_pkts[tx_pkts];
>> + if (OVS_UNLIKELY(cnt && !retries)) {
>> + atomic_read_relaxed(&dev->vhost_tx_retries, &max_retries);
>> + }
>
> So, for vhost user the value is fixed to the default while vhost
> user client we can tune. Here we only look at the value if there
> are packets left to retry and it is the first time, ok.
>
I'll add a little comment here as well.
thanks!
>
>> } 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);
>> @@ -2361,5 +2382,6 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, int qid,
>> rte_spinlock_lock(&dev->stats_lock);
>> netdev_dpdk_vhost_update_tx_counters(&dev->stats, pkts, total_pkts,
>> - cnt + dropped, retries);
>> + cnt + dropped,
>> + max_retries ? retries : 0);
>
> Because retries is incremented even if it hasn't retried, ok.
>
>> rte_spinlock_unlock(&dev->stats_lock);
>>
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index 69fce7ffb..2f47a2ceb 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -3120,4 +3120,14 @@ ovs-vsctl add-port br0 p0 -- set Interface p0 type=patch options:peer=p1 \
>> </column>
>>
>> + <column name="options" key="vhost-tx-retries"
>> + type='{"type": "integer", "minInteger": 0, "maxInteger": 31}'>
>> + <p>
>> + The value specifies the maximum amount of tx vhost retries that can
>> + be made while trying to send a batch of packets to a dpdkvhostclient
>> + interface. The max batch size is 32 packets.
>> + Only supported by dpdkvhostuserclient interfaces.
>> + </p>
>> + </column>
>> +
>> <column name="options" key="n_rxq_desc"
>> type='{"type": "integer", "minInteger": 1, "maxInteger": 4096}'>
>> --
>> 2.20.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
More information about the dev
mailing list