Re: [PATCH 1/2] xen: add domid_to_domain() helper

[Juergen Gross <jgross@xxxxxxxx>]

On 12.09.22 10:19, Jan Beulich wrote:
> On 12.09.2022 07:53, Juergen Gross wrote:
> > Add a helper domid_to_domain() returning the struct domain pointer for
> > a domain give by its domid and which is known not being able to be
> > released (its reference count isn't incremented and no rcu_lock_domain()
> > is called for it).
> >
> > In order to simplify coding add an internal helper for doing the lookup
> > and call that from the new function and similar functions.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> I don't see an issue with adding such a helper (responding to your concern
> in the cover letter), but I think the constraints need to be empahsized
> more: We already have get_knownalive_domain() and get_domain_by_id(), so
> how about naming the new helper get_knownalive_domain_by_id()? And then ...
I explicitly didn't name it "get_...", as those helpers all increment the
reference count of the domain. And this is NOT done by the new helper.

> > @@ -859,20 +866,27 @@ struct domain *get_domain_by_id(domid_t dom)
> >   
> >   struct domain *rcu_lock_domain_by_id(domid_t dom)
> >   {
> > -    struct domain *d = NULL;
> > +    struct domain *d;
> >   
> >       rcu_read_lock(&domlist_read_lock);
> >   
> > -    for ( d = rcu_dereference(domain_hash[DOMAIN_HASH(dom)]);
> > -          d != NULL;
> > -          d = rcu_dereference(d->next_in_hashbucket) )
> > -    {
> > -        if ( d->domain_id == dom )
> > -        {
> > -            rcu_lock_domain(d);
> > -            break;
> > -        }
> > -    }
> > +    d = domid_2_domain(dom);
> > +    if ( d )
> > +        rcu_lock_domain(d);
> > +
> > +    rcu_read_unlock(&domlist_read_lock);
> > +
> > +    return d;
> > +}
> > +
> > +/* Use only if struct domain is known to stay allocated! */
> > +struct domain *domid_to_domain(domid_t dom)
> > +{
> > +    struct domain *d;
> > +
> > +    rcu_read_lock(&domlist_read_lock);
> > +
> > +    d = domid_2_domain(dom);
> >   
> >       rcu_read_unlock(&domlist_read_lock);
> ... extend the comment here and in the header (or perhaps one in
> the header would suffice and the definition here doesn't need any
> further comment) to explicitly say "reference held or RCU-locked".
Yes, good idea.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature