Re: [PATCH 2/3] xen/sched: carve out memory allocation and freeing from schedule_cpu_rm()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.08.2022 15:27, Juergen Gross wrote:
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -3190,6 +3190,66 @@ out:
>      return ret;
>  }
>  
> +static struct cpu_rm_data *schedule_cpu_rm_alloc(unsigned int cpu)
> +{
> +    struct cpu_rm_data *data;
> +    struct sched_resource *sr;

const?

> +    int idx;

While code is supposedly only being moved, I still question this not
being "unsigned int", the more that sr->granularity is "unsigned int"
as well. (Same then for the retained instance ofthe variable in the
original function.) Of course the loop in the error path then needs
writing differently.

> +    rcu_read_lock(&sched_res_rculock);
> +
> +    sr = get_sched_res(cpu);
> +    data = xzalloc_flex_struct(struct cpu_rm_data, sr, sr->granularity - 1);

Afaict xmalloc_flex_struct() would do here, as you fill all fields.

> +    if ( !data )
> +        goto out;
> +
> +    data->old_ops = sr->scheduler;
> +    data->vpriv_old = idle_vcpu[cpu]->sched_unit->priv;
> +    data->ppriv_old = sr->sched_priv;

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)?

> +    for ( idx = 0; idx < sr->granularity - 1; idx++ )
> +    {
> +        data->sr[idx] = sched_alloc_res();
> +        if ( data->sr[idx] )
> +        {
> +            data->sr[idx]->sched_unit_idle = sched_alloc_unit_mem();
> +            if ( !data->sr[idx]->sched_unit_idle )
> +            {
> +                sched_res_free(&data->sr[idx]->rcu);
> +                data->sr[idx] = NULL;
> +            }
> +        }
> +        if ( !data->sr[idx] )
> +        {
> +            for ( idx--; idx >= 0; idx-- )
> +                sched_res_free(&data->sr[idx]->rcu);
> +            xfree(data);
> +            data = NULL;

XFREE()?

> @@ -3198,53 +3258,22 @@ out:
>   */
>  int schedule_cpu_rm(unsigned int cpu)
>  {
> -    void *ppriv_old, *vpriv_old;
> -    struct sched_resource *sr, **sr_new = NULL;
> +    struct sched_resource *sr;
> +    struct cpu_rm_data *data;
>      struct sched_unit *unit;
> -    struct scheduler *old_ops;
>      spinlock_t *old_lock;
>      unsigned long flags;
> -    int idx, ret = -ENOMEM;
> +    int idx = 0;
>      unsigned int cpu_iter;
>  
> +    data = schedule_cpu_rm_alloc(cpu);
> +    if ( !data )
> +        return -ENOMEM;
> +
>      rcu_read_lock(&sched_res_rculock);
>  
>      sr = get_sched_res(cpu);
> -    old_ops = sr->scheduler;
>  
> -    if ( sr->granularity > 1 )
> -    {

This conditional is lost afaict, resulting in potentially wrong behavior
in the new helper. Considering its purpose I expect there's a guarantee
that the field's value can never be zero, but then I guess an ASSERT()
would be nice next to the potentially problematic uses in the helper.

> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -598,6 +598,14 @@ struct affinity_masks {
>      cpumask_var_t soft;
>  };
>  
> +/* Memory allocation related data for schedule_cpu_rm(). */
> +struct cpu_rm_data {
> +    struct scheduler *old_ops;

const?

Jan