linux-kernel - Re: [RFC PATCH] kernel: sched: Provide a pointer to the valid CPU mask

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170406073832.e7bu4ldpfuq44ui6@linutronix.de>
Date:   Thu, 6 Apr 2017 09:38:33 +0200
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Ingo Molnar <mingo@...nel.org>
Cc:     linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Mike Galbraith <efault@....de>, Ingo Molnar <mingo@...e.hu>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>
Subject: Re: [RFC PATCH] kernel: sched: Provide a pointer to the valid CPU
 mask

On 2017-04-06 08:16:22 [+0200], Ingo Molnar wrote:
> 
> * Sebastian Andrzej Siewior <bigeasy@...utronix.de> wrote:
> 
> > On 2017-04-05 09:39:43 [+0200], Ingo Molnar wrote:
> > > 
> > > So maybe we could add the following facility:
> > > 
> > > 	ptr = sched_migrate_to_cpu_save(cpu);
> > > 
> > > 	...
> > > 
> > > 	sched_migrate_to_cpu_restore(ptr);
> 
> BTW., and I'm sure this has come up before, but why doesn't migrate_disable() use 
> a simple per task flag that the scheduler migration code takes into account?

we could add that. But right now there are two spots which look at the
counter to decide whether or not migration is disabled.

> It should be functionally equivalent to the current solution, and it appears to 
> have a heck of a smaller cross section with the rest of the scheduler.
> 
> I.e.:
> 
> 	static inline void migrate_disable(void)
> 	{
> 		current->migration_disabled++;
> 	}
> 
> 	...
> 
> 	static inline void migrate_enable(void)
> 	{
> 		current->migration_disabled--;
> 	}
> 
> or so? Then add this flag as a condition to can_migrate_task() et al.
> 
> While we generally dislike such flags as they wreck havoc with the scheduler if 
> overused, the cpus_allowed based solution has the exact same effect so it's not 
> like it's a step backwards - and it should also be much faster and less intrusive.

So you are saying that we drop the cpus_ptr + cpus_mask fields again and
instead add a task-flag to ensure that the scheduler does not migrate
the task to another CPU?

> Am I missing some complication?

We do have the counter. We have need to ensure that the CPU is not going
away while we are in a migrate_disable() region since we can be
scheduled out. So the CPU can't go offline until we leave that region.
This version uses get_online_cpus() while in -RT we have something
called "pin_current_cpu()". This is a lightweight version of
get_online_cpus() which should go away…
Right now I have this and I need to test this and complete CPU hop part:

#define migrate_disable()	sched_migrate_to_cpu_save(-1)

int sched_migrate_to_cpu_save(int cpu)
{
       struct task_struct *p = current;

       if (in_atomic()) {
#ifdef CONFIG_SCHED_DEBUG
               p->migrate_disable_atomic++;
               if (cpu >= 0)
                       WARN_ON_ONCE(!cpumask_equal(p->cpus_ptr, cpumask_of(cpu)));
#endif
               return raw_smp_processor_id();
       }
#ifdef CONFIG_SCHED_DEBUG
       WARN_ON_ONCE(p->migrate_disable_atomic);
#endif

       if (p->migrate_disable) {
               p->migrate_disable++;
#ifdef CONFIG_SCHED_DEBUG
               if (cpu >= 0)
                       WARN_ON_ONCE(!cpumask_equal(p->cpus_ptr, cpumask_of(cpu)));
#endif
               return raw_smp_processor_id();
       }

       get_online_cpus();

       preempt_disable();
       p->migrate_disable = 1;

       if (cpu < 0) {
               p->cpus_ptr = &cpumask_of(task_cpu(raw_smp_processor_id()));
       } else {
               if (!cpu_online(cpu)) {
                       preempt_enable();
                       put_online_cpus();
                       WARN(1, "CPU is offline\n");
                       return -ENODEV;
               }
               p->cpus_ptr = &cpumask_of(task_cpu((cpu)));
       }
       t->nr_cpus = 1;
       preempt_enable();

       if (cpumask_equal(p->cpus_ptr, cpumask_of(cpu)))
               return cpu;

       /* move to the correct CPU */
       BUG();
       return raw_smp_processor_id();
}

The task-flag / p->migrate_disable() counter is used in two spots in
       do_set_cpus_allowed();
       __set_cpus_allowed_ptr();

so that a change to the affinity mask does not force a CPU hop.

> Thanks,
> 
> 	Ingo

Sebastian