[ovs-dev,7/8] netdev-dpdk: Configurable retries while enqueuing to vHost User ports.

This commit adds "vhost-enque-retry" where in the number of retries
performed while enqueuing packets to vHostUser ports can be configured
in ovsdb.

Currently number of retries are set to '8' and a retry is performed
when atleast some packets have been successfully sent on previous attempt.
While this approach works well, it causes throughput drop when multiple
vHost User ports are servied by same PMD thread.

This commit by default disables retry mechanism and if retry logic needed
the number of retries can be set in ovsdb. For example if a maximum of
3 retries has to be performed with atleast some pkts successfully
enqueued in previous attempt, set below:

  $ ovs-vsctl set Open_vSwitch . other_config:vhost-enque-retry=3

CC: Kevin Traynor <ktraynor@redhat.com>
Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/dpdk.c           | 10 ++++++++++
 lib/dpdk.h           |  1 +
 lib/netdev-dpdk.c    |  4 ++--
 vswitchd/vswitch.xml | 12 ++++++++++++
 4 files changed, 25 insertions(+), 2 deletions(-)

On 06/07/2017 10:21 AM, Bhanuprakash Bodireddy wrote:
> This commit adds "vhost-enque-retry" where in the number of retries
> performed while enqueuing packets to vHostUser ports can be configured
> in ovsdb.
> 
> Currently number of retries are set to '8' and a retry is performed
> when atleast some packets have been successfully sent on previous attempt.
> While this approach works well, it causes throughput drop when multiple
> vHost User ports are servied by same PMD thread.

Hi Bhanu,

You are saying the approach works well but you are changing the default
behaviour. It would be good to explain a bit more about the negative
effects of changing the default and compare that against the positive
effects, so everyone gets a balanced view. If you have measurements that
would be even better.

Kevin.

> 
> This commit by default disables retry mechanism and if retry logic needed
> the number of retries can be set in ovsdb. For example if a maximum of
> 3 retries has to be performed with atleast some pkts successfully
> enqueued in previous attempt, set below:
> 
>   $ ovs-vsctl set Open_vSwitch . other_config:vhost-enque-retry=3
> 
> CC: Kevin Traynor <ktraynor@redhat.com>
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
> Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
> Co-authored-by: Antonio Fischetti <antonio.fischetti@intel.com>
> ---
>  lib/dpdk.c           | 10 ++++++++++
>  lib/dpdk.h           |  1 +
>  lib/netdev-dpdk.c    |  4 ++--
>  vswitchd/vswitch.xml | 12 ++++++++++++
>  4 files changed, 25 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/dpdk.c b/lib/dpdk.c
> index 8da6c32..77c8274 100644
> --- a/lib/dpdk.c
> +++ b/lib/dpdk.c
> @@ -41,6 +41,7 @@ VLOG_DEFINE_THIS_MODULE(dpdk);
>  static FILE *log_stream = NULL;       /* Stream for DPDK log redirection */
>  
>  static char *vhost_sock_dir = NULL;   /* Location of vhost-user sockets */
> +static int vhost_enq_retries_num = 0;
>  
>  static int
>  process_vhost_flags(char *flag, const char *default_val, int size,
> @@ -345,6 +346,9 @@ dpdk_init__(const struct smap *ovs_other_config)
>          vhost_sock_dir = sock_dir_subcomponent;
>      }
>  
> +    vhost_enq_retries_num = smap_get_int(ovs_other_config,
> +                                          "vhost-enque-retry", 0);
> +
>      argv = grow_argv(&argv, 0, 1);
>      argc = 1;
>      argv[0] = xstrdup(ovs_get_program_name());
> @@ -489,3 +493,9 @@ dpdk_set_lcore_id(unsigned cpu)
>      ovs_assert(cpu != NON_PMD_CORE_ID);
>      RTE_PER_LCORE(_lcore_id) = cpu;
>  }
> +
> +int
> +dpdk_get_vhost_retries(void)
> +{
> +    return vhost_enq_retries_num;
> +}
> diff --git a/lib/dpdk.h b/lib/dpdk.h
> index 673a1f1..9bbd49c 100644
> --- a/lib/dpdk.h
> +++ b/lib/dpdk.h
> @@ -35,5 +35,6 @@ struct smap;
>  void dpdk_init(const struct smap *ovs_other_config);
>  void dpdk_set_lcore_id(unsigned cpu);
>  const char *dpdk_get_vhost_sock_dir(void);
> +int dpdk_get_vhost_retries(void);
>  
>  #endif /* dpdk.h */
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 765718e..a092412 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -146,7 +146,6 @@ BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
>  /* DPDK library uses uint8_t for port_id. */
>  typedef uint8_t dpdk_port_t;
>  
> -#define VHOST_ENQ_RETRY_NUM 8
>  #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ)
>  
>  static const struct rte_eth_conf port_conf = {
> @@ -1727,6 +1726,7 @@ netdev_dpdk_vhost_tx_burst(struct netdev_dpdk *dev, int qid)
>  
>      int tx_vid = netdev_dpdk_get_vid(dev);
>      int tx_qid = qid * VIRTIO_QNUM + VIRTIO_RXQ;
> +    int vhost_retries = dpdk_get_vhost_retries();
>      uint32_t sent = 0;
>      uint32_t retries = 0;
>      uint32_t sum, total_pkts;
> @@ -1745,7 +1745,7 @@ netdev_dpdk_vhost_tx_burst(struct netdev_dpdk *dev, int qid)
>              /* 'sum; packet have to be retransmitted */
>              sum -= ret;
>          }
> -    } while (sum && (retries++ < VHOST_ENQ_RETRY_NUM));
> +    } while (sum && (retries++ < vhost_retries));
>  
>      for (int i=0; i < total_pkts; i++) {
>          dp_packet_delete(txq->pkts[i]);
> diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> index 892f839..f19fa03 100644
> --- a/vswitchd/vswitch.xml
> +++ b/vswitchd/vswitch.xml
> @@ -307,6 +307,18 @@
>          </p>
>        </column>
>  
> +      <column name="other_config" key="vhost-enque-retry"
> +              type='{"type": "integer", "minInteger": 0, "maxInteger": 8}'>
> +        <p>
> +          Specifies the number of retries performed while enqueuing packets
> +          on to the vhost user ports. If this value is unset, no retries by
> +          default is performed on the enqueue side.
> +        </p>
> +        <p>
> +          Changing this value requires restarting the daemon.
> +        </p>
> +      </column>
> +
>        <column name="other_config" key="n-handler-threads"
>                type='{"type": "integer", "minInteger": 1}'>
>          <p>
>

Hi Kevin,

>On 06/07/2017 10:21 AM, Bhanuprakash Bodireddy wrote:
>> This commit adds "vhost-enque-retry" where in the number of retries
>> performed while enqueuing packets to vHostUser ports can be configured
>> in ovsdb.
>>
>> Currently number of retries are set to '8' and a retry is performed
>> when atleast some packets have been successfully sent on previous
>attempt.
>> While this approach works well, it causes throughput drop when
>> multiple vHost User ports are servied by same PMD thread.
>
>Hi Bhanu,
>
>You are saying the approach works well but you are changing the default
>behaviour. It would be good to explain a bit more about the negative effects
>of changing the default and compare that against the positive effects, so
>everyone gets a balanced view. If you have measurements that would be
>even better.

This issue was discussed earlier at different forums (OvS-DPDK day during 2016 fall conference and community call) about the negative effect of retries on vHost User ports. Giving a bit of background for others interested in this problem:

In OvS 2.5 Release: 
The retries on the vHost User ports were performed until a timeout(~100 micro seconds)  is reached. 
The problem with that approach was If the guest is connected and isn't actively processing its queues, it could potentially impact the performance of neighboring guests (other vHost User ports) provided the same PMD thread is servicing them all.  It was reported by me and you indeed provided the fix in 2.6

In OvS 2.6 Release:
Timeout logic is removed and retry logic is introduced. Here a maximum up to '8' retries can be performed provided atleast one packet is transmitted successfully in the previous attempt.  

Problem:
Take the case where there are few VMs (with 3 vHost User ports each) serviced by same PMD thread. Some of the VMs are forwarding at high rates(using dpdk based app) and the remaining are slow VMs doing kernel forwarding in the guest. In this case the PMD would spend significant cycles for slower VMs and may end up doing maximum of 8 retries all the time.  However, in some cases  doing a retry immediately isn't of much value as there may not be any free descriptors available. 

Also if there are more slow ports, the packets can potentially get tail dropped at the NIC as PMD is busy processing the packets and doing retries. I don't have numbers right now to back this problem but can do some tests next week to assess the impact with and without retries. Also adding jan here who wanted the retry logic to be configurable.

Regards,
Bhanuprakash.

>>On 06/07/2017 10:21 AM, Bhanuprakash Bodireddy wrote:
>>> This commit adds "vhost-enque-retry" where in the number of retries
>>> performed while enqueuing packets to vHostUser ports can be
>>> configured in ovsdb.
>>>
>>> Currently number of retries are set to '8' and a retry is performed
>>> when atleast some packets have been successfully sent on previous
>>attempt.
>>> While this approach works well, it causes throughput drop when
>>> multiple vHost User ports are servied by same PMD thread.
>>
>>Hi Bhanu,
>>
>>You are saying the approach works well but you are changing the default
>>behaviour. It would be good to explain a bit more about the negative
>>effects of changing the default and compare that against the positive
>>effects, so everyone gets a balanced view. If you have measurements
>>that would be even better.
>
>This issue was discussed earlier at different forums (OvS-DPDK day during
>2016 fall conference and community call) about the negative effect of retries
>on vHost User ports. Giving a bit of background for others interested in this
>problem:
>
>In OvS 2.5 Release:
>The retries on the vHost User ports were performed until a timeout(~100
>micro seconds)  is reached.
>The problem with that approach was If the guest is connected and isn't
>actively processing its queues, it could potentially impact the performance of
>neighboring guests (other vHost User ports) provided the same PMD thread is
>servicing them all.  It was reported by me and you indeed provided the fix in
>2.6
>
>In OvS 2.6 Release:
>Timeout logic is removed and retry logic is introduced. Here a maximum up to
>'8' retries can be performed provided atleast one packet is transmitted
>successfully in the previous attempt.
>
>Problem:
>Take the case where there are few VMs (with 3 vHost User ports each)
>serviced by same PMD thread. Some of the VMs are forwarding at high
>rates(using dpdk based app) and the remaining are slow VMs doing kernel
>forwarding in the guest. In this case the PMD would spend significant cycles
>for slower VMs and may end up doing maximum of 8 retries all the time.
>However, in some cases  doing a retry immediately isn't of much value as
>there may not be any free descriptors available.
>
>Also if there are more slow ports, the packets can potentially get tail dropped
>at the NIC as PMD is busy processing the packets and doing retries. I don't
>have numbers right now to back this problem but can do some tests next
>week to assess the impact with and without retries. Also adding jan here who
>wanted the retry logic to be configurable.

Hi Kevin,

I did some testing today with and without retries  and found  little performance improvement with retries turned off.
My test bench is pretty basic and not tuned for performance.
 - 2 PMD threads
 - 4 VMs with kernel based forwarding enabled in the guest.
 - VM running 3.x kernel / Qemu 2.5 / mrg_rxbuf=off
- 64 byte packets @ line rate with each VM receiving 25% of the traffic(3.7 mpps).

With retries enabled the aggregate throughput stands at 2.39Mpps in steady state,  whereas with retries turned off It is 2.42 Mpps. 

Regards,
Bhanuprakash.