Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1574357 > unrolled thread

tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

Back to article view | Back to linux.kernel

Contents

  tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Mike Galbraith <efault@gmx.de> - 2017-02-06 05:30 +0100
    Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Ingo Molnar <mingo@kernel.org> - 2017-02-06 11:40 +0100
      Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Mike Galbraith <efault@gmx.de> - 2017-02-06 13:20 +0100
        Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Ingo Molnar <mingo@kernel.org> - 2017-02-06 13:30 +0100
          Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Mike Galbraith <efault@gmx.de> - 2017-02-06 13:50 +0100
          Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Peter Zijlstra <peterz@infradead.org> - 2017-02-06 14:40 +0100
            Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Ingo Molnar <mingo@kernel.org> - 2017-02-06 23:30 +0100
              Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Thomas Gleixner <tglx@linutronix.de> - 2017-02-08 12:10 +0100
                Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Peter Zijlstra <peterz@infradead.org> - 2017-02-08 13:40 +0100
                  Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Ingo Molnar <mingo@kernel.org> - 2017-02-09 08:00 +0100
                    Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Peter Zijlstra <peterz@infradead.org> - 2017-02-09 10:00 +0100
                    Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Thomas Gleixner <tglx@linutronix.de> - 2017-02-09 10:40 +0100
                  Re: tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed() Ingo Molnar <mingo@kernel.org> - 2017-02-09 08:20 +0100

#1574357 — tip: demise of tsk_cpus_allowed() and tsk_nr_cpus_allowed()

Hi Ingo,

Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
they grow more functionality in -rt, which is allegedly slowly but
surely headed toward merge.  I don't suppose they could be left intact?
 I can easily restore them in my local tree, but it seems a bit of a
shame to whack these integration friendly bits.

	-Mike

[toc] | [next] | [standalone]

#1574546

* Mike Galbraith <efault@gmx.de> wrote:

> Hi Ingo,
> 
> Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> they grow more functionality in -rt, which is allegedly slowly but
> surely headed toward merge.  I don't suppose they could be left intact?
>  I can easily restore them in my local tree, but it seems a bit of a
> shame to whack these integration friendly bits.

Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1574611

On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> * Mike Galbraith <efault@gmx.de> wrote:
> 
> > Hi Ingo,
> > 
> > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > they grow more functionality in -rt, which is allegedly slowly but
> > surely headed toward merge.  I don't suppose they could be left intact?
> >  I can easily restore them in my local tree, but it seems a bit of a
> > shame to whack these integration friendly bits.
> 
> Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?

RT extends them to reflect whether migration is disabled or not.

+/* Future-safe accessor for struct task_struct's cpus_allowed. */
+static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
+{
+       if (__migrate_disabled(p))
+               return cpumask_of(task_cpu(p));
+
+       return &p->cpus_allowed;
+}
+
+static inline int tsk_nr_cpus_allowed(struct task_struct *p)
+{
+       if (__migrate_disabled(p))
+               return 1;
+       return p->nr_cpus_allowed;
+}

[toc] | [prev] | [next] | [standalone]

#1574628

* Mike Galbraith <efault@gmx.de> wrote:

> On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > * Mike Galbraith <efault@gmx.de> wrote:
> > 
> > > Hi Ingo,
> > > 
> > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > they grow more functionality in -rt, which is allegedly slowly but
> > > surely headed toward merge.  I don't suppose they could be left intact?
> > >  I can easily restore them in my local tree, but it seems a bit of a
> > > shame to whack these integration friendly bits.
> > 
> > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> 
> RT extends them to reflect whether migration is disabled or not.
> 
> +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> +{
> +       if (__migrate_disabled(p))
> +               return cpumask_of(task_cpu(p));
> +
> +       return &p->cpus_allowed;
> +}
> +
> +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> +{
> +       if (__migrate_disabled(p))
> +               return 1;
> +       return p->nr_cpus_allowed;
> +}

So ... I think the cleaner approach in -rt would be to introduce 
->cpus_allowed_saved, and when disabling/enabling migration then saving the 
current mask there and changing ->cpus_allowed - and then restoring it when 
re-enabling migration.

This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
would be required, AFAICS.

( Some extra care would be required in places that change ->cpus_allowed because 
  they'd now have to be aware of ->cpus_allowed_saved. )

Am I missing something?

	Ingo

[toc] | [prev] | [next] | [standalone]

#1574640

On Mon, 2017-02-06 at 13:29 +0100, Ingo Molnar wrote:
> * Mike Galbraith <efault@gmx.de> wrote:
> 
> > On Mon, 2017-02-06 at 11:31 +0100, Ingo Molnar wrote:
> > > * Mike Galbraith <efault@gmx.de> wrote:
> > > 
> > > > Hi Ingo,
> > > > 
> > > > Doing my ~daily tip merge of -rt, I couldn't help noticing $subject, as
> > > > they grow more functionality in -rt, which is allegedly slowly but
> > > > surely headed toward merge.  I don't suppose they could be left intact?
> > > >  I can easily restore them in my local tree, but it seems a bit of a
> > > > shame to whack these integration friendly bits.
> > > 
> > > Oh, I missed that. How is tsk_cpus_allowed() wrapped in -rt right now?
> > 
> > RT extends them to reflect whether migration is disabled or not.
> > 
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +       if (__migrate_disabled(p))
> > +               return cpumask_of(task_cpu(p));
> > +
> > +       return &p->cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +       if (__migrate_disabled(p))
> > +               return 1;
> > +       return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

I suppose it's a matter of personal preference.  I prefer the above,
looks nice and clean to me.  Hohum, I'll just put them back locally for
the nonce.  My trees are only place holders until official releases
catch up anyway.

	-Mike

[toc] | [prev] | [next] | [standalone]

#1574707

On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > +{
> > +       if (__migrate_disabled(p))
> > +               return cpumask_of(task_cpu(p));
> > +
> > +       return &p->cpus_allowed;
> > +}
> > +
> > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > +{
> > +       if (__migrate_disabled(p))
> > +               return 1;
> > +       return p->nr_cpus_allowed;
> > +}
> 
> So ... I think the cleaner approach in -rt would be to introduce 
> ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> current mask there and changing ->cpus_allowed - and then restoring it when 
> re-enabling migration.
> 
> This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
> would be required, AFAICS.
> 
> ( Some extra care would be required in places that change ->cpus_allowed because 
>   they'd now have to be aware of ->cpus_allowed_saved. )
> 
> Am I missing something?

cpumasks are a pain, the above avoids allocating more of them.

[toc] | [prev] | [next] | [standalone]

#1575206

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Mon, Feb 06, 2017 at 01:29:28PM +0100, Ingo Molnar wrote:
> > > +/* Future-safe accessor for struct task_struct's cpus_allowed. */
> > > +static inline const struct cpumask *tsk_cpus_allowed(struct task_struct *p)
> > > +{
> > > +       if (__migrate_disabled(p))
> > > +               return cpumask_of(task_cpu(p));
> > > +
> > > +       return &p->cpus_allowed;
> > > +}
> > > +
> > > +static inline int tsk_nr_cpus_allowed(struct task_struct *p)
> > > +{
> > > +       if (__migrate_disabled(p))
> > > +               return 1;
> > > +       return p->nr_cpus_allowed;
> > > +}
> > 
> > So ... I think the cleaner approach in -rt would be to introduce 
> > ->cpus_allowed_saved, and when disabling/enabling migration then saving the 
> > current mask there and changing ->cpus_allowed - and then restoring it when 
> > re-enabling migration.
> > 
> > This means ->cpus_allowed could be used by the scheduler directly, no wrappery 
> > would be required, AFAICS.
> > 
> > ( Some extra care would be required in places that change ->cpus_allowed because 
> >   they'd now have to be aware of ->cpus_allowed_saved. )
> > 
> > Am I missing something?
> 
> cpumasks are a pain, the above avoids allocating more of them.

Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
than the wrappery, because as I've noted in the changelog there's a large body of 
upstream code that does not use the wrappers but uses ->cpus_allowed directly:

arch/ia64/kernel/mca.c:	cpumask_set_cpu(cpu, &p->cpus_allowed);
arch/ia64/kernel/salinfo.c:	cpumask_t save_cpus_allowed = current->cpus_allowed;
arch/ia64/kernel/topology.c:	oldmask = current->cpus_allowed;
arch/ia64/sn/kernel/sn2/sn_hwperf.c:			save_allowed = current->cpus_allowed;
arch/mips/include/asm/switch_to.h: * isn't set), we undo the restriction on cpus_allowed.
arch/mips/include/asm/switch_to.h:		prev->cpus_allowed = prev->thread.user_cpus_allowed;	\
arch/mips/kernel/mips-mt-fpaff.c:	cpumask_var_t cpus_allowed, new_mask, effective_mask;
arch/mips/kernel/mips-mt-fpaff.c:	if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
arch/mips/kernel/mips-mt-fpaff.c:		cpuset_cpus_allowed(p, cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:		if (!cpumask_subset(effective_mask, cpus_allowed)) {
arch/mips/kernel/mips-mt-fpaff.c:			 * update. Just reset the cpus_allowed to the
arch/mips/kernel/mips-mt-fpaff.c:			 * cpuset's cpus_allowed
arch/mips/kernel/mips-mt-fpaff.c:			cpumask_copy(new_mask, cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:	free_cpumask_var(cpus_allowed);
arch/mips/kernel/mips-mt-fpaff.c:	cpumask_or(&allowed, &p->thread.user_cpus_allowed, &p->cpus_allowed);
arch/mips/kernel/traps.c:		if (cpumask_intersects(&current->cpus_allowed, &mt_fpu_cpumask)) {
arch/mips/kernel/traps.c:				= current->cpus_allowed;
arch/mips/kernel/traps.c:			cpumask_and(&tmask, &current->cpus_allowed,
arch/powerpc/platforms/cell/spufs/sched.c:	cpumask_copy(&ctx->cpus_allowed, tsk_cpus_allowed(current));
arch/powerpc/platforms/cell/spufs/sched.c:		if (cpumask_intersects(mask, &ctx->cpus_allowed))
arch/powerpc/platforms/cell/spufs/spufs.h:	cpumask_t cpus_allowed;
arch/tile/include/asm/setup.h:	if (!cpumask_equal(&p->cpus_allowed, new_mask)) \
arch/tile/kernel/hardwall.c:	if (cpumask_weight(&p->cpus_allowed) != 1)
arch/tile/kernel/hardwall.c:	BUG_ON(cpumask_first(&p->cpus_allowed) != cpu);
arch/tile/kernel/hardwall.c: * We assume the cpus_allowed, pid, and comm fields are still valid.
arch/tile/kernel/hardwall.c:	if (cpumask_weight(&task->cpus_allowed) != 1) {
arch/tile/kernel/hardwall.c:		       cpumask_weight(&task->cpus_allowed));
drivers/acpi/processor_throttling.c:	cpumask_copy(saved_mask, &current->cpus_allowed);
drivers/cpufreq/ia64-acpi-cpufreq.c:	saved_mask = current->cpus_allowed;
drivers/cpufreq/ia64-acpi-cpufreq.c:	saved_mask = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:	cpus_allowed = current->cpus_allowed;
drivers/cpufreq/loongson2_cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/cpufreq/sh-cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:	cpus_allowed = current->cpus_allowed;
drivers/cpufreq/sh-cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:	cpumask_copy(&cpus_allowed, tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/cpufreq/sparc-us2e-cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us2e-cpufreq.c:	cpumask_copy(&cpus_allowed, tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us2e-cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/cpufreq/sparc-us3-cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us3-cpufreq.c:	cpumask_copy(&cpus_allowed, tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us3-cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/cpufreq/sparc-us3-cpufreq.c:	cpumask_t cpus_allowed;
drivers/cpufreq/sparc-us3-cpufreq.c:	cpumask_copy(&cpus_allowed, tsk_cpus_allowed(current));
drivers/cpufreq/sparc-us3-cpufreq.c:	set_cpus_allowed_ptr(current, &cpus_allowed);
drivers/crypto/n2_core.c:	cpumask_copy(old_allowed, &current->cpus_allowed);
drivers/infiniband/hw/qib/qib_file_ops.c:	const unsigned int weight = cpumask_weight(&current->cpus_allowed);
drivers/infiniband/hw/qib/qib_file_ops.c:		const unsigned int cpu = cpumask_first(&current->cpus_allowed);
drivers/infiniband/hw/qib/qib_file_ops.c:			cpumask_weight(&current->cpus_allowed);
fs/proc/array.c:		   cpumask_pr_args(&task->cpus_allowed));
fs/proc/array.c:		   cpumask_pr_args(&task->cpus_allowed));
include/linux/init_task.h:	.cpus_allowed	= CPU_MASK_ALL,					\
include/linux/sched.h:	cpumask_t cpus_allowed;
include/linux/sched.h:/* Future-safe accessor for struct task_struct's cpus_allowed. */
include/linux/sched.h:#define tsk_cpus_allowed(tsk) (&(tsk)->cpus_allowed)
include/linux/sched.h:#define PF_NO_SETAFFINITY 0x04000000	/* Userland is not allowed to meddle with cpus_allowed */
kernel/sched/core.c:	 * __migrate_task() such that we will not miss enforcing cpus_allowed
kernel/sched/core.c:	cpumask_copy(&p->cpus_allowed, new_mask);
kernel/sched/core.c:	if (cpumask_equal(&p->cpus_allowed, new_mask))
kernel/sched/core.c: * ->cpus_allowed is protected by both rq->lock and p->pi_lock
kernel/sched/core.c: * The caller (fork, wakeup) owns p->pi_lock, ->cpus_allowed is stable.
kernel/sched/core.c:	 * to rely on ttwu() to place the task on a valid ->cpus_allowed
kernel/sched/core.c:	 *  - cpus_allowed can change in the fork path
kernel/sched/core.c:			if (!cpumask_subset(span, &p->cpus_allowed) ||
kernel/sched/core.c:	cpumask_var_t cpus_allowed, new_mask;
kernel/sched/core.c:	if (!alloc_cpumask_var(&cpus_allowed, GFP_KERNEL)) {
kernel/sched/core.c:	cpuset_cpus_allowed(p, cpus_allowed);
kernel/sched/core.c:	cpumask_and(new_mask, in_mask, cpus_allowed);
kernel/sched/core.c:		cpuset_cpus_allowed(p, cpus_allowed);
kernel/sched/core.c:		if (!cpumask_subset(new_mask, cpus_allowed)) {
kernel/sched/core.c:			 * update. Just reset the cpus_allowed to the
kernel/sched/core.c:			 * cpuset's cpus_allowed
kernel/sched/core.c:			cpumask_copy(new_mask, cpus_allowed);
kernel/sched/core.c:	free_cpumask_var(cpus_allowed);
kernel/sched/core.c:	cpumask_and(mask, &p->cpus_allowed, cpu_active_mask);
kernel/sched/core.c:	 * before cpus_allowed may be changed.
kernel/sched/core.c:		 * Rules for changing task_struct::cpus_allowed are holding
kernel/sched/fair.c:	 * 2) cannot be migrated to this CPU due to cpus_allowed, or
kernel/sched/fair.c:	 * isn't true due to cpus_allowed constraints and the like.
kernel/trace/trace_hwlat.c:	if (!cpumask_equal(current_mask, &current->cpus_allowed))
kernel/workqueue.c: * its cpus_allowed.  If @cpu is in @pool's cpumask which didn't have any
kernel/workqueue.c: * online CPU before, cpus_allowed of all its workers should be restored.
net/sunrpc/svc.c: * Set the given thread's cpus_allowed mask so that it

By turning ->cpus_allowed into a pointer we add a small amount of indirection, but 
not in any true scheduler fast-path, and doing this would be preferable to -rt as 
well as it self-maintains this notion in a way that will benefit -rt.

Note that we already pass around cpus_allowed by pointer in a few cases, so the 
indirection cost should be pretty minimal.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1576449

On Mon, 6 Feb 2017, Ingo Molnar wrote:
> * Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > cpumasks are a pain, the above avoids allocating more of them.

Indeed.

> Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
> than the wrappery,

You mean:

struct task_struct {
       cpumask_t	cpus_allowed;
       cpumask_t	*effective_cpus_allowed;
};

and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
Or what do you have in mind?

> because as I've noted in the changelog there's a large body of 
> upstream code that does not use the wrappers but uses ->cpus_allowed directly:

Right and we really should audit those places. I bet that half of them are
just broken and evil hacks.

The wrapper we added is just covering the core scheduler code where the
information really matters for decision, but leaves the other oddball cases
alone.

The extra pointer might be a nicer concept, but it still has the same issue
as the wrapper. How do we enforce that random code accesses the right
thing?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1576510

On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > * Peter Zijlstra <peterz@infradead.org> wrote:
> > >
> > > cpumasks are a pain, the above avoids allocating more of them.
> 
> Indeed.
> 
> > Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
> > than the wrappery,
> 
> You mean:
> 
> struct task_struct {
>        cpumask_t	cpus_allowed;
>        cpumask_t	*effective_cpus_allowed;
> };
> 
> and make the scheduler use effective_cpus_allowed instead of cpus_allowed?
> Or what do you have in mind?

That scheme is weird for nr_cpus_allowed. Not to mention that the
pointer to the integer is larger than the integer itself.

I really prefer the current wrappers, they're trivial and consistent
with one another.

[toc] | [prev] | [next] | [standalone]

#1577340

* Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > > >
> > > > > cpumasks are a pain, the above avoids allocating more of them.
> > > 
> > > Indeed.
> > > 
> > > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
> > > > than the wrappery,
> > > 
> > > You mean:
> > > 
> > > struct task_struct {
> > >        cpumask_t	cpus_allowed;
> > >        cpumask_t	*effective_cpus_allowed;
> > > };
> 
> Yeah. I'd name it a bit differently and constify the pointer to get type 
> safety and to make sure the mask is never modified through the pointer:
> 
> 	struct task_struct {
> 		const cpumask_t		*cpus_ptr;
> 		cpumask_t		cpus_mask;
> 	};
> 
> ( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
>   right? )
> 
> and upstream would essentially just do:
> 
> 	t->cpus_allowed_ptr = &t->cpus_allowed;
> 
> And -rt, when it wants to pin a task, would do:
> 
> 	t->cpus_allowed_ptr = &cpumask_of(task_cpu(p));
> 
> The rules are:
> 
>  - Code that 'uses' ->cpus_allowed would use the pointer.
> 
>  - Code that 'modifies' ->cpus_allowed would use the direct mask.
> 
> The upstream advantages are:
> 
>  - The type separation of modifications from usage.
> 
>  - Removal of wrappery.
> 
>  - Maybe sometime in the future upstream would want to disable migration too ...
> 
> In fact -rt gains something too:
> 
>  - With this scheme we would AFAICS get slightly more optimal code on -rt.
>    (Because there's no __migration_disabled() branching anymore.)
> 
>  - Plus all new code is automatically -rt ready - no need to maintain the wrappery 
>    space. Much less code path forking.
> 
> So as I see it it's win-win for both upstream and for -rt!
> 
> > > and make the scheduler use effective_cpus_allowed instead of cpus_allowed? Or 
> > > what do you have in mind?
> > 
> > That scheme is weird for nr_cpus_allowed. Not to mention that the
> > pointer to the integer is larger than the integer itself.
> 
> So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
> at all: whenever the pointer (or mask) is changed in set_cpus_allowed_common() 
> nr_cpus_allowed is recalculated as well - like today.
> 
> It should be self-maintaining. Am I missing something?

And -rt would do something like this in migration_disable()/enable():
 
	t->cpus_ptr = &cpumask_of(task_cpu(p));
	t->nr_cpus = 1;

	...

	t->cpus_ptr = &t->cpus_mask;
	t->nr_cpus = cpumask_weight(t->cpus_mask);

In addition to that we could cache the weight of the cpumask as an additional 
optimization:

	t->cpus_ptr = &t->cpus_mask;
	t->nr_cpus = t->cpus_mask_weight;

It all looks like a pretty natural construct to me. The migration_disabled() flag 
spreads almost a hundred branches all across the scheduler.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1577383

On Thu, Feb 09, 2017 at 07:57:27AM +0100, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
> 	t->cpus_ptr = &cpumask_of(task_cpu(p));
> 	t->nr_cpus = 1;
> 
> 	...
> 
> 	t->cpus_ptr = &t->cpus_mask;
> 	t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
> 	t->cpus_ptr = &t->cpus_mask;
> 	t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() flag 
> spreads almost a hundred branches all across the scheduler.

Could work I suppose. But please then implement this instead of ripping
out the current thing, because taking out the accessors leaves RT in a
bind without recourse.

[toc] | [prev] | [next] | [standalone]

#1577419

On Thu, 9 Feb 2017, Ingo Molnar wrote:
> And -rt would do something like this in migration_disable()/enable():
>  
> 	t->cpus_ptr = &cpumask_of(task_cpu(p));
> 	t->nr_cpus = 1;
> 
> 	...
> 
> 	t->cpus_ptr = &t->cpus_mask;
> 	t->nr_cpus = cpumask_weight(t->cpus_mask);
> 
> In addition to that we could cache the weight of the cpumask as an additional 
> optimization:
> 
> 	t->cpus_ptr = &t->cpus_mask;
> 	t->nr_cpus = t->cpus_mask_weight;
> 
> It all looks like a pretty natural construct to me. The migration_disabled() flag 
> spreads almost a hundred branches all across the scheduler.

I'm fine with that. Making the pointer const is clever and prevents people
from manipulating the wrong thing. If would be great if you could rework it
that way instead of just ripping all out.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1577351

* Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Feb 08, 2017 at 11:20:19AM +0100, Thomas Gleixner wrote:
> > On Mon, 6 Feb 2017, Ingo Molnar wrote:
> > > * Peter Zijlstra <peterz@infradead.org> wrote:
> > > >
> > > > cpumasks are a pain, the above avoids allocating more of them.
> > 
> > Indeed.
> > 
> > > Yeah, so this could then be done by pointerifying ->cpus_allowed - more robust 
> > > than the wrappery,
> > 
> > You mean:
> > 
> > struct task_struct {
> >        cpumask_t	cpus_allowed;
> >        cpumask_t	*effective_cpus_allowed;
> > };

Yeah. I'd name it a bit differently and constify the pointer to get type 
safety and to make sure the mask is never modified through the pointer:

	struct task_struct {
		const cpumask_t		*cpus_ptr;
		cpumask_t		cpus_mask;
	};

( I'd drop the 'allowed' part, it's obvious enough what task->cpus_mask does, 
  right? )

and upstream would essentially just do:

	t->cpus_allowed_ptr = &t->cpus_allowed;

And -rt, when it wants to pin a task, would do:

	t->cpus_allowed_ptr = &cpumask_of(task_cpu(p));

The rules are:

 - Code that 'uses' ->cpus_allowed would use the pointer.

 - Code that 'modifies' ->cpus_allowed would use the direct mask.

The upstream advantages are:

 - The type separation of modifications from usage.

 - Removal of wrappery.

 - Maybe sometime in the future upstream would want to disable migration too ...

In fact -rt gains something too:

 - With this scheme we would AFAICS get slightly more optimal code on -rt.
   (Because there's no __migration_disabled() branching anymore.)

 - Plus all new code is automatically -rt ready - no need to maintain the wrappery 
   space. Much less code path forking.

So as I see it it's win-win for both upstream and for -rt!

> > and make the scheduler use effective_cpus_allowed instead of cpus_allowed? Or 
> > what do you have in mind?
> 
> That scheme is weird for nr_cpus_allowed. Not to mention that the
> pointer to the integer is larger than the integer itself.

So in the new scheme I don't think nr_cpus_allowed would have to be wrapped
at all: whenever the pointer (or mask) is changed in set_cpus_allowed_common() 
nr_cpus_allowed is recalculated as well - like today.

It should be self-maintaining. Am I missing something?

> I really prefer the current wrappers, they're trivial and consistent
> with one another.

I think it's ugly wrappery and we can do better! ;-)

But of course if I cannot suggest a better alternative then it stands.

Thanks,

	Ingo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web