[dpdk-stable] [PATCH v16.11 LTS] vhost: avoid concurrency when logging dirty pages

[dpdk-stable] [PATCH v16.11 LTS] vhost: avoid concurrency when logging dirty pages

[Maxime Coquelin]

[ backported from upstream commit 394313fff39d0f994325c47f7eab39daf5dc9e11 ]

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: 6bf02ab821fb ("vhost: make page logging atomic")

Cc: stable@dpdk.org

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>

Conflicts:
        lib/librte_vhost/vhost.h
---

Hi Luca,

This patch is backported from upstream master,
I propose it as the backport requires some conflicts to be fixed.

Cheers,
Maxime

 lib/librte_vhost/virtio_net.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index 0024f729e..ebcb56a91 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -52,18 +52,14 @@
 #define VHOST_LOG_PAGE	4096
 
 /*
- * Atomically set a bit in memory.
- */
-static inline void __attribute__((always_inline))
-vhost_set_bit(unsigned int nr, volatile uint8_t *addr)
-{
-	__sync_fetch_and_or_8(addr, (1U << nr));
-}
-
+ * 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 inline void __attribute__((always_inline))
 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 inline void __attribute__((always_inline))
-- 
2.14.3

Re: [dpdk-stable] [PATCH v16.11 LTS] vhost: avoid concurrency when logging dirty pages

[Maxime Coquelin]

On 04/18/2018 09:24 AM, Maxime Coquelin wrote:
> [ backported from upstream commit 394313fff39d0f994325c47f7eab39daf5dc9e11 ]
> 
> 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: 6bf02ab821fb ("vhost: make page logging atomic")
> 
> Cc: stable@dpdk.org
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Reviewed-by: Jianfeng Tan <jianfeng.tan@intel.com>
> 
> Conflicts:
>          lib/librte_vhost/vhost.h
> ---
> 
> Hi Luca,
> 
> This patch is backported from upstream master,
> I propose it as the backport requires some conflicts to be fixed.
> 
> Cheers,
> Maxime
> 

Hi Luca,

Please don't apply the patch, I have just been made aware by QE that
migration with more than one queue pair never converges, so I need to
rework it.

Cheers,
Maxime