Hi Ingo,
current_is_keventd() macro checks "current" with the per-cpu
thread of _all_ possible cpus attached to keventd_wq. Can't it just check
against the per-cpu thread of _current_ cpu alone (since the per-cpu workqueue
threads are anyway bound only to their cpus)?
int current_is_keventd(void)
{
struct cpu_workqueue_struct *cwq;
- int cpu;
+ int cpu = smp_processor_id();
BUG_ON(!keventd_wq);
- for_each_cpu(cpu) {
- cwq = keventd_wq->cpu_wq + cpu;
- if (current == cwq->thread)
- return 1;
- }
- return 0;
+ cwq = keventd_wq->cpu_wq + cpu;
+ if (current == cwq->thread)
+ return 1;
+ else
+ return 0;
}
----- Forwarded message from Rusty Russell <[email protected]> -----
Subject: Re: [lhcs-devel] Re: Kthread_create() never returns when called
from worker_thread
From: Rusty Russell <[email protected]>
To: [email protected]
Cc: [email protected],
"Keshavamurthy, Anil S" <[email protected]>,
[email protected], "Raj, Ashok" <[email protected]>,
"Shah, Rajesh" <[email protected]>
X-Mailer: Ximian Evolution 1.4.5
Date: Mon, 08 Mar 2004 11:29:27 +1100
On Wed, 2004-03-03 at 20:08, Srivatsa Vaddagiri wrote:
> On Tue, Mar 02, 2004 at 12:10:08PM +1100, Rusty Russell wrote:
> > /* If we're being called to start the first workqueue, we
> > * can't use keventd. */
> > - if (!keventd_up())
> > + if (!keventd_up() || current_is_keventd())
> > work.func(work.data);
> > else {
> > schedule_work(&work);
>
> I noticed that current_is_keventd() check actually goes and compares
> "current" with all the per-cpu threads attached to keventd() workqueue ..
> Is that really necessary? Can't it compare with the per-cpu thread
> attached to _current_ cpu only?
You'd think so, but it's not my code. I believe you are correct though.
Cheers,
Rusty.
--
Anyone who quotes me in their signature is an idiot -- Rusty Russell
-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
lhcs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/lhcs-devel
----- End forwarded message -----
--
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017
Srivatsa Vaddagiri <[email protected]> wrote:
>
> current_is_keventd() macro checks "current" with the per-cpu
> thread of _all_ possible cpus attached to keventd_wq. Can't it just check
> against the per-cpu thread of _current_ cpu alone (since the per-cpu workqueue
> threads are anyway bound only to their cpus)?
Seems that way, yes.
> int current_is_keventd(void)
> {
> struct cpu_workqueue_struct *cwq;
> - int cpu;
> + int cpu = smp_processor_id();
>
> BUG_ON(!keventd_wq);
>
> - for_each_cpu(cpu) {
> - cwq = keventd_wq->cpu_wq + cpu;
> - if (current == cwq->thread)
> - return 1;
> - }
>
> - return 0;
>
> + cwq = keventd_wq->cpu_wq + cpu;
> + if (current == cwq->thread)
> + return 1;
> + else
> + return 0;
> }
Is racy in the presence of preemption. Please replace smp_processor_id()
with get_cpu(), stick a put_cpu() at the end, avoid having two function
return points, test it and send me the diff?
On Tue, 2004-03-09 at 09:36, Andrew Morton wrote:
> Srivatsa Vaddagiri <[email protected]> wrote:
> > int current_is_keventd(void)
> > {
> > + int cpu = smp_processor_id();
> > + cwq = keventd_wq->cpu_wq + cpu;
> > + if (current == cwq->thread)
> > + return 1;
> > + else
> > + return 0;
> > }
>
> Is racy in the presence of preemption.
Actually, it's not, because if current *is* keventd we're nailed to the
CPU, and if it's not, we're going to return false either way.
But it *should* be fixed, simply as an example to others.
Cheers,
Rusty.
--
Anyone who quotes me in their signature is an idiot -- Rusty Russell
On Mon, Mar 08, 2004 at 02:36:58PM -0800, Andrew Morton wrote:
> Is racy in the presence of preemption. Please replace smp_processor_id()
> with get_cpu(), stick a put_cpu() at the end, avoid having two function
> return points, test it and send me the diff?
Hi Andrew,
I had considered preemption and had thought using just
smp_processor_id() should be safe, since anyway the per-cpu kevent
thread is bound to its cpu alone.
Patch below is booted/tested against 2.6.4-rc2-mm1 on a 4-way x86 SMP box.
For testing, I basically called call_usermodehelper from inside a
work function (activated using schedule_work) and checked that
current_is_keventd() macro was returning true.
--- workqueue.c.org 2004-03-09 11:34:30.000000000 +0530
+++ workqueue.c 2004-03-09 19:06:51.000000000 +0530
@@ -374,16 +374,17 @@
int current_is_keventd(void)
{
struct cpu_workqueue_struct *cwq;
- int cpu;
+ int cpu = smp_processor_id();
+ int ret = 0;
BUG_ON(!keventd_wq);
- for_each_cpu(cpu) {
- cwq = keventd_wq->cpu_wq + cpu;
- if (current == cwq->thread)
- return 1;
- }
- return 0;
+ cwq = keventd_wq->cpu_wq + cpu;
+ if (current == cwq->thread)
+ ret = 1;
+
+ return ret;
+
}
#ifdef CONFIG_HOTPLUG_CPU
--
Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017