Re: [Xen-devel] [PATCH v7 08/12] x86/hvm/ioreq: maintain an array of ioreq servers rather than a list

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 26.09.17 at 12:55, <Paul.Durrant@xxxxxxxxxx> wrote:
>> Sent: 25 September 2017 16:17
>> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
>> >>> On 18.09.17 at 17:31, <paul.durrant@xxxxxxxxxx> wrote:
>> > @@ -785,29 +797,27 @@ int hvm_get_ioreq_server_info(struct domain
>> *d, ioservid_t id,
>> >
>> >      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>> >
>> > -    rc = -ENOENT;
>> > -    list_for_each_entry ( s,
>> > -                          &d->arch.hvm_domain.ioreq_server.list,
>> > -                          list_entry )
>> > -    {
>> > -        if ( s == d->arch.hvm_domain.default_ioreq_server )
>> > -            continue;
>> > +    s = get_ioreq_server(d, id);
>> >
>> > -        if ( s->id != id )
>> > -            continue;
>> > +    rc = -ENOENT;
>> > +    if ( !s )
>> > +        goto out;
>> >
>> > -        *ioreq_gfn = s->ioreq.gfn;
>> > +    rc = -EOPNOTSUPP;
>> > +    if ( IS_DEFAULT(s) )
>> > +        goto out;
>> 
>> Why EOPNOTSUPP when it was just the same ENOENT as no
>> server at all before (same further down)?
>> 
> 
> This was because of comments from Roger. In some cases I think a return of 
> EOPNOTSUPP is more appropriate. Passing the default id is a distinct failure 
> case.

And I think the change is fine as long as the commit message makes
clear it's an intentional change.

>> >  void hvm_destroy_all_ioreq_servers(struct domain *d)
>> >  {
>> > -    struct hvm_ioreq_server *s, *next;
>> > +    struct hvm_ioreq_server *s;
>> > +    unsigned int id;
>> >
>> >      spin_lock_recursive(&d->arch.hvm_domain.ioreq_server.lock);
>> >
>> >      /* No need to domain_pause() as the domain is being torn down */
>> >
>> > -    list_for_each_entry_safe ( s,
>> > -                               next,
>> > -                               &d->arch.hvm_domain.ioreq_server.list,
>> > -                               list_entry )
>> > +    FOR_EACH_IOREQ_SERVER(d, id, s)
>> >      {
>> > -        bool is_default = (s == d->arch.hvm_domain.default_ioreq_server);
>> > -
>> > -        hvm_ioreq_server_disable(s, is_default);
>> > -
>> > -        if ( is_default )
>> > -            d->arch.hvm_domain.default_ioreq_server = NULL;
>> > +        if ( !s )
>> > +            continue;
>> >
>> > -        list_del(&s->list_entry);
>> > +        hvm_ioreq_server_disable(s);
>> > +        hvm_ioreq_server_deinit(s);
>> >
>> > -        hvm_ioreq_server_deinit(s, is_default);
>> > +        ASSERT(d->arch.hvm_domain.ioreq_server.count);
>> > +        --d->arch.hvm_domain.ioreq_server.count;
>> 
>> Seeing this - do you actually need the count as a separate field?
>> I.e. are there performance critical uses of it, where going through
>> the array would be too expensive? Most of the uses are just
>> ASSERT()s anyway.
> 
> The specific case is in hvm_select_ioreq_server(). If there was no count 
> then the array would have to be searched for the initial test.

And is this something that happens frequently, i.e. the
performance of which matters?

Jan


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