linux-kernel - Re: [PATCH 7/9] PM: Add suspend blocking work.

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: <y2ud6200be21004231549q8e52c9c0z441d82b5a15e177d@mail.gmail.com>
Date:	Fri, 23 Apr 2010 15:49:56 -0700
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Oleg Nesterov <oleg@...hat.com>
Cc:	Tejun Heo <tj@...nel.org>, linux-pm@...ts.linux-foundation.org,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Dmitri Vorobiev <dmitri.vorobiev@...ial.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...e.hu>, Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 7/9] PM: Add suspend blocking work.

On Fri, Apr 23, 2010 at 5:20 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> On 04/23, Tejun Heo wrote:
>>
>> On 04/23/2010 03:08 AM, Arve Hjønnevåg wrote:
>> > Allow work to be queued that will block suspend while it is pending
>> > or executing. To get the same functionality in the calling code often
>> > requires a separate suspend_blocker for pending and executing work, or
>> > additional state and locking.
>>
>> Hmm... I think this can be implemented as pure wrapper around
>> workqueue instead of injecting a flag and code into workqueue core.
>> Adding @fn field to suspend_blocking_work struct and using a custom
>> work function to call it and then invoke suspend_unblock() should be
>> enough, right?  Oh, dedicated queue functions will be needed too.  I
>> don't think it's wise to meddle with workqueue core code for this.
>
> Completely agreed. The patch adds very "strange" hacks into workqueue
> code to solve the very specific problems.
>

I want the suspend blocker active when the work is pending or running.
I did not see a way to do this on top of the workqueue api without
adding additional locking.

>
> Besides, the patch doesn't look right. suspend_unblock() can be called
> twice if you use cancel_work(). Perhaps this is not a problem, I dunno.

Calling suspend_unblock() twice is not a problem as long as
"unblocked" is the expected final state.

> WORK_STRUCT_SUSPEND_BLOCKING needs to ensure that cpu_workqueue_struct
> has a proper alignment.

OK.

> The unblock code in run_workqueue() is racy,
> it can unblock after the work was queued on another CPU, cwq->lock can't
> help.

If the work is both queued and starts running on another workqueue
between "get_wq_data(work) == cwq" and "!work_pending(work)", then
suspend_unblock will be called when it shouldn't. It should work fine
if I change to it check pending first though, since it cannot move
back to the current workqueue without locking cwq->lock first.

Or are you talking about the race when the callback is running on
multiple (cpu) workqueues at the same time. In that case the suspend
blocker is released when the callback returns from the last workqueue
is was queued on, not when all the callbacks have returned. On that
note, is it ever safe to use flush_work and cancel_work_sync for work
queues on anything other than a single single threaded workqueue?

-- 
Arve Hjønnevåg
--
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/