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

Ilya Maximets i.maximets at samsung.com
Thu Jul 4 14:13:40 UTC 2019


On 04.07.2019 16:59, Ian Stokes wrote:
> On 7/4/2019 12:15 PM, Ilya Maximets wrote:
>> Few comments inline.
>> Probably, could be fixed while applying the patch.
> 
> Ok, these look reasonable to me. I can apply them this evening before committing.
> 
> If I add these will I had an ACK for you Ilya?

Sure. With the changes applied:

Acked-by: Ilya Maximets <i.maximets at samsung.com>

> 
> Regards
> Ian
>>
>> On 02.07.2019 3:32, 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.
>>>
>>> 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 ++++++++
>>>   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
>>> +
>>> +.. 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
>>
>> An empty line would be nice here.
>>
>>>   #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);
>>
>> This should be atomic_init().
>>
>>> +
>>>       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);
>>
>> s/dev->up.name/netdev_get_name(netdev)/
>>
>>> +    }
>>>       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