Re: [dpdk-dev] [PATCH v3] vhost: improve dirty pages logging performance

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 05/17/2018 06:50 AM, Tiwei Bie wrote:
> On Thu, May 17, 2018 at 06:06:34AM +0300, Michael S. Tsirkin wrote:
>> On Wed, May 16, 2018 at 06:54:23PM +0200, Maxime Coquelin wrote:
>>> This patch caches all dirty pages logging until the used ring index
>>> is updated.
>>>
>>> The goal of this optimization is to fix a performance regression
>>> introduced when the vhost library started to use atomic operations
>>> to set bits in the shared dirty log map. While the fix was valid
>>> as previous implementation wasn't safe against concurent accesses,
>>> contention was induced.
>>>
>>> With this patch, during migration, we have:
>>> 1. Less atomic operations as only a single atomic OR operation
>>> per 32 or 64 (depending on CPU) pages.
>>> 2. Less atomic operations as during a burst, the same page will
>>> be marked dirty only once.
>>> 3. Less write memory barriers.
>>>
>>> Fixes: 897f13a1f726 ("vhost: make page logging atomic")
>>>
>>> Cc: stable@dpdk.org
>>>
>>> Cc: Tiwei Bie <tiwei.bie@intel.com>
>>> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>>
>> Deferring updates until GET_BASE would also be possible,
>> but would increase the chance that a disconnect causes
>> ring to become inconsistent.
> 
> Yeah. The sync of the updates from vhost backend will
> be deferred a lot. Another issue is that, it probably
> will increase the downtime, as there will be more pages
> to sync after the old device is stopped and before the
> new device is started.
> 
>>
>> I'm not sure whether there is a chance of that with this
>> patch (in case of a crash after used idx updated but
>> before dirty log update of the used idx), but
>> at least it's not bigger than before this patch.
> 
> The used idx update and the corresponding logging
> are two operations instead of one atomic operation.
> So theoretically, it should be possible.
> 
> I got your point now. Maybe we should add a barrier
> between cache sync and the used idx update to ensure
> that all dirty pages are logged before they can be
> seen by the guest.

Good idea, I'll add a carrier at the end of the sync.

Thanks,
Maxime

> Best regards,
> Tiwei Bie
>