linux-kernel - Re: [PATCH 04/12] workqueue: simplify is_chained

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: <50657342.6000008@cn.fujitsu.com>
Date:	Fri, 28 Sep 2012 17:52:02 +0800
From:	Lai Jiangshan <laijs@...fujitsu.com>
To:	Tejun Heo <tj@...nel.org>
CC:	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 04/12] workqueue: simplify is_chained_work()

On 09/27/2012 02:28 AM, Tejun Heo wrote:
> On Thu, Sep 27, 2012 at 01:20:35AM +0800, Lai Jiangshan wrote:
>> is_chained_work() is too complicated. we can simply found out
>> whether current task is worker by PF_WQ_WORKER or wq->rescuer.
>>
>> Signed-off-by: Lai Jiangshan <laijs@...fujitsu.com>
>> ---
>>  kernel/workqueue.c |   36 ++++++++++++------------------------
>>  1 files changed, 12 insertions(+), 24 deletions(-)
>>
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> index e41c562..c718b94 100644
>> --- a/kernel/workqueue.c
>> +++ b/kernel/workqueue.c
>> @@ -1182,34 +1182,22 @@ static void insert_work(struct cpu_workqueue_struct *cwq,
>>  
>>  /*
>>   * Test whether @work is being queued from another work executing on the
>> - * same workqueue.  This is rather expensive and should only be used from
>> - * cold paths.
>> + * same workqueue.
>>   */
>>  static bool is_chained_work(struct workqueue_struct *wq)
>>  {
>> -	unsigned long flags;
>> -	unsigned int cpu;
>> +	struct worker *worker = NULL;
>>  
>> -	for_each_gcwq_cpu(cpu) {
>> -		struct global_cwq *gcwq = get_gcwq(cpu);
>> -		struct worker *worker;
>> -		struct hlist_node *pos;
>> -		int i;
>> +	if (wq->rescuer && current == wq->rescuer->task) /* rescuer_thread() */
>> +		worker = wq->rescuer;
>> +	else if (current->flags & PF_WQ_WORKER) /* worker_thread() */
>> +		worker = kthread_data(current);
>>  
>> -		spin_lock_irqsave(&gcwq->lock, flags);
>> -		for_each_busy_worker(worker, i, pos, gcwq) {
>> -			if (worker->task != current)
>> -				continue;
>> -			spin_unlock_irqrestore(&gcwq->lock, flags);
>> -			/*
>> -			 * I'm @worker, no locking necessary.  See if @work
>> -			 * is headed to the same workqueue.
>> -			 */
>> -			return worker->current_cwq->wq == wq;
>> -		}
>> -		spin_unlock_irqrestore(&gcwq->lock, flags);
>> -	}
>> -	return false;
>> +	/*
>> +	 * I'm @worker, no locking necessary.  See if @work
>> +	 * is headed to the same workqueue.
>> +	 */
>> +	return worker && worker->current_cwq->wq == wq;

	if current is a worker and ...

> 
> How about,
> 
> 	if (wq->rescuer && current == wq->rescuer->task)
> 		worker = wq->rescuer;
> 	else if (current->flags & PF_WQ_WORKER)
> 		worker = kthread_data(current);
> 	else
> 		return NULL;
> 
> 	return worker->curent_cwq->wq == wq;
> 

Hi, Tejun

Your code is good, but I don't think I need to resend(and use your code).

Main reason: I think the readability of your is the same as mine,
and your add two lines.

Tiny reason: my code uses only one return. (I don't always keep one return,
but I try to keep it if it is still clean)

Is there any other reason to change it?

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/