[PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Jan Beulich]

Nothing hypercall-related needs setting up there. Nor do we need to
check whether the idle domain is shutting down - it never will.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop vmtrace_alloc_buffer() part.

--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -475,12 +475,6 @@ struct vcpu *vcpu_create(struct domain *
     v->vcpu_id = vcpu_id;
     v->dirty_cpu = VCPU_CPU_CLEAN;
 
-    rwlock_init(&v->virq_lock);
-
-    tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
-
-    grant_table_init_vcpu(v);
-
     if ( is_idle_domain(d) )
     {
         v->runstate.state = RUNSTATE_running;
@@ -488,6 +482,12 @@ struct vcpu *vcpu_create(struct domain *
     }
     else
     {
+        rwlock_init(&v->virq_lock);
+
+        tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
+
+        grant_table_init_vcpu(v);
+
         v->runstate.state = RUNSTATE_offline;
         v->runstate.state_entry_time = NOW();
         set_bit(_VPF_down, &v->pause_flags);
@@ -516,7 +516,8 @@ struct vcpu *vcpu_create(struct domain *
     }
 
     /* Must be called after making new vcpu visible to for_each_vcpu(). */
-    vcpu_check_shutdown(v);
+    if ( !is_idle_domain(d) )
+        vcpu_check_shutdown(v);
 
     return v;
 

Re: [PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Roger Pau Monné]

On Mon, Feb 16, 2026 at 04:54:30PM +0100, Jan Beulich wrote:
> Nothing hypercall-related needs setting up there. Nor do we need to
> check whether the idle domain is shutting down - it never will.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> v2: Drop vmtrace_alloc_buffer() part.
> 
> --- a/xen/common/domain.c
> +++ b/xen/common/domain.c
> @@ -475,12 +475,6 @@ struct vcpu *vcpu_create(struct domain *
>      v->vcpu_id = vcpu_id;
>      v->dirty_cpu = VCPU_CPU_CLEAN;
>  
> -    rwlock_init(&v->virq_lock);
> -
> -    tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
> -
> -    grant_table_init_vcpu(v);
> -
>      if ( is_idle_domain(d) )
>      {
>          v->runstate.state = RUNSTATE_running;
> @@ -488,6 +482,12 @@ struct vcpu *vcpu_create(struct domain *
>      }
>      else
>      {
> +        rwlock_init(&v->virq_lock);
> +
> +        tasklet_init(&v->continue_hypercall_tasklet, NULL, NULL);
> +
> +        grant_table_init_vcpu(v);
> +
>          v->runstate.state = RUNSTATE_offline;
>          v->runstate.state_entry_time = NOW();
>          set_bit(_VPF_down, &v->pause_flags);
> @@ -516,7 +516,8 @@ struct vcpu *vcpu_create(struct domain *
>      }
>  
>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
> -    vcpu_check_shutdown(v);
> +    if ( !is_idle_domain(d) )
> +        vcpu_check_shutdown(v);

I would possibly leave this as-is.  I agree that the idle domain will
never shut down, but it's possibly best to needlessly call into
vcpu_check_shutdown() for the idle domain rather than adding the extra
conditional for the common case?

My Ack stands regardless.

Thanks, Roger.

Re: [PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Jan Beulich]

On 17.02.2026 12:04, Roger Pau Monné wrote:
> On Mon, Feb 16, 2026 at 04:54:30PM +0100, Jan Beulich wrote:
>> Nothing hypercall-related needs setting up there. Nor do we need to
>> check whether the idle domain is shutting down - it never will.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks.

>> @@ -516,7 +516,8 @@ struct vcpu *vcpu_create(struct domain *
>>      }
>>  
>>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
>> -    vcpu_check_shutdown(v);
>> +    if ( !is_idle_domain(d) )
>> +        vcpu_check_shutdown(v);
> 
> I would possibly leave this as-is.  I agree that the idle domain will
> never shut down, but it's possibly best to needlessly call into
> vcpu_check_shutdown() for the idle domain rather than adding the extra
> conditional for the common case?

I'd prefer to keep it conditional: Calling the function for the idle
domain gives a wrong impression, plus it may be the only one where the
shutdown lock is used for that domain. We may want to make lock init
conditional in domain_create() as well (possibly with other things we
needlessly do for idle or more generally system domains).

> My Ack stands regardless.

Thank you.

Jan

Re: [PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Roger Pau Monné]

On Tue, Feb 17, 2026 at 12:17:35PM +0100, Jan Beulich wrote:
> On 17.02.2026 12:04, Roger Pau Monné wrote:
> > On Mon, Feb 16, 2026 at 04:54:30PM +0100, Jan Beulich wrote:
> >> Nothing hypercall-related needs setting up there. Nor do we need to
> >> check whether the idle domain is shutting down - it never will.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> Thanks.
> 
> >> @@ -516,7 +516,8 @@ struct vcpu *vcpu_create(struct domain *
> >>      }
> >>  
> >>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
> >> -    vcpu_check_shutdown(v);
> >> +    if ( !is_idle_domain(d) )
> >> +        vcpu_check_shutdown(v);
> > 
> > I would possibly leave this as-is.  I agree that the idle domain will
> > never shut down, but it's possibly best to needlessly call into
> > vcpu_check_shutdown() for the idle domain rather than adding the extra
> > conditional for the common case?
> 
> I'd prefer to keep it conditional: Calling the function for the idle
> domain gives a wrong impression, plus it may be the only one where the
> shutdown lock is used for that domain. We may want to make lock init
> conditional in domain_create() as well (possibly with other things we
> needlessly do for idle or more generally system domains).

I've been thinking about this, and I'm not sure whether it's the best
approach to avoid initializing locks or lists for the idle
vCPUs/domain.

It's certainly good to avoid initializing stuff that consumes memory
or other resources, but skipping plain initialization (iow: setting of
values) of fields that are in the respective structs seems dangerous
to a certain degree.  If for whatever reason we end up with stray
calls from the idle vCPUs/domain into functions that use those fields
it's likely safer to have them initialized, rather than tripping into
some uninitialized pointer or deadlock trying to acquire and
uninitiated lock.

Thanks, Roger.

Re: [PATCH v2] domain: skip more stuff for idle's vCPU-s in vcpu_create()

[Jan Beulich]

On 17.02.2026 12:46, Roger Pau Monné wrote:
> On Tue, Feb 17, 2026 at 12:17:35PM +0100, Jan Beulich wrote:
>> On 17.02.2026 12:04, Roger Pau Monné wrote:
>>> On Mon, Feb 16, 2026 at 04:54:30PM +0100, Jan Beulich wrote:
>>>> @@ -516,7 +516,8 @@ struct vcpu *vcpu_create(struct domain *
>>>>      }
>>>>  
>>>>      /* Must be called after making new vcpu visible to for_each_vcpu(). */
>>>> -    vcpu_check_shutdown(v);
>>>> +    if ( !is_idle_domain(d) )
>>>> +        vcpu_check_shutdown(v);
>>>
>>> I would possibly leave this as-is.  I agree that the idle domain will
>>> never shut down, but it's possibly best to needlessly call into
>>> vcpu_check_shutdown() for the idle domain rather than adding the extra
>>> conditional for the common case?
>>
>> I'd prefer to keep it conditional: Calling the function for the idle
>> domain gives a wrong impression, plus it may be the only one where the
>> shutdown lock is used for that domain. We may want to make lock init
>> conditional in domain_create() as well (possibly with other things we
>> needlessly do for idle or more generally system domains).
> 
> I've been thinking about this, and I'm not sure whether it's the best
> approach to avoid initializing locks or lists for the idle
> vCPUs/domain.
> 
> It's certainly good to avoid initializing stuff that consumes memory
> or other resources, but skipping plain initialization (iow: setting of
> values) of fields that are in the respective structs seems dangerous
> to a certain degree.  If for whatever reason we end up with stray
> calls from the idle vCPUs/domain into functions that use those fields
> it's likely safer to have them initialized, rather than tripping into
> some uninitialized pointer or deadlock trying to acquire and
> uninitiated lock.

Otoh without doing so it's pretty unlikely that we would spot such stray
calls. Which better would be avoided imo.

Jan