[PATCH 0/4] some fixes target for 18.05

[PATCH 0/4] some fixes target for 18.05

[Jianfeng Tan]

Some trivial fixes, and a vhost memory optimization to avoid polulating
guest memory.

Jianfeng Tan (4):
  vhost: remove unused macro
  vhost: avoid function call in data path
  app/testpmd: add option to avoid lock all memory
  vhost: avoid populate guest memory

 app/test-pmd/parameters.c     |  5 ++++-
 app/test-pmd/testpmd.c        | 32 ++++++++++++++++++--------------
 app/test-pmd/testpmd.h        |  1 +
 lib/librte_vhost/vhost.c      | 13 -------------
 lib/librte_vhost/vhost.h      | 15 ++++++++++++---
 lib/librte_vhost/vhost_user.c |  4 +++-
 6 files changed, 38 insertions(+), 32 deletions(-)

-- 
2.7.4

[PATCH 1/4] vhost: remove unused macro

[Jianfeng Tan]

Cc: tomaszx.kulasek@intel.com
Cc: maxime.coquelin@redhat.com
Cc: yliu@fridaylinux.org

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_vhost/vhost.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d947bc9..ecd5b7b 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -296,7 +296,6 @@ vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
 
 #ifdef RTE_LIBRTE_VHOST_DEBUG
 #define VHOST_MAX_PRINT_BUFF 6072
-#define LOG_LEVEL RTE_LOG_DEBUG
 #define LOG_DEBUG(log_type, fmt, args...) RTE_LOG(DEBUG, log_type, fmt, ##args)
 #define PRINT_PACKET(device, addr, size, header) do { \
 	char *pkt_addr = (char *)(addr); \
@@ -316,7 +315,6 @@ vhost_log_used_vring(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	LOG_DEBUG(VHOST_DATA, "%s", packet); \
 } while (0)
 #else
-#define LOG_LEVEL RTE_LOG_INFO
 #define LOG_DEBUG(log_type, fmt, args...) do {} while (0)
 #define PRINT_PACKET(device, addr, size, header) do {} while (0)
 #endif
-- 
2.7.4

[PATCH 2/4] vhost: avoid function call in data path

[Jianfeng Tan]

Previously, get_device() is a function call. It's OK for slow path
configuration, but takes some cycles for data path.

To avoid that, we turn this function to inline type.

Cc: maxime.coquelin@redhat.com
Cc: yliu@fridaylinux.org

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_vhost/vhost.c | 13 -------------
 lib/librte_vhost/vhost.h | 13 ++++++++++++-
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index a407067..f6f12a0 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -68,19 +68,6 @@ __vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 	return 0;
 }
 
-struct virtio_net *
-get_device(int vid)
-{
-	struct virtio_net *dev = vhost_devices[vid];
-
-	if (unlikely(!dev)) {
-		RTE_LOG(ERR, VHOST_CONFIG,
-			"(%d) device not found.\n", vid);
-	}
-
-	return dev;
-}
-
 void
 cleanup_vq(struct vhost_virtqueue *vq, int destroy)
 {
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index ecd5b7b..3fce13b 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -343,7 +343,18 @@ gpa_to_hpa(struct virtio_net *dev, uint64_t gpa, uint64_t size)
 	return 0;
 }
 
-struct virtio_net *get_device(int vid);
+static __rte_always_inline struct virtio_net *
+get_device(int vid)
+{
+	struct virtio_net *dev = vhost_devices[vid];
+
+	if (unlikely(!dev)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"(%d) device not found.\n", vid);
+	}
+
+	return dev;
+}
 
 int vhost_new_device(void);
 void cleanup_device(struct virtio_net *dev, int destroy);
-- 
2.7.4

[PATCH 3/4] app/testpmd: add option to avoid lock all memory

[Jianfeng Tan]

In some cases, we don't want to lock all memory.

Add an option --no-mlockall to avoid lock all memory.

Cc: wenzhuo.lu@intel.com
Cc: jingjing.wu@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 app/test-pmd/parameters.c |  5 ++++-
 app/test-pmd/testpmd.c    | 32 ++++++++++++++++++--------------
 app/test-pmd/testpmd.h    |  1 +
 3 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 97d22b8..5060a6c 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -186,6 +186,7 @@ usage(char* progname)
 	printf("  --flow-isolate-all: "
 	       "requests flow API isolated mode on all ports at initialization time.\n");
 	printf("  --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n");
+	printf("  --no-mlockall: do not lock all memory\n");
 }
 
 #ifdef RTE_LIBRTE_CMDLINE
@@ -621,6 +622,7 @@ launch_args_parse(int argc, char** argv)
 		{ "print-event",		1, 0, 0 },
 		{ "mask-event",			1, 0, 0 },
 		{ "tx-offloads",		1, 0, 0 },
+		{ "no-mlockall",		0, 0, 0 },
 		{ 0, 0, 0, 0 },
 	};
 
@@ -1102,7 +1104,8 @@ launch_args_parse(int argc, char** argv)
 					rte_exit(EXIT_FAILURE,
 						 "invalid mask-event argument\n");
 				}
-
+			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
+				no_mlockall = 1;
 			break;
 		case 'h':
 			usage(argv[0]);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..59bdc85 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -294,6 +294,10 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RESET) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV);
+/*
+ * Decide if all memory are locked for performance.
+ */
+int no_mlockall = 0;
 
 /*
  * NIC bypass mode configuration options.
@@ -2489,7 +2493,20 @@ main(int argc, char** argv)
 		rte_panic("Cannot register log type");
 	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
 
-	if (mlockall(MCL_CURRENT | MCL_FUTURE)) {
+	/* Bitrate/latency stats disabled by default */
+#ifdef RTE_LIBRTE_BITRATE
+	bitrate_enabled = 0;
+#endif
+#ifdef RTE_LIBRTE_LATENCY_STATS
+	latencystats_enabled = 0;
+#endif
+
+	argc -= diag;
+	argv += diag;
+	if (argc > 1)
+		launch_args_parse(argc, argv);
+
+	if (!no_mlockall && mlockall(MCL_CURRENT | MCL_FUTURE)) {
 		TESTPMD_LOG(NOTICE, "mlockall() failed with error \"%s\"\n",
 			strerror(errno));
 	}
@@ -2511,19 +2528,6 @@ main(int argc, char** argv)
 		rte_panic("Empty set of forwarding logical cores - check the "
 			  "core mask supplied in the command parameters\n");
 
-	/* Bitrate/latency stats disabled by default */
-#ifdef RTE_LIBRTE_BITRATE
-	bitrate_enabled = 0;
-#endif
-#ifdef RTE_LIBRTE_LATENCY_STATS
-	latencystats_enabled = 0;
-#endif
-
-	argc -= diag;
-	argv += diag;
-	if (argc > 1)
-		launch_args_parse(argc, argv);
-
 	if (tx_first && interactive)
 		rte_exit(EXIT_FAILURE, "--tx-first cannot be used on "
 				"interactive mode.\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 153abea..028b873 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -319,6 +319,7 @@ extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
 extern uint32_t event_print_mask;
+extern int no_mlockall; /**< set by "--no-mlockall" parameter */
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 
 #ifdef RTE_LIBRTE_IXGBE_BYPASS
-- 
2.7.4

[PATCH 4/4] vhost: avoid populate guest memory

[Jianfeng Tan]

It's not necessary to polulate guest memory from vhost side.

Cc: maxime.coquelin@redhat.com
Cc: yliu@fridaylinux.org

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 lib/librte_vhost/vhost_user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 90ed211..9bd0391 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 	uint64_t mmap_offset;
 	uint64_t alignment;
 	uint32_t i;
+	int populate;
 	int fd;
 
 	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
@@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 		}
 		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
 
+		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE : 0;
 		mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
-				 MAP_SHARED | MAP_POPULATE, fd, 0);
+				 MAP_SHARED | populate, fd, 0);
 
 		if (mmap_addr == MAP_FAILED) {
 			RTE_LOG(ERR, VHOST_CONFIG,
-- 
2.7.4

Re: [PATCH 3/4] app/testpmd: add option to avoid lock all memory

[Wu, Jingjing]

New options is added, the doc also need to update.


> -----Original Message-----
> From: Tan, Jianfeng
> Sent: Wednesday, February 14, 2018 12:02 PM
> To: dev@dpdk.org
> Cc: Tan, Jianfeng <jianfeng.tan@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu,
> Jingjing <jingjing.wu@intel.com>
> Subject: [PATCH 3/4] app/testpmd: add option to avoid lock all memory
> 
> In some cases, we don't want to lock all memory.
> 
> Add an option --no-mlockall to avoid lock all memory.
> 
> Cc: wenzhuo.lu@intel.com
> Cc: jingjing.wu@intel.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  app/test-pmd/parameters.c |  5 ++++-
>  app/test-pmd/testpmd.c    | 32 ++++++++++++++++++--------------
>  app/test-pmd/testpmd.h    |  1 +
>  3 files changed, 23 insertions(+), 15 deletions(-)
> 
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 97d22b8..5060a6c 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -186,6 +186,7 @@ usage(char* progname)
>  	printf("  --flow-isolate-all: "
>  	       "requests flow API isolated mode on all ports at initialization time.\n");
>  	printf("  --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n");
> +	printf("  --no-mlockall: do not lock all memory\n");
>  }
> 
>  #ifdef RTE_LIBRTE_CMDLINE
> @@ -621,6 +622,7 @@ launch_args_parse(int argc, char** argv)
>  		{ "print-event",		1, 0, 0 },
>  		{ "mask-event",			1, 0, 0 },
>  		{ "tx-offloads",		1, 0, 0 },
> +		{ "no-mlockall",		0, 0, 0 },
>  		{ 0, 0, 0, 0 },
>  	};
> 
> @@ -1102,7 +1104,8 @@ launch_args_parse(int argc, char** argv)
>  					rte_exit(EXIT_FAILURE,
>  						 "invalid mask-event argument\n");
>  				}
> -
> +			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
> +				no_mlockall = 1;
>  			break;
>  		case 'h':
>  			usage(argv[0]);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4c0e258..59bdc85 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -294,6 +294,10 @@ uint32_t event_print_mask = (UINT32_C(1) <<
> RTE_ETH_EVENT_UNKNOWN) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RESET) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
>  			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV);
> +/*
> + * Decide if all memory are locked for performance.
> + */
> +int no_mlockall = 0;
> 
>  /*
>   * NIC bypass mode configuration options.
> @@ -2489,7 +2493,20 @@ main(int argc, char** argv)
>  		rte_panic("Cannot register log type");
>  	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
> 
> -	if (mlockall(MCL_CURRENT | MCL_FUTURE)) {
> +	/* Bitrate/latency stats disabled by default */
> +#ifdef RTE_LIBRTE_BITRATE
> +	bitrate_enabled = 0;
> +#endif
> +#ifdef RTE_LIBRTE_LATENCY_STATS
> +	latencystats_enabled = 0;
> +#endif
> +
> +	argc -= diag;
> +	argv += diag;
> +	if (argc > 1)
> +		launch_args_parse(argc, argv);
> +
> +	if (!no_mlockall && mlockall(MCL_CURRENT | MCL_FUTURE)) {
>  		TESTPMD_LOG(NOTICE, "mlockall() failed with error \"%s\"\n",
>  			strerror(errno));
>  	}
> @@ -2511,19 +2528,6 @@ main(int argc, char** argv)
>  		rte_panic("Empty set of forwarding logical cores - check the "
>  			  "core mask supplied in the command parameters\n");
> 
> -	/* Bitrate/latency stats disabled by default */
> -#ifdef RTE_LIBRTE_BITRATE
> -	bitrate_enabled = 0;
> -#endif
> -#ifdef RTE_LIBRTE_LATENCY_STATS
> -	latencystats_enabled = 0;
> -#endif
> -
> -	argc -= diag;
> -	argv += diag;
> -	if (argc > 1)
> -		launch_args_parse(argc, argv);
> -
>  	if (tx_first && interactive)
>  		rte_exit(EXIT_FAILURE, "--tx-first cannot be used on "
>  				"interactive mode.\n");
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index 153abea..028b873 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -319,6 +319,7 @@ extern volatile int test_done; /* stop packet forwarding when set
> to 1. */
>  extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
>  extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
>  extern uint32_t event_print_mask;
> +extern int no_mlockall; /**< set by "--no-mlockall" parameter */
>  /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
> 
>  #ifdef RTE_LIBRTE_IXGBE_BYPASS
> --
> 2.7.4

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Maxime Coquelin]

Hi Jianfeng,

On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> It's not necessary to polulate guest memory from vhost side.
> 
> Cc: maxime.coquelin@redhat.com
> Cc: yliu@fridaylinux.org
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   lib/librte_vhost/vhost_user.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 90ed211..9bd0391 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>   	uint64_t mmap_offset;
>   	uint64_t alignment;
>   	uint32_t i;
> +	int populate;
>   	int fd;
>   
>   	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
> @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>   		}
>   		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
>   
> +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE : 0;
>   		mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> -				 MAP_SHARED | MAP_POPULATE, fd, 0);
> +				 MAP_SHARED | populate, fd, 0);
>   
>   		if (mmap_addr == MAP_FAILED) {
>   			RTE_LOG(ERR, VHOST_CONFIG,
> 

Wouldn't not populating all the guest memory have a bad impact on 0%
acceptable loss use-cases?

Thanks,
Maxime

Re: [PATCH 2/4] vhost: avoid function call in data path

[Maxime Coquelin]

On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> Previously, get_device() is a function call. It's OK for slow path
> configuration, but takes some cycles for data path.
> 
> To avoid that, we turn this function to inline type.
> 
> Cc: maxime.coquelin@redhat.com
> Cc: yliu@fridaylinux.org
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   lib/librte_vhost/vhost.c | 13 -------------
>   lib/librte_vhost/vhost.h | 13 ++++++++++++-
>   2 files changed, 12 insertions(+), 14 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH 1/4] vhost: remove unused macro

[Maxime Coquelin]

On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> Cc: tomaszx.kulasek@intel.com
> Cc: maxime.coquelin@redhat.com
> Cc: yliu@fridaylinux.org
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   lib/librte_vhost/vhost.h | 2 --
>   1 file changed, 2 deletions(-)

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH 1/4] vhost: remove unused macro

[Maxime Coquelin]

On 02/19/2018 09:49 PM, Maxime Coquelin wrote:
> 
> 
> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
>> Cc: tomaszx.kulasek@intel.com
>> Cc: maxime.coquelin@redhat.com
>> Cc: yliu@fridaylinux.org
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>>   lib/librte_vhost/vhost.h | 2 --
>>   1 file changed, 2 deletions(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>Applied to

Applied to dpdk-next-virtio/master.

Thanks,
Maxime

Re: [PATCH 2/4] vhost: avoid function call in data path

[Maxime Coquelin]

On 02/19/2018 09:48 PM, Maxime Coquelin wrote:
> 
> 
> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
>> Previously, get_device() is a function call. It's OK for slow path
>> configuration, but takes some cycles for data path.
>>
>> To avoid that, we turn this function to inline type.
>>
>> Cc: maxime.coquelin@redhat.com
>> Cc: yliu@fridaylinux.org
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>>   lib/librte_vhost/vhost.c | 13 -------------
>>   lib/librte_vhost/vhost.h | 13 ++++++++++++-
>>   2 files changed, 12 insertions(+), 14 deletions(-)
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-virtio/master.

Thanks,
Maxime

Re: [PATCH 0/4] some fixes target for 18.05

[Maxime Coquelin]

Hi Jianfeng,

On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> Some trivial fixes, and a vhost memory optimization to avoid polulating
> guest memory.
> 
> Jianfeng Tan (4):
>    vhost: remove unused macro
>    vhost: avoid function call in data path
>    app/testpmd: add option to avoid lock all memory
>    vhost: avoid populate guest memory
> 
>   app/test-pmd/parameters.c     |  5 ++++-
>   app/test-pmd/testpmd.c        | 32 ++++++++++++++++++--------------
>   app/test-pmd/testpmd.h        |  1 +
>   lib/librte_vhost/vhost.c      | 13 -------------
>   lib/librte_vhost/vhost.h      | 15 ++++++++++++---
>   lib/librte_vhost/vhost_user.c |  4 +++-
>   6 files changed, 38 insertions(+), 32 deletions(-)
> 

I applied patches 1 & 2 for now, while we discuss patch 4.


Patch 3 is not to be picked by me.

Thanks,
Maxime

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Tan, Jianfeng]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Tuesday, February 20, 2018 4:45 AM
> To: Tan, Jianfeng; dev@dpdk.org
> Cc: yliu@fridaylinux.org
> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
> 
> Hi Jianfeng,
> 
> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> > It's not necessary to polulate guest memory from vhost side.
> >
> > Cc: maxime.coquelin@redhat.com
> > Cc: yliu@fridaylinux.org
> >
> > Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> > ---
> >   lib/librte_vhost/vhost_user.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 90ed211..9bd0391 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev,
> struct VhostUserMsg *pmsg)
> >   	uint64_t mmap_offset;
> >   	uint64_t alignment;
> >   	uint32_t i;
> > +	int populate;
> >   	int fd;
> >
> >   	if (dev->mem && !vhost_memory_changed(&memory, dev->mem))
> {
> > @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev,
> struct VhostUserMsg *pmsg)
> >   		}
> >   		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
> >
> > +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE :
> 0;
> >   		mmap_addr = mmap(NULL, mmap_size, PROT_READ |
> PROT_WRITE,
> > -				 MAP_SHARED | MAP_POPULATE, fd, 0);
> > +				 MAP_SHARED | populate, fd, 0);
> >
> >   		if (mmap_addr == MAP_FAILED) {
> >   			RTE_LOG(ERR, VHOST_CONFIG,
> >
> 
> Wouldn't not populating all the guest memory have a bad impact on 0%
> acceptable loss use-cases?

Yes, it could affect such use case; but we can address that by warming up the system a little bit, can't we?

From a good point of view, it could save the memory for VMs without pre-allocating.

Thanks,
Jianfeng

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Maxime Coquelin]

On 02/22/2018 03:42 AM, Tan, Jianfeng wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Tuesday, February 20, 2018 4:45 AM
>> To: Tan, Jianfeng; dev@dpdk.org
>> Cc: yliu@fridaylinux.org
>> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
>>
>> Hi Jianfeng,
>>
>> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
>>> It's not necessary to polulate guest memory from vhost side.
>>>
>>> Cc: maxime.coquelin@redhat.com
>>> Cc: yliu@fridaylinux.org
>>>
>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>> ---
>>>    lib/librte_vhost/vhost_user.c | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 90ed211..9bd0391 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev,
>> struct VhostUserMsg *pmsg)
>>>    	uint64_t mmap_offset;
>>>    	uint64_t alignment;
>>>    	uint32_t i;
>>> +	int populate;
>>>    	int fd;
>>>
>>>    	if (dev->mem && !vhost_memory_changed(&memory, dev->mem))
>> {
>>> @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev,
>> struct VhostUserMsg *pmsg)
>>>    		}
>>>    		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
>>>
>>> +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE :
>> 0;
>>>    		mmap_addr = mmap(NULL, mmap_size, PROT_READ |
>> PROT_WRITE,
>>> -				 MAP_SHARED | MAP_POPULATE, fd, 0);
>>> +				 MAP_SHARED | populate, fd, 0);
>>>
>>>    		if (mmap_addr == MAP_FAILED) {
>>>    			RTE_LOG(ERR, VHOST_CONFIG,
>>>
>>
>> Wouldn't not populating all the guest memory have a bad impact on 0%
>> acceptable loss use-cases?
> 
> Yes, it could affect such use case; but we can address that by warming up the system a little bit, can't we?

I'm not sure this is a good idea to ask the real user to warm-up the
system.

Also, even with benchmarking, the loss happens when the queues are full,
so it is likely that it happens with buffers not used before, even if
system has been warmed-up.

>  From a good point of view, it could save the memory for VMs without pre-allocating.

What could be done is maybe to have an EAL API for mmaping, with an
associated EAL parameter to state whether it want populating or not.
This option would be disabled by default.

Does that sounds reasonable?

Cheers,
Maxime

> Thanks,
> Jianfeng
> 

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Tan, Jianfeng]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, February 22, 2018 4:26 PM
> To: Tan, Jianfeng; dev@dpdk.org
> Cc: yliu@fridaylinux.org
> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
> 
> 
> 
> On 02/22/2018 03:42 AM, Tan, Jianfeng wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> >> Sent: Tuesday, February 20, 2018 4:45 AM
> >> To: Tan, Jianfeng; dev@dpdk.org
> >> Cc: yliu@fridaylinux.org
> >> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
> >>
> >> Hi Jianfeng,
> >>
> >> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
> >>> It's not necessary to polulate guest memory from vhost side.
> >>>
> >>> Cc: maxime.coquelin@redhat.com
> >>> Cc: yliu@fridaylinux.org
> >>>
> >>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >>> ---
> >>>    lib/librte_vhost/vhost_user.c | 4 +++-
> >>>    1 file changed, 3 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> >>> index 90ed211..9bd0391 100644
> >>> --- a/lib/librte_vhost/vhost_user.c
> >>> +++ b/lib/librte_vhost/vhost_user.c
> >>> @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net
> *dev,
> >> struct VhostUserMsg *pmsg)
> >>>    	uint64_t mmap_offset;
> >>>    	uint64_t alignment;
> >>>    	uint32_t i;
> >>> +	int populate;
> >>>    	int fd;
> >>>
> >>>    	if (dev->mem && !vhost_memory_changed(&memory, dev->mem))
> >> {
> >>> @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net
> *dev,
> >> struct VhostUserMsg *pmsg)
> >>>    		}
> >>>    		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
> >>>
> >>> +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE :
> >> 0;
> >>>    		mmap_addr = mmap(NULL, mmap_size, PROT_READ |
> >> PROT_WRITE,
> >>> -				 MAP_SHARED | MAP_POPULATE, fd, 0);
> >>> +				 MAP_SHARED | populate, fd, 0);
> >>>
> >>>    		if (mmap_addr == MAP_FAILED) {
> >>>    			RTE_LOG(ERR, VHOST_CONFIG,
> >>>
> >>
> >> Wouldn't not populating all the guest memory have a bad impact on 0%
> >> acceptable loss use-cases?
> >
> > Yes, it could affect such use case; but we can address that by warming up
> the system a little bit, can't we?
> 
> I'm not sure this is a good idea to ask the real user to warm-up the
> system.
> 
> Also, even with benchmarking, the loss happens when the queues are full,
> so it is likely that it happens with buffers not used before, even if
> system has been warmed-up.

OK, warm-up is a bad idea here :-)

But if a VM is used for such use case, I think we'd better pre-allocate the memory at QEMU side.

> 
> >  From a good point of view, it could save the memory for VMs without pre-
> allocating.
> 
> What could be done is maybe to have an EAL API for mmaping, with an
> associated EAL parameter to state whether it want populating or not.
> This option would be disabled by default.
> 
> Does that sounds reasonable?

If we look for an application-level configuration, it's not necessary to have such a parameter. Refer to the 3rd patch in this series, if we make all (current/future) memory locked, the mmap() syscall will populate the memory.

Thanks,
Jianfeng

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Maxime Coquelin]

On 02/22/2018 09:40 AM, Tan, Jianfeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Thursday, February 22, 2018 4:26 PM
>> To: Tan, Jianfeng; dev@dpdk.org
>> Cc: yliu@fridaylinux.org
>> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
>>
>>
>>
>> On 02/22/2018 03:42 AM, Tan, Jianfeng wrote:
>>> Hi Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>>>> Sent: Tuesday, February 20, 2018 4:45 AM
>>>> To: Tan, Jianfeng; dev@dpdk.org
>>>> Cc: yliu@fridaylinux.org
>>>> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
>>>>
>>>> Hi Jianfeng,
>>>>
>>>> On 02/14/2018 05:01 AM, Jianfeng Tan wrote:
>>>>> It's not necessary to polulate guest memory from vhost side.
>>>>>
>>>>> Cc: maxime.coquelin@redhat.com
>>>>> Cc: yliu@fridaylinux.org
>>>>>
>>>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>>>> ---
>>>>>     lib/librte_vhost/vhost_user.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>>>> index 90ed211..9bd0391 100644
>>>>> --- a/lib/librte_vhost/vhost_user.c
>>>>> +++ b/lib/librte_vhost/vhost_user.c
>>>>> @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net
>> *dev,
>>>> struct VhostUserMsg *pmsg)
>>>>>     	uint64_t mmap_offset;
>>>>>     	uint64_t alignment;
>>>>>     	uint32_t i;
>>>>> +	int populate;
>>>>>     	int fd;
>>>>>
>>>>>     	if (dev->mem && !vhost_memory_changed(&memory, dev->mem))
>>>> {
>>>>> @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net
>> *dev,
>>>> struct VhostUserMsg *pmsg)
>>>>>     		}
>>>>>     		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
>>>>>
>>>>> +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE :
>>>> 0;
>>>>>     		mmap_addr = mmap(NULL, mmap_size, PROT_READ |
>>>> PROT_WRITE,
>>>>> -				 MAP_SHARED | MAP_POPULATE, fd, 0);
>>>>> +				 MAP_SHARED | populate, fd, 0);
>>>>>
>>>>>     		if (mmap_addr == MAP_FAILED) {
>>>>>     			RTE_LOG(ERR, VHOST_CONFIG,
>>>>>
>>>>
>>>> Wouldn't not populating all the guest memory have a bad impact on 0%
>>>> acceptable loss use-cases?
>>>
>>> Yes, it could affect such use case; but we can address that by warming up
>> the system a little bit, can't we?
>>
>> I'm not sure this is a good idea to ask the real user to warm-up the
>> system.
>>
>> Also, even with benchmarking, the loss happens when the queues are full,
>> so it is likely that it happens with buffers not used before, even if
>> system has been warmed-up.
> 
> OK, warm-up is a bad idea here :-)
> 
> But if a VM is used for such use case, I think we'd better pre-allocate the memory at QEMU side.
> 
>>
>>>   From a good point of view, it could save the memory for VMs without pre-
>> allocating.
>>
>> What could be done is maybe to have an EAL API for mmaping, with an
>> associated EAL parameter to state whether it want populating or not.
>> This option would be disabled by default.
>>
>> Does that sounds reasonable?
> 
> If we look for an application-level configuration, it's not necessary to have such a parameter. Refer to the 3rd patch in this series, if we make all (current/future) memory locked, the mmap() syscall will populate the memory.

OK, but in that case it should be documented.
I see OVS has also a parameter to request the memory to be locked, but 
it seems not to be the default, so the user could face a change in the
behavior it didn't expect.

Thanks,
Maxime

> Thanks,
> Jianfeng
> 

Re: [PATCH 4/4] vhost: avoid populate guest memory

[Tan, Jianfeng]

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Thursday, February 22, 2018 8:33 PM
> To: Tan, Jianfeng; dev@dpdk.org
> Cc: yliu@fridaylinux.org
> Subject: Re: [PATCH 4/4] vhost: avoid populate guest memory
> 
> 
> >> What could be done is maybe to have an EAL API for mmaping, with an
> >> associated EAL parameter to state whether it want populating or not.
> >> This option would be disabled by default.
> >>
> >> Does that sounds reasonable?
> >
> > If we look for an application-level configuration, it's not necessary to have
> such a parameter. Refer to the 3rd patch in this series, if we make all
> (current/future) memory locked, the mmap() syscall will populate the
> memory.
> 
> OK, but in that case it should be documented.
> I see OVS has also a parameter to request the memory to be locked, but
> it seems not to be the default, so the user could face a change in the
> behavior it didn't expect.

Make sense, will send v2 with the note.

Thanks,
Jianfeng

[PATCH v2] app/testpmd: add option to avoid lock all memory

[Jianfeng Tan]

In some cases, we don't want to lock all memory.

Add an option --no-mlockall to avoid lock all memory.

Cc: wenzhuo.lu@intel.com
Cc: jingjing.wu@intel.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 app/test-pmd/parameters.c             |  5 ++++-
 app/test-pmd/testpmd.c                | 32 ++++++++++++++++++--------------
 app/test-pmd/testpmd.h                |  1 +
 doc/guides/testpmd_app_ug/run_app.rst |  4 ++++
 4 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 97d22b8..5060a6c 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -186,6 +186,7 @@ usage(char* progname)
 	printf("  --flow-isolate-all: "
 	       "requests flow API isolated mode on all ports at initialization time.\n");
 	printf("  --tx-offloads=0xXXXXXXXX: hexadecimal bitmask of TX queue offloads\n");
+	printf("  --no-mlockall: do not lock all memory\n");
 }
 
 #ifdef RTE_LIBRTE_CMDLINE
@@ -621,6 +622,7 @@ launch_args_parse(int argc, char** argv)
 		{ "print-event",		1, 0, 0 },
 		{ "mask-event",			1, 0, 0 },
 		{ "tx-offloads",		1, 0, 0 },
+		{ "no-mlockall",		0, 0, 0 },
 		{ 0, 0, 0, 0 },
 	};
 
@@ -1102,7 +1104,8 @@ launch_args_parse(int argc, char** argv)
 					rte_exit(EXIT_FAILURE,
 						 "invalid mask-event argument\n");
 				}
-
+			if (!strcmp(lgopts[opt_idx].name, "no-mlockall"))
+				no_mlockall = 1;
 			break;
 		case 'h':
 			usage(argv[0]);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..59bdc85 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -294,6 +294,10 @@ uint32_t event_print_mask = (UINT32_C(1) << RTE_ETH_EVENT_UNKNOWN) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RESET) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_MACSEC) |
 			    (UINT32_C(1) << RTE_ETH_EVENT_INTR_RMV);
+/*
+ * Decide if all memory are locked for performance.
+ */
+int no_mlockall = 0;
 
 /*
  * NIC bypass mode configuration options.
@@ -2489,7 +2493,20 @@ main(int argc, char** argv)
 		rte_panic("Cannot register log type");
 	rte_log_set_level(testpmd_logtype, RTE_LOG_DEBUG);
 
-	if (mlockall(MCL_CURRENT | MCL_FUTURE)) {
+	/* Bitrate/latency stats disabled by default */
+#ifdef RTE_LIBRTE_BITRATE
+	bitrate_enabled = 0;
+#endif
+#ifdef RTE_LIBRTE_LATENCY_STATS
+	latencystats_enabled = 0;
+#endif
+
+	argc -= diag;
+	argv += diag;
+	if (argc > 1)
+		launch_args_parse(argc, argv);
+
+	if (!no_mlockall && mlockall(MCL_CURRENT | MCL_FUTURE)) {
 		TESTPMD_LOG(NOTICE, "mlockall() failed with error \"%s\"\n",
 			strerror(errno));
 	}
@@ -2511,19 +2528,6 @@ main(int argc, char** argv)
 		rte_panic("Empty set of forwarding logical cores - check the "
 			  "core mask supplied in the command parameters\n");
 
-	/* Bitrate/latency stats disabled by default */
-#ifdef RTE_LIBRTE_BITRATE
-	bitrate_enabled = 0;
-#endif
-#ifdef RTE_LIBRTE_LATENCY_STATS
-	latencystats_enabled = 0;
-#endif
-
-	argc -= diag;
-	argv += diag;
-	if (argc > 1)
-		launch_args_parse(argc, argv);
-
 	if (tx_first && interactive)
 		rte_exit(EXIT_FAILURE, "--tx-first cannot be used on "
 				"interactive mode.\n");
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 153abea..028b873 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -319,6 +319,7 @@ extern volatile int test_done; /* stop packet forwarding when set to 1. */
 extern uint8_t lsc_interrupt; /**< disabled by "--no-lsc-interrupt" parameter */
 extern uint8_t rmv_interrupt; /**< disabled by "--no-rmv-interrupt" parameter */
 extern uint32_t event_print_mask;
+extern int no_mlockall; /**< set by "--no-mlockall" parameter */
 /**< set by "--print-event xxxx" and "--mask-event xxxx parameters */
 
 #ifdef RTE_LIBRTE_IXGBE_BYPASS
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 1fd5395..bdc6262 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -479,3 +479,7 @@ The commandline options are:
 
     Set the hexadecimal bitmask of TX queue offloads.
     The default value is 0.
+
+*   ``--no-mlockall``
+
+    Disable locking all memory.
-- 
2.7.4

[PATCH v2] vhost: avoid populate guest memory

[Jianfeng Tan]

It's not necessary to populate guest memory from vhost side unless
zerocopy is enabled or users want better performance.

Update the doc for guest memory requirement clarification.

Cc: maxime.coquelin@redhat.com

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++++++++++++++
 doc/guides/sample_app_ug/vhost.rst  |  9 ---------
 lib/librte_vhost/vhost_user.c       |  4 +++-
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
index 18227b6..f474736 100644
--- a/doc/guides/prog_guide/vhost_lib.rst
+++ b/doc/guides/prog_guide/vhost_lib.rst
@@ -214,6 +214,27 @@ the vhost device from the data plane.
 
 When the socket connection is closed, vhost will destroy the device.
 
+Guest memory requirement
+------------------------
+
+* Memory pre-allocation
+
+  For non-zerocopy, guest memory pre-allocation is not a must. This can help
+  save of memory. If users really want the guest memory to be pre-allocated
+  (e.g., for performance reason), we can add option ``-mem-prealloc`` when
+  starting QEMU. Or, we can lock all memory at vhost side which will force
+  memory to be allocated when mmap at vhost side; option --mlockall in
+  ovs-dpdk is an example in hand.
+
+  For zerocopy, we force the VM memory to be pre-allocated at vhost lib when
+  mapping the guest memory; and also we need to lock the memory to prevent
+  pages being swapped out to disk.
+
+* Memory sharing
+
+  Make sure ``share=on`` QEMU option is given. vhost-user will not work with
+  a QEMU version without shared memory mapping.
+
 Vhost supported vSwitch reference
 ---------------------------------
 
diff --git a/doc/guides/sample_app_ug/vhost.rst b/doc/guides/sample_app_ug/vhost.rst
index a4bdc6a..da161a9 100644
--- a/doc/guides/sample_app_ug/vhost.rst
+++ b/doc/guides/sample_app_ug/vhost.rst
@@ -175,15 +175,6 @@ Common Issues
   The command above indicates how many hugepages are free to support QEMU's
   allocation request.
 
-* vhost-user will not work with QEMU without the ``-mem-prealloc`` option
-
-  The current implementation works properly only when the guest memory is
-  pre-allocated.
-
-* vhost-user will not work with a QEMU version without shared memory mapping:
-
-  Make sure ``share=on`` QEMU option is given.
-
 * Failed to build DPDK in VM
 
   Make sure "-cpu host" QEMU option is given.
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 90ed211..9bd0391 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 	uint64_t mmap_offset;
 	uint64_t alignment;
 	uint32_t i;
+	int populate;
 	int fd;
 
 	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
@@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
 		}
 		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
 
+		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE : 0;
 		mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
-				 MAP_SHARED | MAP_POPULATE, fd, 0);
+				 MAP_SHARED | populate, fd, 0);
 
 		if (mmap_addr == MAP_FAILED) {
 			RTE_LOG(ERR, VHOST_CONFIG,
-- 
2.7.4

Re: [PATCH v2] vhost: avoid populate guest memory

[Maxime Coquelin]

On 03/28/2018 08:56 AM, Jianfeng Tan wrote:
> It's not necessary to populate guest memory from vhost side unless
> zerocopy is enabled or users want better performance.
> 
> Update the doc for guest memory requirement clarification.
> 
> Cc: maxime.coquelin@redhat.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>   doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++++++++++++++
>   doc/guides/sample_app_ug/vhost.rst  |  9 ---------
>   lib/librte_vhost/vhost_user.c       |  4 +++-
>   3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/doc/guides/prog_guide/vhost_lib.rst b/doc/guides/prog_guide/vhost_lib.rst
> index 18227b6..f474736 100644
> --- a/doc/guides/prog_guide/vhost_lib.rst
> +++ b/doc/guides/prog_guide/vhost_lib.rst
> @@ -214,6 +214,27 @@ the vhost device from the data plane.
>   
>   When the socket connection is closed, vhost will destroy the device.
>   
> +Guest memory requirement
> +------------------------
> +
> +* Memory pre-allocation
> +
> +  For non-zerocopy, guest memory pre-allocation is not a must. This can help
> +  save of memory. If users really want the guest memory to be pre-allocated
> +  (e.g., for performance reason), we can add option ``-mem-prealloc`` when
> +  starting QEMU. Or, we can lock all memory at vhost side which will force
> +  memory to be allocated when mmap at vhost side; option --mlockall in
> +  ovs-dpdk is an example in hand.
> +
> +  For zerocopy, we force the VM memory to be pre-allocated at vhost lib when
> +  mapping the guest memory; and also we need to lock the memory to prevent
> +  pages being swapped out to disk.
> +
> +* Memory sharing
> +
> +  Make sure ``share=on`` QEMU option is given. vhost-user will not work with
> +  a QEMU version without shared memory mapping.
> +

nice write-up!

>   Vhost supported vSwitch reference
>   ---------------------------------
>   
> diff --git a/doc/guides/sample_app_ug/vhost.rst b/doc/guides/sample_app_ug/vhost.rst
> index a4bdc6a..da161a9 100644
> --- a/doc/guides/sample_app_ug/vhost.rst
> +++ b/doc/guides/sample_app_ug/vhost.rst
> @@ -175,15 +175,6 @@ Common Issues
>     The command above indicates how many hugepages are free to support QEMU's
>     allocation request.
>   
> -* vhost-user will not work with QEMU without the ``-mem-prealloc`` option
> -
> -  The current implementation works properly only when the guest memory is
> -  pre-allocated.
> -
> -* vhost-user will not work with a QEMU version without shared memory mapping:
> -
> -  Make sure ``share=on`` QEMU option is given.
> -
>   * Failed to build DPDK in VM
>   
>     Make sure "-cpu host" QEMU option is given.
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 90ed211..9bd0391 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -644,6 +644,7 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>   	uint64_t mmap_offset;
>   	uint64_t alignment;
>   	uint32_t i;
> +	int populate;
>   	int fd;
>   
>   	if (dev->mem && !vhost_memory_changed(&memory, dev->mem)) {
> @@ -714,8 +715,9 @@ vhost_user_set_mem_table(struct virtio_net *dev, struct VhostUserMsg *pmsg)
>   		}
>   		mmap_size = RTE_ALIGN_CEIL(mmap_size, alignment);
>   
> +		populate = (dev->dequeue_zero_copy) ? MAP_POPULATE : 0;
>   		mmap_addr = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> -				 MAP_SHARED | MAP_POPULATE, fd, 0);
> +				 MAP_SHARED | populate, fd, 0);
>   
>   		if (mmap_addr == MAP_FAILED) {
>   			RTE_LOG(ERR, VHOST_CONFIG,
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [PATCH v2] vhost: avoid populate guest memory

[Maxime Coquelin]

On 03/30/2018 10:21 AM, Maxime Coquelin wrote:
> 
> 
> On 03/28/2018 08:56 AM, Jianfeng Tan wrote:
>> It's not necessary to populate guest memory from vhost side unless
>> zerocopy is enabled or users want better performance.
>>
>> Update the doc for guest memory requirement clarification.
>>
>> Cc: maxime.coquelin@redhat.com
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>>   doc/guides/prog_guide/vhost_lib.rst | 21 +++++++++++++++++++++
>>   doc/guides/sample_app_ug/vhost.rst  |  9 ---------
>>   lib/librte_vhost/vhost_user.c       |  4 +++-
>>   3 files changed, 24 insertions(+), 10 deletions(-)
>>
>> diff --git a/doc/guides/prog_guide/vhost_lib.rst 


Applied to dpdk-next-virtio/master

Thanks,
Maxime

Re: [PATCH v2] app/testpmd: add option to avoid lock all memory

[Thomas Monjalon]

This patch has been forgotten.
Review please?

24/02/2018 04:26, Jianfeng Tan:
> In some cases, we don't want to lock all memory.
> 
> Add an option --no-mlockall to avoid lock all memory.
> 
> Cc: wenzhuo.lu@intel.com
> Cc: jingjing.wu@intel.com
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

Re: [PATCH v2] app/testpmd: add option to avoid lock all memory

[Burakov, Anatoly]

On 23-Apr-18 12:05 AM, Thomas Monjalon wrote:
> This patch has been forgotten.
> Review please?
> 
> 24/02/2018 04:26, Jianfeng Tan:
>> In some cases, we don't want to lock all memory.
>>
>> Add an option --no-mlockall to avoid lock all memory.
>>
>> Cc: wenzhuo.lu@intel.com
>> Cc: jingjing.wu@intel.com
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>

I will take this over and submit a new version.

-- 
Thanks,
Anatoly