[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 04/15] argo: init, destroy and soft-reset, with enable command line opt
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |