linux-kernel - Re: [PATCH] workqueue: don't alloc

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, 22 Jan 2009 11:42:20 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Oleg Nesterov <oleg@...hat.com>
CC:	Ingo Molnar <mingo@...e.hu>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] workqueue: don't alloc_percpu for single workqueue

Oleg Nesterov wrote:
> On 01/21, Lai Jiangshan wrote:
>> allocating memory for every cpu for single workqueue is waste.
> 
> Yes, perhaps this makes sense, we can save a bit of per-cpu memory
> for each single-threaded wq, and the patch looks correct.
> 
>> -static struct cpu_workqueue_struct *
>> -init_cpu_workqueue(struct workqueue_struct *wq, int cpu)
>> +static void init_cpu_workqueue(struct workqueue_struct *wq,
>> +		struct cpu_workqueue_struct *cwq)
>>  {
>> -	struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
>> -
>>  	cwq->wq = wq;
>>  	spin_lock_init(&cwq->lock);
>>  	INIT_LIST_HEAD(&cwq->worklist);
>>  	init_waitqueue_head(&cwq->more_work);
>> -
>> -	return cwq;
>>  }
> 
> Do we really need to change the prototype of init_cpu_workqueue()
> and change then change __create_workqueue_key() accordingly?
> Afaics, the only change in init_cpu_workqueue() we need is
> 
> 	-	struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> 	+	struct cpu_workqueue_struct *cwq = wq_per_cpu(wq, cpu);
> 
> no?

Thanks, it is simpler.

> 
>> @@ -906,6 +907,13 @@ void destroy_workqueue(struct workqueue_struct *wq)
>>  	const struct cpumask *cpu_map = wq_cpu_map(wq);
>>  	int cpu;
>>
>> +	if (is_wq_single_threaded(wq)) {
>> +		cleanup_workqueue_thread(wq->cpu_wq);
>> +		kfree(wq->cpu_wq);
>> +		kfree(wq);
>> +		return;
>> +	}
> 
> again, not sure I understand why this change is needed. Afaics we
> only need to use kfree(wq->cpu_wq) instead of free_percpu() if
> it is single-threaded.
> 

I think this change is needed.
In the single thread case, we don't need
1) cpu_maps_update_begin(). --> require cpu_add_remove_lock
2) remove workqueue from the list. (we did not inserted it)

It is indeed that there is no bad result occurred when we do these
things for single thread. But I think the destroying should not
do things more than the creating.

Thanks, Lai


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