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 <[email protected]> > > 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 <[email protected]> >>> Acked-by: Eelco Chaudron <[email protected]> >>> Acked-by: Flavio Leitner <[email protected]> >>> --- >>> 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}'> >>> > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
