linux-kernel - Re: Async suspend-resume patch w/ completions (was: Re: Async suspend-resume patch w/ rwsems)

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]

Date:	Thu, 10 Dec 2009 20:40:10 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
Cc:	Alan Stern <stern@...land.harvard.edu>,
	Zhang Rui <rui.zhang@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
	pm list <linux-pm@...ts.linux-foundation.org>
Subject: Re: Async suspend-resume patch w/ completions (was: Re: Async suspend-resume patch w/ rwsems)

On Thursday 10 December 2009, Linus Torvalds wrote:
> 
> On Thu, 10 Dec 2009, Rafael J. Wysocki wrote:
> > 
> > Completions it is, then.
> 
> What was so hard with the "Try the simple one first" to understand? You 
> had a simpler working patch, why are you making this more complex one 
> without ever having had any problems with the simpler one?

OK, why don't you just say you won't merge anything that doesn't use rwsems
(although you said before that completions would be fine with you)?  That would
make things clear, but also it would mean we gave up handling the off-tree
dependencies in general.

> Btw, your 'atomic_set()' with errors is pure voodoo programming. That's 
> not how atomics work. They do SMP-atomic addition etc, the 'atomic_set()' 
> and 'atomic_read()' things are not in any way more atomic than any other 
> access.
>
> They are meant for racy reads (atomic_read()) and for initializations 
> (atomic_set()), and the way you use them that 'atomic' part is entirely 
> pointless, because it really isn't anything different from an 'int', 
> except that it may be very very expensive on some architectures due to 
> hashed spinlocks etc.
> 
> So stop this overdesign thing. Start simple. If you _ever_ see real 
> problems, that's when you add stuff. As it is, any time you add 
> complexity, you just add bugs.

OK, so that need not be atomic.
 
> > +/**
> > + * dpm_synchronize - Wait for PM callbacks of all devices to complete.
> > + */
> > +static void dpm_synchronize(void)
> > +{
> > +	struct device *dev;
> > +
> > +	async_synchronize_full();
> > +
> > +	mutex_lock(&dpm_list_mtx);
> > +	list_for_each_entry(dev, &dpm_list, power.entry)
> > +		INIT_COMPLETION(dev->power.completion);
> > +	mutex_unlock(&dpm_list_mtx);
> > +}
> 
> And this, for example, is pretty disgusting. Not only is that 
> INIT_COMPLETION purely brought on by the whole problem with completions 
> (they are fundamentally one-shot, but you want to use them over and over

Actually, twice.  However, since I don't want to do any async handling in the
_noirq phases any more, I can get rid of this whole function.  Thanks for
pointing that out to me.

Rafael
--
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/