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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Wed, Jan 9, 2019 at 1:48 AM Christopher Clark
<christopher.w.clark@xxxxxxxxx> wrote:
>
> On Tue, Jan 8, 2019 at 2:54 PM Jason Andryuk <jandryuk@xxxxxxxxx> wrote:
> >
> > On Mon, Jan 7, 2019 at 2:43 AM Christopher Clark
> > <christopher.w.clark@xxxxxxxxx> wrote:

> > > +
> > > +/* A space-available notification that is awaiting sufficient space */
> > > +struct pending_ent
> > > +{
> > > +    /* List node within argo_ring_info's pending list */
> > > +    struct hlist_node node;
> > > +    /*
> > > +     * List node within argo_domain's wildcard_pend_list. Only used if 
> > > the
> > > +     * ring is one with a wildcard partner (ie. that any domain may send 
> > > to)
> > > +     * to enable cancelling signals on wildcard rings on domain destroy.
> > > +     */
> > > +    struct hlist_node wildcard_node;
> > > +    /*
> > > +     * Pointer to the ring_info that this ent pertains to. Used to 
> > > ensure that
> > > +     * ring_info->npending is decremented when ents for wildcard rings 
> > > are
> > > +     * cancelled for domain destroy.
> > > +     * Caution: Must hold the correct locks before accessing ring_info 
> > > via this.
> >
> > It would be clearer if this stated the correct locks.
>
> ok - it would mean duplicating the statement about which locks are
> needed though, since it is explained elsewhere in the file, which means
> it will need updating in two places if the locking requirements change.
> That was why I worded it that way, as an indicator to go and find where
> it is already described, to avoid that.

"Caution" made me think *ring_info points from domain A's pending_ent
to domain B's ring_info.  Reading patch 10 (notify op) I see that it
really just points back to domain A's ring_info.  So the "Caution" is
just that you still have to lock ring_info (L3) even though you can
get to the pointer via L2.  Is that correct?

I agree a single location for the locking documentation is better than
splitting or duplicating.  As long as no cross-domain locking is
required, this is fine.

> > > +     */
> > > +    struct argo_ring_info *ring_info;
> > > +    /* domain to be notified when space is available */
> > > +    domid_t domain_id;
> > > +    uint16_t pad;
> >
> > Can we order domain_id after len and drop the pad?
>
> I'm not sure that would be right to do that. I think that the pad
> ensures that len is word aligned to 32-bit boundary.  I was asked to
> insert a pad field for such a struct like this in an earlier review here:
>
> https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg00239.html

I'll respond to this in the other email from Jan.

> > > +
> > > +static void
> > > +wildcard_rings_pending_remove(struct domain *d)
> > > +{
> > > +    struct hlist_node *node, *next;
> > > +    struct pending_ent *ent;
> > > +
> > > +    ASSERT(rw_is_write_locked(&argo_lock));
> > > +
> > > +    hlist_for_each_entry_safe(ent, node, next, 
> > > &d->argo->wildcard_pend_list,
> > > +                              node)
> > > +    {
> > > +        hlist_del(&ent->node);
> > > +        ent->ring_info->npending--;
> > > +        hlist_del(&ent->wildcard_node);
> > > +        xfree(ent);
> > > +    }
> > > +}
> > > +
> >
> > Maybe move ring_unmap() here so it's closer to where it is used?
>
> I'm fine with moving it if it needs it, but it's located where it is in
> order to put it right next to the corresponding ring_map_page function -
> the two are paired really, with one doing map_domain_page_global and the
> other undoing it with unmap_domain_page_global. That's how it ends up
> when the full series is applied.

Okay.  My comment came only from reading only this single patch.

> > > +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 )
> >
> > Shouldn't this function just exit early if argo is disabled?
>
> There has been support added to Xen with a hypercall to make a subset of
> boot parameters modifiable at runtime. Argo-enabled isn't currently one
> of them, but that may be changed later so I did not want to bake into
> this function the assumption that the enabled/disabled configuration
> could not change after being initially evaluated at the time the domain
> was launched. That's possibly a conservative choice though.
>
> http://xenbits.xen.org/gitweb/?p=xen.git;a=commit;h=82cf78468e96de1e4d1400bbf5508f8b111650c3

Okay.  I was not aware of this functionality.

Regards,
Jason

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