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

["Roger Pau Monné" <royger@freebsd.org>]

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 */
> >
> > You also add an argo_lock to each domain struct which doesn't seem to
> > be mentioned here at all.
>
> You're right! Thanks - that's a nice find. That lock is not used at all.
> I'd missed it since it's just not referenced anywhere in the argo.c file.
> I've removed it.
>
> > Shouldn't that lock be the one that protects d->argo? (instead of this global lock?)
>
> According the design that is in place at the moment, no, but
> I need to study that option a bit before I can comment further on
> whether it would make sense to add it in order to do so.
> I imagine not though because we're not looking to add any more locks.

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.

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.

> > > +/*
> > > + * L2 : The per-domain ring hash lock: d->argo->lock
> > > + * Holding a read lock on L2 protects the ring hash table and
> > > + * the elements in the hash_table d->argo->ring_hash, and
> > > + * the node and id fields in struct argo_ring_info in the
> > > + * hash table.
> > > + * Holding a write lock on L2 protects all of the elements of
> > > + * struct argo_ring_info.
> > > + *
> > > + * To take L2 you must already have R(L1). W(L1) implies W(L2) and L3.
> > > + *
> > > + * L3 : The ringinfo lock: argo_ring_info *ringinfo; ringinfo->lock
> > > + * Protects all the fields within the argo_ring_info, aside from the ones that
> > > + * L2 already protects: node, id, lock.
> > > + *
> > > + * To aquire L3 you must already have R(L2). W(L2) implies L3.
> > > + *
> > > + * Lsend : The per-domain single-sender partner rings lock: d->argo->send_lock
> > > + * Protects the per-domain send hash table : d->argo->send_hash
> > > + * and the elements in the hash table, and the node and id fields
> > > + * in struct argo_send_info in the hash table.
> > > + *
> > > + * To take Lsend, you must already have R(L1). W(L1) implies Lsend.
> > > + * Do not attempt to acquire a L2 on any domain after taking and while
> > > + * holding a Lsend lock -- acquire the L2 (if one is needed) beforehand.
> > > + *
> > > + * Lwildcard : The per-domain wildcard pending list lock: d->argo->wildcard_lock
> > > + * Protects the per-domain list of outstanding signals for space availability
> > > + * on wildcard rings.
> > > + *
> > > + * To take Lwildcard, you must already have R(L1). W(L1) implies Lwildcard.
> > > + * No other locks are acquired after obtaining Lwildcard.
> > > + */
> >
> > IMO I think the locking is overly complicated, and there's no
> > reasoning why so many locks are needed. Wouldn't it be enough to start
> > with a single lock that protects the whole d->argo existence and
> > contents?
> >
> > I would start with a very simple (as simple as possible) locking
> > structure and go improving from there if there are performance
> > bottlenecks.
>
> It definitely doesn't help when there's an extra lock lying around
> just to be confusing. Sorry.
>
> The locking discipline in this code is challenging and you are right that
> there hasn't a explanation given as to _why_ there are the locks that there
> are. I will fix that. I can also review the placement of the ASSERTs that
> check (and document) the locks within the code, if that helps.
>
> The current locking comments describe the how, but the why hasn't been
> covered so far and it is needed. The unreasonably-short version is: this
> code is *hot* when the communication paths are in use -- it operates the
> data path -- and there needs to be isolation for paths using rings from the
> potentially malicious or disruptive activities of other domains, or even
> other vcpus of the same domain operating other rings.

Yes, that’s fine, but as said above I wonder why for example a global
argo_lock is used to protect d->argo, instead of a per-domain lock. At
first sight this doesn’t look like the best approach performance wise.

> I am confident that the locking (that actually gets operated) is correct and
> justified though, and I hope that adding some new clear documentation for it
> can address this.

I’m not saying otherwise, but I cannot assert it either.

> > > +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?

Thanks, Roger.

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