| Message ID | 20160318190601.8117.67010.stgit@Solace.station (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 18/03/16 19:06, Dario Faggioli wrote: > like what's there already in both Credit1 and RTDS. In > fact, when playing with affinity, a lot of cpumask > manipulation is necessary, inside of various functions. > > To avoid having a lot of cpumask_var_t on the stack, > this patch introduces a global scratch area. > > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> > --- > Cc: George Dunlap <george.dunlap@eu.citrix.com> > --- I would suggest instead going with static DEFINE_PER_CPU(cpumask_t, csched2_cpumask_scratch); Functions which want to use it can use cpumask_t *scratch = &this_cpu(csched2_cpumask_scratch); This avoids all this opencoded allocation/refcounting, the chance that starting a scheduler would fail for memory reasons, and one extra cpumask in the per-cpu data area won't break the bank. ~Andrew
On 18/03/16 19:27, Andrew Cooper wrote: > On 18/03/16 19:06, Dario Faggioli wrote: >> like what's there already in both Credit1 and RTDS. In >> fact, when playing with affinity, a lot of cpumask >> manipulation is necessary, inside of various functions. >> >> To avoid having a lot of cpumask_var_t on the stack, >> this patch introduces a global scratch area. >> >> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> >> --- >> Cc: George Dunlap <george.dunlap@eu.citrix.com> >> --- > > I would suggest instead going with > > static DEFINE_PER_CPU(cpumask_t, csched2_cpumask_scratch); > > Functions which want to use it can use > > cpumask_t *scratch = &this_cpu(csched2_cpumask_scratch); > > > This avoids all this opencoded allocation/refcounting, the chance that > starting a scheduler would fail for memory reasons, and one extra > cpumask in the per-cpu data area won't break the bank. Going a bit further, since (according to the changelog) both credit1 and rtds also do this, would it make sense to have schedule.c define these, and allow any of the schedulers to use them? (Assuming that both credit1 and rtds allocate exactly one mask per cpu.) -George
On 24/03/16 12:44, George Dunlap wrote: > On 18/03/16 19:27, Andrew Cooper wrote: >> On 18/03/16 19:06, Dario Faggioli wrote: >>> like what's there already in both Credit1 and RTDS. In >>> fact, when playing with affinity, a lot of cpumask >>> manipulation is necessary, inside of various functions. >>> >>> To avoid having a lot of cpumask_var_t on the stack, >>> this patch introduces a global scratch area. >>> >>> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> >>> --- >>> Cc: George Dunlap <george.dunlap@eu.citrix.com> >>> --- >> I would suggest instead going with >> >> static DEFINE_PER_CPU(cpumask_t, csched2_cpumask_scratch); >> >> Functions which want to use it can use >> >> cpumask_t *scratch = &this_cpu(csched2_cpumask_scratch); >> >> >> This avoids all this opencoded allocation/refcounting, the chance that >> starting a scheduler would fail for memory reasons, and one extra >> cpumask in the per-cpu data area won't break the bank. > Going a bit further, since (according to the changelog) both credit1 and > rtds also do this, would it make sense to have schedule.c define these, > and allow any of the schedulers to use them? > > (Assuming that both credit1 and rtds allocate exactly one mask per cpu.) If more than one scheduler needs scratch space, then yes. That would be better than having one scratch per scheduler per cpu. After all, we have things like keyhandler_scratch, for a similar reason. ~Andrew
On Thu, 2016-03-24 at 12:44 +0000, George Dunlap wrote: > On 18/03/16 19:27, Andrew Cooper wrote: > > This avoids all this opencoded allocation/refcounting, the chance > > that > > starting a scheduler would fail for memory reasons, and one extra > > cpumask in the per-cpu data area won't break the bank. > Going a bit further, since (according to the changelog) both credit1 > and > rtds also do this, would it make sense to have schedule.c define > these, > and allow any of the schedulers to use them? > Yeah, well, I guess it could. > (Assuming that both credit1 and rtds allocate exactly one mask per > cpu.) > They do. The only difference between credit1 and the other two is that credit1 already has a per-cpu private scheduler structure (csched_pcpu, where also the runqueue is), and the mask has been put there, so it's handled a little bit differently. But it should be no problem turning it into using a mask from schedule.c, in the same way that RTDS and Credit2 will do. I like the idea, and I'm up for it for v2. Thanks and Regards, Dario
diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c index 07b8c67..a650216 100644 --- a/xen/common/sched_credit2.c +++ b/xen/common/sched_credit2.c @@ -238,6 +238,23 @@ static void parse_credit2_runqueue(const char *s) custom_param("credit2_runqueue", parse_credit2_runqueue); /* + * Scratch space, useful to avoid having too many cpumask_var_t on the stack. + * + * We want to only allocate the array the first time an instance of this + * scheduler is used, and avoid reallocating it (e.g., when more instances + * are activated inside new cpupools) or leaking it (e.g., when the last + * instance is de-inited). + * + * Counting the number of active Credit2 instances is all we need, and it + * does not even have to happen via atomic_t operations, as the counter + * only changes during during boot, or under the cpupool_lock. + */ +static cpumask_t **_cpumask_scratch; +#define cpumask_scratch _cpumask_scratch[smp_processor_id()] + +static unsigned int nr_csched2_ops; + +/* * Per-runqueue data */ struct csched2_runqueue_data { @@ -2166,6 +2183,15 @@ csched2_switch_sched(struct scheduler *new_ops, unsigned int cpu, spin_unlock_irq(&prv->lock); } +static void * +csched2_alloc_pdata(const struct scheduler *ops, int cpu) +{ + if ( !zalloc_cpumask_var(&_cpumask_scratch[cpu]) ) + return ERR_PTR(-ENOMEM); + + return NULL; +} + static void csched2_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) { @@ -2205,6 +2231,9 @@ csched2_free_pdata(const struct scheduler *ops, void *pcpu, int cpu) spin_unlock_irqrestore(&prv->lock, flags); + free_cpumask_var(_cpumask_scratch[cpu]); + _cpumask_scratch[cpu] = NULL; + return; } @@ -2239,6 +2268,19 @@ csched2_init(struct scheduler *ops) if ( prv == NULL ) return -ENOMEM; ops->sched_data = prv; + + ASSERT( _cpumask_scratch == NULL || nr_csched2_ops > 0 ); + if ( !_cpumask_scratch ) + { + _cpumask_scratch = xmalloc_array(cpumask_var_t, nr_cpu_ids); + if ( !_cpumask_scratch ) + { + xfree(prv); + return -ENOMEM; + } + } + nr_csched2_ops++; + spin_lock_init(&prv->lock); INIT_LIST_HEAD(&prv->sdom); @@ -2259,6 +2301,13 @@ csched2_deinit(struct scheduler *ops) { struct csched2_private *prv; + ASSERT( _cpumask_scratch && nr_csched2_ops > 0 ); + if ( (--nr_csched2_ops) == 0 ) + { + xfree(_cpumask_scratch); + _cpumask_scratch = NULL; + } + prv = CSCHED2_PRIV(ops); ops->sched_data = NULL; xfree(prv); @@ -2293,6 +2342,7 @@ static const struct scheduler sched_credit2_def = { .alloc_vdata = csched2_alloc_vdata, .free_vdata = csched2_free_vdata, .init_pdata = csched2_init_pdata, + .alloc_pdata = csched2_alloc_pdata, .free_pdata = csched2_free_pdata, .switch_sched = csched2_switch_sched, .alloc_domdata = csched2_alloc_domdata,
like what's there already in both Credit1 and RTDS. In fact, when playing with affinity, a lot of cpumask manipulation is necessary, inside of various functions. To avoid having a lot of cpumask_var_t on the stack, this patch introduces a global scratch area. Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com> --- Cc: George Dunlap <george.dunlap@eu.citrix.com> --- xen/common/sched_credit2.c | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)