----- Original Message ----- > From: "Maxime Coquelin" <maxime.coque...@redhat.com> > To: "Victor Kaplansky" <vkapl...@redhat.com> > Cc: dev@dpdk.org, y...@fridaylinux.org, "tiwei bie" <tiwei....@intel.com>, > "jianfeng tan" <jianfeng....@intel.com>, > sta...@dpdk.org, jfrei...@redhat.com > Sent: Monday, November 27, 2017 10:27:22 AM > Subject: Re: [PATCH v2 2/3] vhost: protect dirty logging against logging base > change > > Hi Victor, > > On 11/27/2017 09:16 AM, Victor Kaplansky wrote: > > Hi, > > > > While I agree that taking full fledged lock by rte_rwlock_read_lock() > > solves the race condition, > > I'm afraid that it would be too expensive in case when logging is off, > > since it introduces > > acquiring and releasing lock into the main flow of ring updates. > > Actually my v2 fixes the performance penalty when logging is off. The > lock is now taken after the logging feature check. > > But still, I agree logging on case will suffer from a performance > penalty.
Yes, checking of logging feature is better than nothing, but VHOST_F_LOG_ALL marks only whether logging is supported by the device and not if logging is in the action. Thus, any guest will hit the performance degradation even not during migration. > > > It is OK for now, as it fixes the bug, but we need to perform more careful > > performance measurements, > > and see whether the performance degradation is not too prohibitive. > > > > As alternative, we may consider using more light weighted busy looping. > > I think it will end up almost being the same, as both threads will need > to busy loop. PMD thread to be sure the protocol thread isn't being > unmapping the region before doing the logging, and protocol thread to be > sure the PMD thread is not doing logging before handling the set log > base. > I'm not fully aware how rte_rwlock_read_lock() is implemented, but theoretically busy looping should be much cheaper in cases when taking lock by one side is very rare. > Maybe you have something else in mind? > > > Also, lets fix by this series the __sync_fetch_and_or_8 -> > > __sync_fetch_and_or, > > as it may improve the performance slightly. > > Sure, this can be done, but it would need to be benchmarked first. Agree. > > Regards, > Maxime >
