Re: [Xen-devel] [PATCH v4 3/8] ioreq-server: create basic ioreq server abstraction.

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 07 April 2014 12:37
> To: Paul Durrant
> Cc: xen-devel@xxxxxxxxxxxxx; Keir (Xen.org)
> Subject: Re: [PATCH v4 3/8] ioreq-server: create basic ioreq server
> abstraction.
> 
> >>> On 02.04.14 at 17:11, <paul.durrant@xxxxxxxxxx> wrote:
> > +static int hvm_ioreq_server_add_vcpu(struct hvm_ioreq_server *s,
> > +                                     struct vcpu *v)
> > +{
> > +    struct hvm_ioreq_vcpu *sv;
> > +    int rc;
> > +
> > +    spin_lock(&s->lock);
> > +
> > +    sv = xzalloc(struct hvm_ioreq_vcpu);
> > +
> > +    rc = -ENOMEM;
> > +    if ( !sv )
> > +        goto fail1;
> 
> I don't see why this allocation needs to be done with the lock already
> held. For the other (event channel) allocations further down I would
> also prefer if you allocated them without holding the lock yet, even
> if that means freeing the per-domain one if you find it already set.
> 

Ok.

> > +
> > +    rc = alloc_unbound_xen_event_channel(v, s->domid, NULL);
> > +    if ( rc < 0 )
> > +        goto fail2;
> > +
> > +    sv->ioreq_evtchn = rc;
> > +
> > +    if ( v->vcpu_id == 0 )
> > +    {
> 
> I generally dislike needless dependencies on vCPU 0 being the first
> one to make it into any specific function. Can't you check emptiness
> of s->ioreq_vcpu_list instead?
> 

That will need a bit more code. I could create the buffered channel on first 
cpu addition but, I'd then need to track which cpu that  was and re-plumb the 
event channel if that cpu disappears. Also, the default server has always bound 
the buffered channel to cpu 0 so would I not risk a compatibility issue by 
changing this?

> > +static void hvm_ioreq_server_remove_vcpu(struct hvm_ioreq_server *s,
> > +                                         struct vcpu *v)
> > +{
> > +    struct list_head *entry;
> > +
> > +    spin_lock(&s->lock);
> > +
> > +    list_for_each ( entry, &s->ioreq_vcpu_list )
> > +    {
> > +        struct hvm_ioreq_vcpu *sv = container_of(entry,
> > +                                                 struct hvm_ioreq_vcpu,
> > +                                                 list_entry);
> > +
> > +        if ( sv->vcpu != v )
> > +            continue;
> > +
> > +        list_del_init(&sv->list_entry);
> > +
> > +        if ( v->vcpu_id == 0 )
> > +            free_xen_event_channel(v, s->bufioreq_evtchn);
> > +
> > +        free_xen_event_channel(v, sv->ioreq_evtchn);
> > +
> > +        xfree(sv);
> > +        break;
> 
> Similar comments as above: Try to avoid depending on vCPU 0 being
> the last one to be cleaned up (I'm not even certain this is the case),
> and try freeing stuff with the lock already dropped.
> 
> > +static int hvm_set_ioreq_pfn(struct domain *d, bool_t buf,
> > +                             unsigned long pfn)
> > +{
> > +    struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> > +    struct hvm_ioreq_page *iorp = buf ? &s->bufioreq : &s->ioreq;
> > +    int rc;
> > +
> > +    spin_lock(&s->lock);
> > +
> > +    rc = hvm_map_ioreq_page(d, iorp, pfn);
> 
> While I realize that at this point there's only one server per domain,
> the locking and operation still look to be out of sync at the first
> glance: As this can't remain that way anyway till the end of the series,
> can't this be brought back in sync here right away (whether that's by
> passing s instead of d into the function or acquiring the lock only after
> the call I don't know offhand)?

I don't follow what you mean by 'out of sync' here: The lock is taken, the pfn 
is mapped, the lock is dropped. What am I missing?

> 
> > +    if ( rc )
> > +        goto fail;
> > +
> > +    if (!buf) {
> 
> Coding style.
> 

Ok.

> > +static int hvm_replace_event_channel(struct vcpu *v, domid_t
> remote_domid,
> > +                                     evtchn_port_t *p_port)
> > +{
> > +    evtchn_port_t old_port, new_port;
> > +
> > +    new_port = alloc_unbound_xen_event_channel(v, remote_domid,
> NULL);
> > +    if ( new_port < 0 )
> 
> evtchn_port_t is an unsigned type, so this check won't work.
> 

Yes, it should be an int. Over-zealous type cleanup.

> > +static int hvm_set_dm_domain(struct domain *d, domid_t domid)
> > +{
> > +    struct hvm_ioreq_server *s = d->arch.hvm_domain.ioreq_server;
> > +    int rc = 0;
> > +
> > +    spin_lock(&s->lock);
> > +    domain_pause(d);
> 
> The other way around perhaps?

Yes.

> 
> > +
> > +    if ( s->domid != domid ) {
> 
> Coding style again.
> 
> > +        struct list_head *entry;
> > +
> > +        list_for_each ( entry, &s->ioreq_vcpu_list )
> > +        {
> > +            struct hvm_ioreq_vcpu *sv = container_of(entry,
> > +                                                     struct hvm_ioreq_vcpu,
> > +                                                     list_entry);
> > +            struct vcpu *v = sv->vcpu;
> > +
> > +            if ( v->vcpu_id == 0 ) {
> 
> And again; won't make further remarks to this effect.
> 
> >  int hvm_domain_initialise(struct domain *d)
> >  {
> > +    domid_t domid;
> 
> Do you really need this new variable, being used just once? (There
> is at least one more similar case elsewhere.)

This is to avoid a line becoming very long.

> 
> > @@ -1339,30 +1549,10 @@ int hvm_vcpu_initialise(struct vcpu *v)
> >           && (rc = nestedhvm_vcpu_initialise(v)) < 0 ) /* teardown:
> > nestedhvm_vcpu_destroy */
> >          goto fail5;
> >
> > -    dm_domid = d-
> >arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
> > -
> > -    /* Create ioreq event channel. */
> > -    rc = alloc_unbound_xen_event_channel(v, dm_domid, NULL); /*
> teardown: none */
> > -    if ( rc < 0 )
> > +    rc = hvm_ioreq_server_add_vcpu(s, v);
> > +    if ( rc != 0 )
> 
> Can this really be > 0 now, and if so is this being handled correctly in
> the caller(s)?

It's not the same function being called. hvm_ioreq_server_add_vcpu () can only 
return 0 or a -ve errno. I can test for < 0 but IIRC you objected to doing that 
in a review of a previous patch if the function never returns > 0. Would you 
prefer 'if ( rc )'?

  Paul

> 
> Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel