linux-kernel - Re: workqueue_set_max

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: <20111215183537.GA32002@google.com>
Date:	Thu, 15 Dec 2011 10:35:37 -0800
From:	Tejun Heo <tj@...nel.org>
To:	Johannes Berg <johannes@...solutions.net>
Cc:	LKML <linux-kernel@...r.kernel.org>
Subject: Re: workqueue_set_max_active(wq, 0)?

Hello, Johannes.

On Thu, Dec 15, 2011 at 04:38:12PM +0100, Johannes Berg wrote:
> --- a/kernel/workqueue.c	2011-12-10 17:32:26.000000000 +0100
> +++ b/kernel/workqueue.c	2011-12-15 16:36:06.000000000 +0100
> @@ -51,6 +51,7 @@ enum {
>  	GCWQ_DISASSOCIATED	= 1 << 2,	/* cpu can't serve workers */
>  	GCWQ_FREEZING		= 1 << 3,	/* freeze in progress */
>  	GCWQ_HIGHPRI_PENDING	= 1 << 4,	/* highpri works on queue */
> +	GCWQ_PAUSING		= 1 << 5,

Hmmm... confused.  Pausing is per-wq, why is this flag on gcwq?
Shouldn't it be on workqueue_struct?

> +void pause_workqueue(struct workqueue_struct *wq)
> +{
> +	unsigned int cpu;
> +
> +	for_each_cwq_cpu(cpu, wq) {
> +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> +		struct global_cwq *gcwq = cwq->gcwq;
> +
> +		spin_lock_irq(&gcwq->lock);
> +
> +		WARN_ON(gcwq->flags & GCWQ_PAUSING);
> +		gcwq->flags |= GCWQ_PAUSING;
> +
> +		cwq->max_active = 0;
> +
> +		spin_unlock_irq(&gcwq->lock);
> +	}
> +
> +	wait_event(wq->waitq, count_active(wq) == 0);
> +}
> +EXPORT_SYMBOL(pause_workqueue);

What if there are multiple callers of this function on the same wq?
Maybe something like wq->pause_depth and also use it from freeze path?

> +void resume_workqueue(struct workqueue_struct *wq)
> +{
> +	unsigned int cpu;
> +
> +	for_each_cwq_cpu(cpu, wq) {
> +		struct cpu_workqueue_struct *cwq = get_cwq(cpu, wq);
> +		struct global_cwq *gcwq = cwq->gcwq;
> +
> +		spin_lock_irq(&gcwq->lock);
> +
> +		WARN_ON(!(gcwq->flags & GCWQ_PAUSING));
> +		gcwq->flags &= ~GCWQ_PAUSING;
> +
> +		cwq->max_active = wq->saved_max_active;
> +
> +		wake_up_worker(gcwq);
> +		spin_unlock_irq(&gcwq->lock);
> +	}
> +}
> +EXPORT_SYMBOL(resume_workqueue);

I don't think wake_up_worker() would be sufficient.
cwq_activate_first_delayed() needs to be called to kick the delayed
work items.

I think it would be great if this can be abstracted out so that both
the freezer and explicit pausing use the same facility.  They aren't
that different after all.

Thanks.

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