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

[Juergen Gross <jgross@xxxxxxxx>]

On 15.08.22 14:00, Jan Beulich wrote:
> 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?
Correct.

> 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.
Okay, will add something like:

"Between schedule_cpu_rm_alloc() and the real cpu removal action the relevant
 contents of struct sched_resource can't change, as the cpu in question is
 locked against any other movement to or from cpupools, and the data copied
 by schedule_cpu_rm_alloc() is cpupool specific."

Is that okay?


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature