Re: [dpdk-dev] [PATCH] i40evf: use non spinning delay when issuing AQ request to PF

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

> -----Original Message-----
> From: Laurent Hardy <laurent.hardy@6wind.com>
> Sent: Monday, March 30, 2020 10:34 PM
> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Cc: olivier.matz@6wind.com; david.marchand@redhat.com
> Subject: [PATCH] i40evf: use non spinning delay when issuing AQ request to
> PF
> 
> When a port is handled by the i40evf dpdk pmd we could observe a cpu
> usage around 70% in case of rte eth stats functions (rte_eth_stats_get and
> rte_eth_xstats_get) called periodically via an application control thread.
> 
> This is due to the polling mechanism to handle communication between VF
> and PF introduced for x710 (eg: VSI and virtual channel).
> 
> After issuing any request to the PF, the VF will wait in a blocking mode until
> it gets a response from the PF or until timeout (2sec).
> Instead, uses rte_delay_us_sleep to sleep for ASQ_DELAY_MS, which will use
> system sleep and will not block the CPU core.
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> Signed-off-by: Laurent Hardy <laurent.hardy@6wind.com>
> 
> ---
> Hi all,
> Some question coming along with this patch:
> 1)Is there any downside to use the rte_eal_delay_us_sleep which will put the
> thread to sleep instead of the rte_eal_delay_ms (for both control plane and
> dataplane threads) ?

Do we already have API rte_delay_us_callback_register, so user can decide if block or non-block delay they want?

> 
> 2)Another alternative to this patch could be to modify at librte_eal layer the
> function rte_eal_delay to put the thread to sleep in case of application
> control plane thread (by looking at the coreid).
> 
>     --- a/lib/librte_eal/common/include/generic/rte_cycles.h
>     +++ b/lib/librte_eal/common/include/generic/rte_cycles.h
>     @@ -13,9 +13,11 @@
>     */
> 
>      #include <stdint.h>
>     +#include <unistd.h>
>      #include <rte_compat.h>
>      #include <rte_debug.h>
>      #include <rte_atomic.h>
>     +#include <rte_lcore.h>
> 
>      #define MS_PER_S 1000
>      #define US_PER_S 1000000
>     @@ -147,7 +149,10 @@ extern void
>      static inline void
>      rte_delay_ms(unsigned ms)
>      {
>     -	rte_delay_us(ms * 1000);
>     +	if (rte_lcore_id() == LCORE_ID_ANY)
>     +		usleep(ms * 1000);
>     +	else
>     +		rte_delay_us(ms * 1000);
>      }
> 
> regards,
> Laurent
> ---
>  drivers/net/i40e/i40e_ethdev_vf.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/i40e/i40e_ethdev_vf.c
> b/drivers/net/i40e/i40e_ethdev_vf.c
> index 244397e0e..c700c66fd 100644
> --- a/drivers/net/i40e/i40e_ethdev_vf.c
> +++ b/drivers/net/i40e/i40e_ethdev_vf.c
> @@ -315,6 +315,7 @@ _atomic_set_cmd(struct i40e_vf *vf, enum
> virtchnl_ops ops)
> 
>  #define MAX_TRY_TIMES 200
>  #define ASQ_DELAY_MS  10
> +#define DELAY_MS(x) DELAY(x * 1000)

Ideally the macros in base/i40e_osdep.h should only be consumed by base code itself.
The pmd driver that adapt to ethdev should take rte_xxx APIs.

> 
>  static int
>  i40evf_execute_vf_cmd(struct rte_eth_dev *dev, struct vf_cmd_info *args)
> @@ -358,7 +359,7 @@ i40evf_execute_vf_cmd(struct rte_eth_dev *dev,
> struct vf_cmd_info *args)
>  				break;
>  			} else if (ret == I40EVF_MSG_ERR)
>  				break;
> -			rte_delay_ms(ASQ_DELAY_MS);
> +			DELAY_MS(ASQ_DELAY_MS);
>  			/* If don't read msg or read sys event, continue */
>  		} while (i++ < MAX_TRY_TIMES);
>  		_clear_cmd(vf);
> @@ -380,7 +381,7 @@ i40evf_execute_vf_cmd(struct rte_eth_dev *dev,
> struct vf_cmd_info *args)
>  					   ret == I40EVF_MSG_CMD) {
>  				break;
>  			}
> -			rte_delay_ms(ASQ_DELAY_MS);
> +			DELAY_MS(ASQ_DELAY_MS);
>  			/* If don't read msg or read sys event, continue */
>  		} while (i++ < MAX_TRY_TIMES);
>  		_clear_cmd(vf);
> @@ -394,7 +395,7 @@ i40evf_execute_vf_cmd(struct rte_eth_dev *dev,
> struct vf_cmd_info *args)
>  				err = 0;
>  				break;
>  			}
> -			rte_delay_ms(ASQ_DELAY_MS);
> +			DELAY_MS(ASQ_DELAY_MS);
>  			/* If don't read msg or read sys event, continue */
>  		} while (i++ < MAX_TRY_TIMES);
>  		/* If there's no response is received, clear command */
> --
> 2.24.1