Xen-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Juergen Gross <jgross@suse.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Tim Deegan <tim@xen.org>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
	George Dunlap <george.dunlap@eu.citrix.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Ian Jackson <ian.jackson@eu.citrix.com>,
	Robert VanVossen <robert.vanvossen@dornerworks.com>,
	Dario Faggioli <dfaggioli@suse.com>,
	Julien Grall <julien.grall@arm.com>,
	Josh Whitehead <josh.whitehead@dornerworks.com>,
	Meng Xu <mengxu@cis.upenn.edu>,
	xen-devel@lists.xenproject.org
Subject: Re: [Xen-devel] [PATCH v2 21/48] xen/sched: use sched_resource cpu instead smp_processor_id in schedulers
Date: Thu, 12 Sep 2019 11:34:23 +0200
Message-ID: <13f2cf63-2b61-07fa-f43d-044c61601bca@suse.com> (raw)
In-Reply-To: <18ec96a1-b3c3-adbf-4b0e-1fcb7185e046@suse.com>

On 09.09.19 16:17, Jan Beulich wrote:
> On 09.08.2019 16:58, Juergen Gross wrote:
>> Especially in the do_schedule() functions of the different schedulers
>> using smp_processor_id() for the local cpu number is correct only if
>> the sched_unit is a single vcpu. As soon as larger sched_units are
>> used most uses should be replaced by the cpu number of the local
>> sched_resource instead.
> 
> I have to admit that I don't follow this argument, not the least because
> (as I think I had indicated before) it is unclear to me what _the_ (i.e.
> single) CPU for a sched unit is. I've gone back to patches 4 and 7
> without finding what the conceptual model behind this is intended to be.
> Besides an explanation I think one or both of those two patches also
> want to be revisited wrt the use of the name "processor" for the
> respective field.

Fair point. Naming it "master_cpu" in struct sched_resource and when
referencing it seems to be a good idea.

> 
>> --- a/xen/common/sched_credit.c
>> +++ b/xen/common/sched_credit.c
>> @@ -1684,7 +1684,7 @@ csched_load_balance(struct csched_private *prv, int cpu,
>>       int peer_cpu, first_cpu, peer_node, bstep;
>>       int node = cpu_to_node(cpu);
>>   
>> -    BUG_ON( cpu != sched_unit_cpu(snext->unit) );
>> +    BUG_ON( sched_get_resource_cpu(cpu) != sched_unit_cpu(snext->unit) );
> 
> In cases like this one, would you mind dropping the stray blanks
> immediately inside the parentheses?

Will do.

> 
>> @@ -1825,8 +1825,9 @@ static struct task_slice
>>   csched_schedule(
>>       const struct scheduler *ops, s_time_t now, bool_t tasklet_work_scheduled)
>>   {
>> -    const int cpu = smp_processor_id();
>> -    struct list_head * const runq = RUNQ(cpu);
>> +    const unsigned int cpu = smp_processor_id();
>> +    const unsigned int sched_cpu = sched_get_resource_cpu(cpu);
>> +    struct list_head * const runq = RUNQ(sched_cpu);
> 
> By retaining a local variable named "cpu" you make it close to
> impossible to notice, during a re-base, an addition to the
> function still referencing a variable of this name. Similarly
> review is being made harder because one needs to go hunt all
> the remaining uses of "cpu". For example there a trace entry
> being generated, and it's not obvious to me whether this wouldn't
> better also used sched_cpu.

Okayy, I'll rename "cpu" to "my_cpu".

I used cpu in the trace entry on purpose, as it might be interesting on
which cpu the entry has been produced.

> 
>> @@ -1967,7 +1968,7 @@ csched_schedule(
>>       if ( snext->pri > CSCHED_PRI_TS_OVER )
>>           __runq_remove(snext);
>>       else
>> -        snext = csched_load_balance(prv, cpu, snext, &ret.migrated);
>> +        snext = csched_load_balance(prv, sched_cpu, snext, &ret.migrated);
> 
> And in a case like this one I wonder whether passing a "sort of
> CPU" isn't sufficiently confusing, compared to e.g. simply
> passing the corresponding unit.

I guess you mean sched_resource.

I don't think changing the parameter type is a good idea. We need both
(resource and cpu number) on caller and callee side, but the main
object csched_load_balance() is working on is the cpu number.

> 
>> @@ -1975,12 +1976,12 @@ csched_schedule(
>>        */
>>       if ( !tasklet_work_scheduled && snext->pri == CSCHED_PRI_IDLE )
>>       {
>> -        if ( !cpumask_test_cpu(cpu, prv->idlers) )
>> -            cpumask_set_cpu(cpu, prv->idlers);
>> +        if ( !cpumask_test_cpu(sched_cpu, prv->idlers) )
>> +            cpumask_set_cpu(sched_cpu, prv->idlers);
>>       }
>> -    else if ( cpumask_test_cpu(cpu, prv->idlers) )
>> +    else if ( cpumask_test_cpu(sched_cpu, prv->idlers) )
>>       {
>> -        cpumask_clear_cpu(cpu, prv->idlers);
>> +        cpumask_clear_cpu(sched_cpu, prv->idlers);
>>       }
> 
> And this looks to be a pretty gross abuse of CPU masks then.
> (Nevertheless I can see that using a CPU as a vehicle here is
> helpful to limit the scope of the already long series, but I
> think it needs to be made much more apparent what is meant.)

I don't think it is an abuse. Think of it as a cpumask where only
the bits related to the resource's master_cpus can be set.

> 
>> --- a/xen/common/schedule.c
>> +++ b/xen/common/schedule.c
>> @@ -112,7 +112,7 @@ static struct task_slice sched_idle_schedule(
>>       const unsigned int cpu = smp_processor_id();
>>       struct task_slice ret = { .time = -1 };
>>   
>> -    ret.task = sched_idle_unit(cpu);
>> +    ret.task = sched_idle_unit(sched_get_resource_cpu(cpu));
> 
> Shouldn't sched_idle_unit(cpu) == sched_idle_unit(sched_get_resource_cpu(cpu))
> here?

Yes.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

  reply index

Thread overview: 126+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-09 14:57 [Xen-devel] [PATCH v2 00/48] xen: add core scheduling support Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 01/48] xen/sched: use new sched_unit instead of vcpu in scheduler interfaces Juergen Gross
2019-09-02  9:07   ` Jan Beulich
2019-09-09  5:26     ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 02/48] xen/sched: move per-vcpu scheduler private data pointer to sched_unit Juergen Gross
2019-08-23 10:47   ` Dario Faggioli
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 03/48] xen/sched: build a linked list of struct sched_unit Juergen Gross
2019-08-23 10:52   ` Dario Faggioli
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 04/48] xen/sched: introduce struct sched_resource Juergen Gross
2019-08-23 10:54   ` Dario Faggioli
2019-09-04 13:10   ` Jan Beulich
2019-09-09  5:31     ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 05/48] xen/sched: let pick_cpu return a scheduler resource Juergen Gross
2019-09-04 13:34   ` Jan Beulich
2019-09-09  5:43     ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 06/48] xen/sched: switch schedule_data.curr to point at sched_unit Juergen Gross
2019-09-04 13:36   ` Jan Beulich
2019-09-09  5:46     ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 07/48] xen/sched: move per cpu scheduler private data into struct sched_resource Juergen Gross
2019-09-04 13:48   ` Jan Beulich
2019-09-05  7:13     ` Juergen Gross
2019-09-05  7:38       ` Jan Beulich
2019-09-09 13:03         ` Dario Faggioli
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 08/48] xen/sched: switch vcpu_schedule_lock to unit_schedule_lock Juergen Gross
2019-09-04 14:02   ` Jan Beulich
2019-09-04 14:41     ` Juergen Gross
2019-09-04 14:54       ` Jan Beulich
2019-09-04 15:02         ` Juergen Gross
2019-09-11 16:02           ` Dario Faggioli
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 09/48] xen/sched: move some per-vcpu items to struct sched_unit Juergen Gross
2019-09-04 14:16   ` Jan Beulich
2019-09-09  6:39     ` Juergen Gross
2019-09-09  6:55       ` Jan Beulich
2019-09-09  7:05         ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 10/48] xen/sched: add scheduler helpers hiding vcpu Juergen Gross
2019-09-04 14:49   ` Jan Beulich
2019-09-11 13:22     ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 11/48] xen/sched: rename scheduler related perf counters Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 12/48] xen/sched: switch struct task_slice from vcpu to sched_unit Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 13/48] xen/sched: add is_running indicator to struct sched_unit Juergen Gross
2019-09-04 15:06   ` Jan Beulich
2019-09-11 13:44     ` Juergen Gross
2019-09-11 15:06       ` Jan Beulich
2019-09-11 15:32         ` Juergen Gross
2019-08-09 14:57 ` [Xen-devel] [PATCH v2 14/48] xen/sched: make null scheduler vcpu agnostic Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 15/48] xen/sched: make rt " Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 16/48] xen/sched: make credit " Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 17/48] xen/sched: make credit2 " Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 18/48] xen/sched: make arinc653 " Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 19/48] xen: add sched_unit_pause_nosync() and sched_unit_unpause() Juergen Gross
2019-09-09 13:34   ` Jan Beulich
2019-09-11 14:15     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 20/48] xen: let vcpu_create() select processor Juergen Gross
2019-08-23 16:42   ` Julien Grall
2019-09-09 13:38   ` Jan Beulich
2019-09-11 14:22     ` Juergen Gross
2019-09-11 17:20       ` Dario Faggioli
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 21/48] xen/sched: use sched_resource cpu instead smp_processor_id in schedulers Juergen Gross
2019-09-09 14:17   ` Jan Beulich
2019-09-12  9:34     ` Juergen Gross [this message]
2019-09-12 10:04       ` Jan Beulich
2019-09-12 11:03         ` Juergen Gross
2019-09-12 11:17         ` Juergen Gross
2019-09-12 11:46           ` Jan Beulich
2019-09-12 11:53             ` Juergen Gross
2019-09-12 12:08               ` Jan Beulich
2019-09-12 12:13                 ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 22/48] xen/sched: switch schedule() from vcpus to sched_units Juergen Gross
2019-09-09 14:35   ` Jan Beulich
2019-09-12 13:44     ` Juergen Gross
2019-09-12 14:34       ` Jan Beulich
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 23/48] xen/sched: switch sched_move_irqs() to take sched_unit as parameter Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 24/48] xen: switch from for_each_vcpu() to for_each_sched_unit() Juergen Gross
2019-09-09 15:14   ` Jan Beulich
2019-09-12 14:02     ` Juergen Gross
2019-09-12 14:40       ` Jan Beulich
2019-09-12 14:47         ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 25/48] xen/sched: add runstate counters to struct sched_unit Juergen Gross
2019-09-09 14:30   ` Jan Beulich
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 26/48] xen/sched: rework and rename vcpu_force_reschedule() Juergen Gross
2019-09-10 14:06   ` Jan Beulich
2019-09-13  9:33     ` Juergen Gross
2019-09-13  9:40       ` Jan Beulich
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 27/48] xen/sched: Change vcpu_migrate_*() to operate on schedule unit Juergen Gross
2019-09-10 15:11   ` Jan Beulich
2019-09-13 12:33     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 28/48] xen/sched: move struct task_slice into struct sched_unit Juergen Gross
2019-09-10 15:18   ` Jan Beulich
2019-09-13 12:56     ` Juergen Gross
2019-09-12  8:13   ` Dario Faggioli
2019-09-12  8:21     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 29/48] xen/sched: add code to sync scheduling of all vcpus of a sched unit Juergen Gross
2019-09-10 15:36   ` Jan Beulich
2019-09-13 13:12     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 30/48] xen/sched: introduce unit_runnable_state() Juergen Gross
2019-09-11 10:30   ` Jan Beulich
2019-09-12 10:22     ` Dario Faggioli
2019-09-13 14:07     ` Juergen Gross
2019-09-13 14:44       ` Jan Beulich
2019-09-13 15:23         ` Juergen Gross
2019-09-12 10:24   ` Dario Faggioli
2019-09-13 14:14     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 31/48] xen/sched: add support for multiple vcpus per sched unit where missing Juergen Gross
2019-09-11 10:43   ` Jan Beulich
2019-09-13 15:01     ` Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 32/48] xen/sched: modify cpupool_domain_cpumask() to be an unit mask Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 33/48] xen/sched: support allocating multiple vcpus into one sched unit Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 34/48] xen/sched: add a percpu resource index Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 35/48] xen/sched: add fall back to idle vcpu when scheduling unit Juergen Gross
2019-09-11 11:33   ` Julien Grall
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 36/48] xen/sched: make vcpu_wake() and vcpu_sleep() core scheduling aware Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 37/48] xen/sched: carve out freeing sched_unit memory into dedicated function Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 38/48] xen/sched: move per-cpu variable scheduler to struct sched_resource Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 39/48] xen/sched: move per-cpu variable cpupool " Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 40/48] xen/sched: reject switching smt on/off with core scheduling active Juergen Gross
2019-09-10 15:47   ` Jan Beulich
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 41/48] xen/sched: prepare per-cpupool scheduling granularity Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 42/48] xen/sched: split schedule_cpu_switch() Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 43/48] xen/sched: protect scheduling resource via rcu Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 44/48] xen/sched: support multiple cpus per scheduling resource Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 45/48] xen/sched: support differing granularity in schedule_cpu_[add/rm]() Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 46/48] xen/sched: support core scheduling for moving cpus to/from cpupools Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 47/48] xen/sched: disable scheduling when entering ACPI deep sleep states Juergen Gross
2019-08-09 14:58 ` [Xen-devel] [PATCH v2 48/48] xen/sched: add scheduling granularity enum Juergen Gross
2019-08-15 10:17 ` [Xen-devel] [PATCH v2 00/48] xen: add core scheduling support Sergey Dyasli
2019-09-05  6:22   ` Juergen Gross

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=13f2cf63-2b61-07fa-f43d-044c61601bca@suse.com \
    --to=jgross@suse.com \
    --cc=andrew.cooper3@citrix.com \
    --cc=dfaggioli@suse.com \
    --cc=george.dunlap@eu.citrix.com \
    --cc=ian.jackson@eu.citrix.com \
    --cc=jbeulich@suse.com \
    --cc=josh.whitehead@dornerworks.com \
    --cc=julien.grall@arm.com \
    --cc=konrad.wilk@oracle.com \
    --cc=mengxu@cis.upenn.edu \
    --cc=robert.vanvossen@dornerworks.com \
    --cc=sstabellini@kernel.org \
    --cc=tim@xen.org \
    --cc=wl@xen.org \
    --cc=xen-devel@lists.xenproject.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Xen-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/xen-devel/0 xen-devel/git/0.git
	git clone --mirror https://lore.kernel.org/xen-devel/1 xen-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 xen-devel xen-devel/ https://lore.kernel.org/xen-devel \
		xen-devel@lists.xenproject.org xen-devel@lists.xen.org
	public-inbox-index xen-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.xenproject.lists.xen-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git