Re: [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt

[Christopher Clark <christopher.w.clark@gmail.com>]

On Fri, Jan 11, 2019 at 1:27 AM Roger Pau Monné <royger@freebsd.org> wrote:
> On Fri, Jan 11, 2019 at 7:04 AM Christopher Clark
> <christopher.w.clark@gmail.com> wrote:
> >
> > On Thu, Jan 10, 2019 at 2:19 AM Roger Pau Monné <royger@freebsd.org> wrote:
> > >
> > >  On Mon, Jan 7, 2019 at 8:44 AM Christopher Clark
> > > <christopher.w.clark@gmail.com> wrote:
> > > > +/*
> > > > + * Locking is organized as follows:
> > > > + *
> > > > + * Terminology: R(<lock>) means taking a read lock on the specified lock;
> > > > + *              W(<lock>) means taking a write lock on it.
> > > > + *
> > > > + * L1 : The global lock: argo_lock
> > > > + * Protects the argo elements of all struct domain *d in the system.
> > > > + * It does not protect any of the elements of d->argo, only their
> > > > + * addresses.
> > > > + *
> > > > + * By extension since the destruction of a domain with a non-NULL
> > > > + * d->argo will need to free the d->argo pointer, holding W(L1)
> > > > + * guarantees that no domains pointers that argo is interested in
> > > > + * become invalid whilst this lock is held.
> > > > + */
> > > > +
> > > > +static DEFINE_RWLOCK(argo_lock); /* L1 */
>
> I'm wondering why a global argo_lock shared with all domains is used
> to protect d->argo, instead of using a per-domain lock (d->argo_lock
> for example). This global argo_lock shared between all domains is
> going to introduce contention with no specific benefit AFAICT.

The granular locking structure is motivated by:
1) Performance isolation / DoS avoidance
2) Teardown of state across multiple domains on domain destroy
3) Performance via concurrent operation of rings

Using the single global lock avoids the need for sequencing the
acquisition of multiple individual per-domain locks (and lower level
data structure locks) to prevent deadlock: taking W(L1) grants access
to all and taking R(L1) ensures that teardown of any domain will not
interfere with any Argo hypercall operation. It supports using the
granular locks across domains without complicated or fragile lock
acquisition logic.

I've written a document about the locking to add to the tree with the
series, and a copy is at github here:

https://github.com/dozylynx/xen/blob/0cb95385eba696ecf4856075a524c5e528e60455/docs/misc/argo-locking.md

> I would recommend an initial implementation that uses a single
> per-domain lock (ie: d->argo_lock) to protect the whole contents of
> d->argo, and then go adding more fine grained locking as required,
> providing evidence that such fine grainer locking is actually
> improving performance (or required for some other reason). IMO, the
> current locking scheme is overly complicated, and it's very hard for
> me to reason about it's correctness.

I've now implemented some macros to describe and document locking
requirements in the code, and simplify ASSERTing the correct status.
The majority of functions have a single annotation at entry indicating
and verifying their locking status. They are described in the doc.

>
> > > > +void
> > > > +argo_soft_reset(struct domain *d)
> > > > +{
> > > > +    write_lock(&argo_lock);
> > > > +
> > > > +    argo_dprintk("soft reset d=%d d->argo=%p\n", d->domain_id, d->argo);
> > > > +
> > > > +    if ( d->argo )
> > > > +    {
> > > > +        domain_rings_remove_all(d);
> > > > +        partner_rings_remove(d);
> > > > +        wildcard_rings_pending_remove(d);
> > > > +
> > > > +        if ( !opt_argo_enabled )
> > > > +        {
> > > > +            xfree(d->argo);
> > > > +            d->argo = NULL;
> > >
> > > Can opt_argo_enabled change during runtime?
> >
> > Not at the moment, no. It should be made changeable
> > later, but keeping it fixed assists with derisking this for
> > release consideration.
>
> Then if d->argo is set opt_argo_enabled must be true, and thus this
> condition is never true?

Yes; ack, have removed this logic.

I'm holding off on posting v4 of the series but my latest tree, with the
new macros applied, is on github at:

branch:
https://github.com/dozylynx/xen/tree/staging-argo-2019-01-13
main file within that branch:
https://github.com/dozylynx/xen/blob/staging-argo-2019-01-13/xen/common/argo.c

Given the limited time remaining for the merge for 4.12 under
consideration, if there are any further reservations, please let me
know.

Christopher

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel