On 25/06/2019 09:31, Eelco Chaudron wrote:
>
>
> On 21 Jun 2019, at 15:41, 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 <ktray...@redhat.com>
>> ---
>> Documentation/topics/dpdk/vhost-user.rst | 26
>> ++++++++++++++++++++++++
>> lib/netdev-dpdk.c | 26
>> ++++++++++++++++++++----
>> vswitchd/vswitch.xml | 10 +++++++++
>> 3 files changed, 58 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/topics/dpdk/vhost-user.rst
>> b/Documentation/topics/dpdk/vhost-user.rst
>> index e79ed082e..0b14bc999 100644
>> --- a/Documentation/topics/dpdk/vhost-user.rst
>> +++ b/Documentation/topics/dpdk/vhost-user.rst
>> @@ -387,4 +387,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
>> +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 03a8de380..8ae1093d8 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -409,5 +409,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 +1241,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 +1902,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 +1919,13 @@ netdev_dpdk_vhost_client_set_config(struct
>> netdev *netdev,
>> }
>> }
>> +
>> + max_tx_retries = smap_get_int(args, "vhost-tx-retries",
>> + VHOST_ENQ_RETRY_NUM);
>
> There is no upper bound check for the 31 value you mention in the
> documentation, maybe you could at it here?
>
> #define VHOST_MAX_ENQ_RETRY_NUM 31
> max_tx_retries = MIN(max_tx_retries, VHOST_MAX_ENQ_RETRY_NUM);
>
I didn't do that because there is an effective limit of 31 from the
batch size. There would never be more 31 retries even if the max was set
to be higher, as after 31 retries all the packets would either be sent,
or we'd break out of the loop.
> or log an error if larger…
>
You have a point about the logging - I think I will go with your
suggestion to explicitly limit here and update the logging.
>> + 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);
>>
>> @@ -2299,4 +2312,5 @@ netdev_dpdk_vhost_update_tx_counters(struct
>> netdev_stats *stats,
>> int attempted,
>> int dropped,
>> + int max_retries,
>> int retries)
>> {
>> @@ -2306,5 +2320,5 @@ netdev_dpdk_vhost_update_tx_counters(struct
>> netdev_stats *stats,
>> stats->tx_packets += sent;
>> stats->tx_dropped += dropped;
>> - stats->tx_retries += MIN(retries, VHOST_ENQ_RETRY_NUM);
>> + stats->tx_retries += MIN(retries, max_retries);
>
> We should just store the number of retries, i.e. retries will always be
> smaller or equal to max_retries.
I need to account for max being zero. In that case if some but not all
packets were sent, retries=1, and max=0. Will add a comment at least.
> This would also allow you to drop the max_retries parameter from the
> netdev_dpdk_vhost_update_tx_counters() function.
>
Hmm, I could drop it regardless and put the MIN() as the argument for
retries.
thanks for your comments,
Kevin
>>
>> for (i = 0; i < sent; i++) {
>> @@ -2323,4 +2337,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;
>>
>> qid = dev->tx_q[qid % netdev->n_txq].map;
>> @@ -2351,9 +2366,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);
>> + }
>> } 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 +2379,5 @@ __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);
>> rte_spinlock_unlock(&dev->stats_lock);
>>
>> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
>> index bf4b6f8dc..7ffbdd77a 100644
>> --- a/vswitchd/vswitch.xml
>> +++ b/vswitchd/vswitch.xml
>> @@ -3127,4 +3127,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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
