Re: [PATCH v2] x86/vpt: Do not take pt_migrate rwlock in some cases

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Mar 29, 2021 at 11:56:56AM +0200, Jan Beulich wrote:
> On 27.03.2021 02:51, Boris Ostrovsky wrote:
> > Commit 8e76aef72820 ("x86/vpt: fix race when migrating timers between
> > vCPUs") addressed XSA-336 by introducing a per-domain rwlock that was
> > intended to protect periodic timer during VCPU migration. Since such
> > migration is an infrequent event no performance impact was expected.
> > 
> > Unfortunately this turned out not to be the case: on a fairly large
> > guest (92 VCPUs) we've observed as much as 40% TPCC performance
> > regression with some guest kernels. Further investigation pointed to
> > pt_migrate read lock taken in pt_update_irq() as the largest contributor
> > to this regression. With large number of VCPUs and large number of VMEXITs
> > (from where pt_update_irq() is always called) the update of an atomic in
> > read_lock() is thought to be the main cause.
> > 
> > Stephen Brennan analyzed locking pattern and classified lock users as
> > follows:
> > 
> > 1. Functions which read (maybe write) all periodic_time instances
> > attached to a particular vCPU. These are functions which use pt_vcpu_lock()
> > after the commit, such as pt_restore_timer(), pt_save_timer(), etc.
> > 2. Functions which want to modify a particular periodic_time object.
> > These guys lock whichever vCPU the periodic_time is attached to, but
> > since the vCPU could be modified without holding any lock, they are
> > vulnerable to the bug. Functions in this group use pt_lock(), such as
> > pt_timer_fn() or destroy_periodic_time().
> > 3. Functions which not only want to modify the periodic_time, but also
> > would like to modify the =vcpu= fields. These are create_periodic_time()
> > or pt_adjust_vcpu(). They create the locking imbalance bug for group 2,
> > but we can't simply hold 2 vcpu locks due to the deadlock risk.
> > 
> > Roger Monné then pointed out that group 1 functions don't really need
> > to hold the pt_migrate rwlock and that group 3 should be hardened by
> > holding appropriate vcpu's tm_lock when adding or deleting a timer
> > from its list.
> 
> I'm struggling some with the latter aspect: Is this to mean there is
> something wrong right now?

There's nothing wrong right now AFAICT , because per-vcpu users (ie:
type 1) also hold the rw lock in read mode when iterating over the
per-vcpu list.

> Or does "harden" really mean "just to be
> on the safe side" here?

If the per-domain rw lock is no longer read-locked by type 1 users
then type 2 and type 3 users need to make sure the per-vcpu lock is
taken before adding or removing items from the per-vcpu lists, or else
a type 1 user could see list corruption as a result of modifications
done by type 2 or 3 without holding the per-vcpu lock.

This again makes the locking logic more complicated than it was, so I
will try to get back with my vpt improvements so we can get rid of all
this at least on unstable. Hopefully the comments are clear enough to
follow the logic.

Roger.