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@suse.com>
---
xen/common/domain.c | 54 ++++++++++++++++++++++++++---------------
xen/include/xen/sched.h | 4 +++
2 files changed, 38 insertions(+), 20 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index c23f449451..2b1866ea42 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -832,25 +832,32 @@ out:
return 0;
}
-
-struct domain *get_domain_by_id(domid_t dom)
+/* rcu_read_lock(&domlist_read_lock) must be held. */
+static struct domain *domid_2_domain(domid_t dom)
{
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 )
- {
- if ( unlikely(!get_domain(d)) )
- d = NULL;
- break;
- }
+ return d;
}
+ return NULL;
+}
+
+struct domain *get_domain_by_id(domid_t dom)
+{
+ struct domain *d;
+
+ rcu_read_lock(&domlist_read_lock);
+
+ d = domid_2_domain(dom);
+ if ( d && unlikely(!get_domain(d)) )
+ d = NULL;
+
rcu_read_unlock(&domlist_read_lock);
return d;
@@ -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);
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 557b3229f6..f4c4d3a60f 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -737,8 +737,12 @@ static inline struct domain *rcu_lock_current_domain(void)
return /*rcu_lock_domain*/(current->domain);
}
+/* Get struct domain AND increase ref-count of domain. */
struct domain *get_domain_by_id(domid_t dom);
+/* Get struct domain known to stay allocated. */
+struct domain *domid_to_domain(domid_t dom);
+
struct domain *get_pg_owner(domid_t domid);
static inline void put_pg_owner(struct domain *pg_owner)
--
2.35.3On 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@suse.com>
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 ...
> @@ -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".
Jan
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@suse.com>
>
> 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
On 12.09.2022 10:23, Juergen Gross wrote: > 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@suse.com> >> >> 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. Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" aspect, yet that's relevant to see when reviewing new uses of the function. Such uses aren't likely to make the reviewer go look at the function declaration when the function name is pretty "innocent". Jan
On 12.09.22 10:31, Jan Beulich wrote: > On 12.09.2022 10:23, Juergen Gross wrote: >> 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@suse.com> >>> >>> 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. > > Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" aspect, > yet that's relevant to see when reviewing new uses of the function. Such uses > aren't likely to make the reviewer go look at the function declaration when > the function name is pretty "innocent". Okay, what about domid_to_knownalive_domain()? Or knownalive_domain_from_domid()? Juergen
Hi Juergen, On 12/09/2022 09:33, Juergen Gross wrote: > On 12.09.22 10:31, Jan Beulich wrote: >> On 12.09.2022 10:23, Juergen Gross wrote: >>> 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@suse.com> >>>> >>>> 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. >> >> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" >> aspect, >> yet that's relevant to see when reviewing new uses of the function. >> Such uses >> aren't likely to make the reviewer go look at the function declaration >> when >> the function name is pretty "innocent". > > Okay, what about domid_to_knownalive_domain()? > > Or knownalive_domain_from_domid()? FWIW, I am fine with either. However, please don't replace 'to' with '2' if you need a internal helper. Just suffixed with 'locked' to make clear this is a version where the caller should take the lock. Cheers, -- Julien Grall
On 12.09.22 10:36, Julien Grall wrote: > Hi Juergen, > > On 12/09/2022 09:33, Juergen Gross wrote: >> On 12.09.22 10:31, Jan Beulich wrote: >>> On 12.09.2022 10:23, Juergen Gross wrote: >>>> 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@suse.com> >>>>> >>>>> 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. >>> >>> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" aspect, >>> yet that's relevant to see when reviewing new uses of the function. Such uses >>> aren't likely to make the reviewer go look at the function declaration when >>> the function name is pretty "innocent". >> >> Okay, what about domid_to_knownalive_domain()? >> >> Or knownalive_domain_from_domid()? > > FWIW, I am fine with either. However, please don't replace 'to' with '2' if you > need a internal helper. Just suffixed with 'locked' to make clear this is a > version where the caller should take the lock. I can just rename it to "domid_to_domain()", now that the official helper will get another name. Juergen
On 12.09.2022 10:36, Julien Grall wrote: > On 12/09/2022 09:33, Juergen Gross wrote: >> On 12.09.22 10:31, Jan Beulich wrote: >>> On 12.09.2022 10:23, Juergen Gross wrote: >>>> 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@suse.com> >>>>> >>>>> 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. >>> >>> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" >>> aspect, >>> yet that's relevant to see when reviewing new uses of the function. >>> Such uses >>> aren't likely to make the reviewer go look at the function declaration >>> when >>> the function name is pretty "innocent". >> >> Okay, what about domid_to_knownalive_domain()? >> >> Or knownalive_domain_from_domid()? > > FWIW, I am fine with either. However, please don't replace 'to' with '2' > if you need a internal helper. Just suffixed with 'locked' to make clear > this is a version where the caller should take the lock. Hmm - personally I dislike "_locked" suffixes on functions. If the "internal helper" aspect is to be made more explicit, then perhaps by way of prefixing a single underscore? Jan
On 12/09/2022 09:42, Jan Beulich wrote:
> On 12.09.2022 10:36, Julien Grall wrote:
>> On 12/09/2022 09:33, Juergen Gross wrote:
>>> On 12.09.22 10:31, Jan Beulich wrote:
>>>> On 12.09.2022 10:23, Juergen Gross wrote:
>>>>> 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@suse.com>
>>>>>>
>>>>>> 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.
>>>>
>>>> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive"
>>>> aspect,
>>>> yet that's relevant to see when reviewing new uses of the function.
>>>> Such uses
>>>> aren't likely to make the reviewer go look at the function declaration
>>>> when
>>>> the function name is pretty "innocent".
>>>
>>> Okay, what about domid_to_knownalive_domain()?
>>>
>>> Or knownalive_domain_from_domid()?
>>
>> FWIW, I am fine with either. However, please don't replace 'to' with '2'
>> if you need a internal helper. Just suffixed with 'locked' to make clear
>> this is a version where the caller should take the lock.
>
> Hmm - personally I dislike "_locked" suffixes on functions. If the
> "internal helper" aspect is to be made more explicit, then perhaps
> by way of prefixing a single underscore?
I am OK with that. Although, from previous discussion, I would have
expected that explicit name would have been your preference ("locked" is
more explicit than "_").
Cheers,
--
Julien Grall
On 12.09.2022 10:33, Juergen Gross wrote: > On 12.09.22 10:31, Jan Beulich wrote: >> On 12.09.2022 10:23, Juergen Gross wrote: >>> 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@suse.com> >>>> >>>> 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. >> >> Hmm, agreed. But domid_to_domain() isn't expressing the "known alive" aspect, >> yet that's relevant to see when reviewing new uses of the function. Such uses >> aren't likely to make the reviewer go look at the function declaration when >> the function name is pretty "innocent". > > Okay, what about domid_to_knownalive_domain()? > > Or knownalive_domain_from_domid()? Either would be fine with me. Jan
© 2016 - 2026 Red Hat, Inc.