DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages
@ 2018-03-21 15:44 Maxime Coquelin
  2018-03-27  1:15 ` Tan, Jianfeng
  2018-03-30  7:42 ` Maxime Coquelin
  0 siblings, 2 replies; 4+ messages in thread
From: Maxime Coquelin @ 2018-03-21 15:44 UTC (permalink / raw)
  To: dev, jianfeng.tan, tiwei.bie, jfreimann; +Cc: stable, Maxime Coquelin

This patch aims at fixing a migration performance regression
faced since atomic operation is used to log pages as dirty when
doing live migration.

Instead of setting a single bit by doing an atomic read-modify-write
operation to log a page as dirty, this patch write 0xFF to the
corresponding byte, and so logs 8 page as dirty.

The advantage is that it avoids concurrent atomic operations by
multiple PMD threads, the drawback is that some clean pages are
marked as dirty and so are transferred twice.

Fixes: 897f13a1f726 ("vhost: make page logging atomic")
Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.h | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index d947bc9e3..aa2606f8a 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -247,18 +247,14 @@ struct virtio_net {
 #define VHOST_LOG_PAGE	4096
 
 /*
- * Atomically set a bit in memory.
+ * Mark all pages belonging to the same dirty log bitmap byte
+ * as dirty. The goal is to avoid concurrency between different
+ * threads doing atomic read-modify-writes on the same byte.
  */
-static __rte_always_inline void
-vhost_set_bit(unsigned int nr, volatile uint8_t *addr)
-{
-	__sync_fetch_and_or_8(addr, (1U << nr));
-}
-
 static __rte_always_inline void
 vhost_log_page(uint8_t *log_base, uint64_t page)
 {
-	vhost_set_bit(page % 8, &log_base[page / 8]);
+	log_base[page / 8] = 0xff;
 }
 
 static __rte_always_inline void
-- 
2.14.3

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages
  2018-03-21 15:44 [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages Maxime Coquelin
@ 2018-03-27  1:15 ` Tan, Jianfeng
  2018-03-27 11:24   ` Maxime Coquelin
  2018-03-30  7:42 ` Maxime Coquelin
  1 sibling, 1 reply; 4+ messages in thread
From: Tan, Jianfeng @ 2018-03-27  1:15 UTC (permalink / raw)
  To: Maxime Coquelin, dev, Bie, Tiwei, jfreimann; +Cc: stable



> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Wednesday, March 21, 2018 11:44 PM
> To: dev@dpdk.org; Tan, Jianfeng; Bie, Tiwei; jfreimann@redhat.com
> Cc: stable@dpdk.org; Maxime Coquelin
> Subject: [PATCH] vhost: avoid concurrency when logging dirty pages
> 
> This patch aims at fixing a migration performance regression
> faced since atomic operation is used to log pages as dirty when
> doing live migration.
> 
> Instead of setting a single bit by doing an atomic read-modify-write
> operation to log a page as dirty, this patch write 0xFF to the
> corresponding byte, and so logs 8 page as dirty.
> 
> The advantage is that it avoids concurrent atomic operations by
> multiple PMD threads, the drawback is that some clean pages are
> marked as dirty and so are transferred twice.
> 
> Fixes: 897f13a1f726 ("vhost: make page logging atomic")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

As you mentioned, the concern is if it affects the rate of convergence. Hope we could have some tests on that.

Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks!

> ---
>  lib/librte_vhost/vhost.h | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index d947bc9e3..aa2606f8a 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -247,18 +247,14 @@ struct virtio_net {
>  #define VHOST_LOG_PAGE	4096
> 
>  /*
> - * Atomically set a bit in memory.
> + * Mark all pages belonging to the same dirty log bitmap byte
> + * as dirty. The goal is to avoid concurrency between different
> + * threads doing atomic read-modify-writes on the same byte.
>   */
> -static __rte_always_inline void
> -vhost_set_bit(unsigned int nr, volatile uint8_t *addr)
> -{
> -	__sync_fetch_and_or_8(addr, (1U << nr));
> -}
> -
>  static __rte_always_inline void
>  vhost_log_page(uint8_t *log_base, uint64_t page)
>  {
> -	vhost_set_bit(page % 8, &log_base[page / 8]);
> +	log_base[page / 8] = 0xff;
>  }
> 
>  static __rte_always_inline void
> --
> 2.14.3

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages
  2018-03-27  1:15 ` Tan, Jianfeng
@ 2018-03-27 11:24   ` Maxime Coquelin
  0 siblings, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2018-03-27 11:24 UTC (permalink / raw)
  To: Tan, Jianfeng, dev, Bie, Tiwei, jfreimann; +Cc: stable



On 03/27/2018 03:15 AM, Tan, Jianfeng wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
>> Sent: Wednesday, March 21, 2018 11:44 PM
>> To: dev@dpdk.org; Tan, Jianfeng; Bie, Tiwei; jfreimann@redhat.com
>> Cc: stable@dpdk.org; Maxime Coquelin
>> Subject: [PATCH] vhost: avoid concurrency when logging dirty pages
>>
>> This patch aims at fixing a migration performance regression
>> faced since atomic operation is used to log pages as dirty when
>> doing live migration.
>>
>> Instead of setting a single bit by doing an atomic read-modify-write
>> operation to log a page as dirty, this patch write 0xFF to the
>> corresponding byte, and so logs 8 page as dirty.
>>
>> The advantage is that it avoids concurrent atomic operations by
>> multiple PMD threads, the drawback is that some clean pages are
>> marked as dirty and so are transferred twice.
>>
>> Fixes: 897f13a1f726 ("vhost: make page logging atomic")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> As you mentioned, the concern is if it affects the rate of convergence. Hope we could have some tests on that.

We tested internally with live migration during PVP testing, at a rate
of 3Mpps.
In these conditions, we obtain same numbers as if we revert
897f13a1f726.

> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Thanks,
Maxime

> Thanks!
> 
>> ---
>>   lib/librte_vhost/vhost.h | 12 ++++--------
>>   1 file changed, 4 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
>> index d947bc9e3..aa2606f8a 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -247,18 +247,14 @@ struct virtio_net {
>>   #define VHOST_LOG_PAGE	4096
>>
>>   /*
>> - * Atomically set a bit in memory.
>> + * Mark all pages belonging to the same dirty log bitmap byte
>> + * as dirty. The goal is to avoid concurrency between different
>> + * threads doing atomic read-modify-writes on the same byte.
>>    */
>> -static __rte_always_inline void
>> -vhost_set_bit(unsigned int nr, volatile uint8_t *addr)
>> -{
>> -	__sync_fetch_and_or_8(addr, (1U << nr));
>> -}
>> -
>>   static __rte_always_inline void
>>   vhost_log_page(uint8_t *log_base, uint64_t page)
>>   {
>> -	vhost_set_bit(page % 8, &log_base[page / 8]);
>> +	log_base[page / 8] = 0xff;
>>   }
>>
>>   static __rte_always_inline void
>> --
>> 2.14.3
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages
  2018-03-21 15:44 [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages Maxime Coquelin
  2018-03-27  1:15 ` Tan, Jianfeng
@ 2018-03-30  7:42 ` Maxime Coquelin
  1 sibling, 0 replies; 4+ messages in thread
From: Maxime Coquelin @ 2018-03-30  7:42 UTC (permalink / raw)
  To: dev, jianfeng.tan, tiwei.bie, jfreimann; +Cc: stable



On 03/21/2018 04:44 PM, Maxime Coquelin wrote:
> This patch aims at fixing a migration performance regression
> faced since atomic operation is used to log pages as dirty when
> doing live migration.
> 
> Instead of setting a single bit by doing an atomic read-modify-write
> operation to log a page as dirty, this patch write 0xFF to the
> corresponding byte, and so logs 8 page as dirty.
> 
> The advantage is that it avoids concurrent atomic operations by
> multiple PMD threads, the drawback is that some clean pages are
> marked as dirty and so are transferred twice.
> 
> Fixes: 897f13a1f726 ("vhost: make page logging atomic")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>   lib/librte_vhost/vhost.h | 12 ++++--------
>   1 file changed, 4 insertions(+), 8 deletions(-)

Applied to dpdk-next-virtio/master.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-03-30  7:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-21 15:44 [dpdk-dev] [PATCH] vhost: avoid concurrency when logging dirty pages Maxime Coquelin
2018-03-27  1:15 ` Tan, Jianfeng
2018-03-27 11:24   ` Maxime Coquelin
2018-03-30  7:42 ` Maxime Coquelin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).