linux-kernel - Re: [RFC patch 5/5] infiniband: ehca: Use hotplug thread infrastructure

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: <alpine.LFD.2.02.1206181003240.3086@ionos>
Date:	Mon, 18 Jun 2012 10:08:27 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Rusty Russell <rusty@...tcorp.com.au>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Tejun Heo <tj@...nel.org>
Subject: Re: [RFC patch 5/5] infiniband: ehca: Use hotplug thread
 infrastructure

On Mon, 18 Jun 2012, Rusty Russell wrote:

> On Wed, 13 Jun 2012 11:00:56 -0000, Thomas Gleixner <tglx@...utronix.de> wrote:
> > @@ -662,10 +663,15 @@ static inline int find_next_online_cpu(s
> >  		ehca_dmp(cpu_online_mask, cpumask_size(), "");
> >  
> >  	spin_lock_irqsave(&pool->last_cpu_lock, flags);
> > -	cpu = cpumask_next(pool->last_cpu, cpu_online_mask);
> > -	if (cpu >= nr_cpu_ids)
> > -		cpu = cpumask_first(cpu_online_mask);
> > -	pool->last_cpu = cpu;
> > +	while (1) {
> > +		cpu = cpumask_next(pool->last_cpu, cpu_online_mask);
> > +		if (cpu >= nr_cpu_ids)
> > +			cpu = cpumask_first(cpu_online_mask);
> > +		pool->last_cpu = cpu;
> > +		/* Might be on the way out */
> > +		if (per_cpu_ptr(pool->cpu_comp_tasks, cpu)->active)
> > +			break;
> > +	}
> 
> Heh, isn't this what we used to call a "do while" loop? :)

Yep :)
 
> Your infrastructure is a really weird mix.  On the one hand, it's a set
> of callbacks: setup, cleanup, park, unpark.  Cool.
> 
> On the other hand, instead of a 'run' callback, you've got a thread_fn,
> which has to loop and call smpboot_thread_check_parking().
> 
> If you just had the thread_fn, it'd be trivial to follow program flow.
> If you just had the callbacks, it'd still be pretty easy, though it
> seems like a little too much help.

The reason why I moved stuff into the callbacks and have the state
machine inside of smpboot_check_kthread_parking() is that every user
has to do that. i.e. keep track of the state so you wont
setup/teardown stuff twice.

Now look at that function and copy it into all users of the park
infrastructure. Not pretty. I had it that way and it was fugly as
hell. That's why I came up with the callbacks and the generic state
machine.
 
> As it is, we have Paul doing setup stuff inside his thread_fn:
> 
>      +	trace_rcu_utilization("Start CPU kthread@...ark");
>      +	sp.sched_priority = RCU_KTHREAD_PRIO;
>      +	sched_setscheduler_nocheck(current, SCHED_FIFO, &sp);

I fixed that already :)
 
Thanks,

	tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/