On 15.08.2022 13:55, Juergen Gross wrote:
> On 15.08.22 13:52, Jan Beulich wrote:
>> On 15.08.2022 13:04, Juergen Gross wrote:
>>> --- a/xen/common/sched/core.c
>>> +++ b/xen/common/sched/core.c
>>> @@ -3237,6 +3237,65 @@ out:
>>> return ret;
>>> }
>>>
>>> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
>>> +{
>>> + struct cpu_rm_data *data;
>>> + const struct sched_resource *sr;
>>> + unsigned int idx;
>>> +
>>> + rcu_read_lock(&sched_res_rculock);
>>> +
>>> + sr = get_sched_res(cpu);
>>> + data = xmalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity -
>>> 1);
>>> + if ( !data )
>>> + goto out;
>>> +
>>> + data->old_ops = sr->scheduler;
>>> + data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
>>> + data->ppriv_old = sr->sched_priv;
>>
>> Repeating a v1 comment:
>>
>> "At least from an abstract perspective, doesn't reading fields from
>> sr require the RCU lock to be held continuously (i.e. not dropping
>> it at the end of this function and re-acquiring it in the caller)?"
>>
>> Initially I thought you did respond to this in some way, but when
>> looking for a matching reply I couldn't find one.
>
> Oh, sorry.
>
> The RCU lock is protecting only the sr, not any data pointers in the sr
> are referencing. So it is fine to drop the RCU lock after reading some
> of the fields from the sr and storing it in the cpu_rm_data memory.
Hmm, interesting. "Protecting only the sr" then means what exactly?
Just its allocation, but not its contents?
Plus it's not just the pointers - sr->granularity also would better not
increase in the meantime ... Quite likely there's a reason why that also
cannot happen, yet even then I think a brief code comment might be
helpful here.
Jan
