[PATCH] Revert "vhost: avoid concurrency when logging dirty pages"

[PATCH] Revert "vhost: avoid concurrency when logging dirty pages"

[Maxime Coquelin]

This reverts commit 394313fff39d0f994325c47f7eab39daf5dc9e11.

While the patch did solve concurrency issue, it induces more
pages copies as some clean pages are marked as dirty for
performance reasons. Moreover, as there is no more contention
doing the logging, the rate of packets than can be processed is
higher, leading to even more pages to be dirtied.

It has been reported that with more than one queue pair, and
with a relatively low packet rate (1Mpps), the live migration
never converges until the flow is stopped.

While a better solution is found, it is better to reset to the
old behaviour, i.e. using atomic operation for dirty pages
logging.

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

diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
index c9b64461d..4dbed9ad2 100644
--- a/lib/librte_vhost/vhost.h
+++ b/lib/librte_vhost/vhost.h
@@ -304,14 +304,18 @@ struct virtio_net {
 #define VHOST_LOG_PAGE	4096
 
 /*
- * 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.
+ * Atomically set a bit in memory.
  */
+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)
 {
-	log_base[page / 8] = 0xff;
+	vhost_set_bit(page % 8, &log_base[page / 8]);
 }
 
 static __rte_always_inline void
-- 
2.14.3

Re: [PATCH] Revert "vhost: avoid concurrency when logging dirty pages"

[Ferruh Yigit]

On 4/20/2018 9:39 AM, Maxime Coquelin wrote:
> This reverts commit 394313fff39d0f994325c47f7eab39daf5dc9e11.
> 
> While the patch did solve concurrency issue, it induces more
> pages copies as some clean pages are marked as dirty for
> performance reasons. Moreover, as there is no more contention
> doing the logging, the rate of packets than can be processed is
> higher, leading to even more pages to be dirtied.
> 
> It has been reported that with more than one queue pair, and
> with a relatively low packet rate (1Mpps), the live migration
> never converges until the flow is stopped.
> 
> While a better solution is found, it is better to reset to the
> old behaviour, i.e. using atomic operation for dirty pages
> logging.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Applied to dpdk-next-net/master, thanks.