after the discussion on fastboot I came up with the following patch
(this was all done at 35000 feet so if it's h0rked .. I'll claim lack of oxygen)
I'll also reply to this email with 2 users of the new infrastructure just to show how it'd be used.
>From c5fd398d7210bcdc726dc813523d8b4c58481553 Mon Sep 17 00:00:00 2001
From: Arjan van de Ven <[email protected]>
Date: Sun, 12 Oct 2008 15:27:22 -0400
Subject: [PATCH] fastboot: Introduce an asynchronous function call mechanism
During the system boot there are many things that take a long time
and also can be done asynchronous; this patch introduces a call_async()
function that implements a pool of threads to execute the asynchronous
calls.
The calls are divided into pools, and within a pool the calls are processed
in order; this is done to preserve stable device numbers.
Signed-off-by: Arjan van de Ven <[email protected]>
---
include/linux/workqueue.h | 16 +++
init/main.c | 4 +-
kernel/Makefile | 2 +-
kernel/asynccall.c | 224 +++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 244 insertions(+), 2 deletions(-)
create mode 100644 kernel/asynccall.c
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 5c158c4..8122aff 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -238,4 +238,20 @@ void cancel_rearming_delayed_work(struct delayed_work *work)
cancel_delayed_work_sync(work);
}
+
+
+/* async call infrastructure */
+extern void init_async_calls(void);
+extern void end_async_calls(void);
+extern void call_async(int pool_number, int argc, ...);
+
+#define ASYNC_POOL_SCSI 0 /* for everything using the scsi device space */
+#define ASYNC_POOL_IDE 1 /* for everything using the ide device space */
+#define ASYNC_POOL_USB 2 /* for everything using the usb device space */
+#define ASYNC_POOL_MISC 4 /* for everything using the misc device space */
+#define ASYNC_POOL_SOUND 5 /* for everything using the ALSA device space */
+#define ASYNC_POOL_AGP 6 /* for everything using the AGP device space */
+#define ASYNC_MAX_POOL 7
+
+
#endif
diff --git a/init/main.c b/init/main.c
index 3820323..b3ebf60 100644
--- a/init/main.c
+++ b/init/main.c
@@ -691,7 +691,7 @@ asmlinkage void __init start_kernel(void)
rest_init();
}
-static int initcall_debug;
+int initcall_debug;
static int __init initcall_debug_setup(char *str)
{
@@ -769,10 +769,12 @@ static void __init do_basic_setup(void)
rcu_init_sched(); /* needed by module_init stage. */
/* drivers will send hotplug events */
init_workqueues();
+ init_async_calls();
usermodehelper_init();
driver_init();
init_irq_proc();
do_initcalls();
+ end_async_calls();
}
static void __init do_pre_smp_initcalls(void)
diff --git a/kernel/Makefile b/kernel/Makefile
index 4e1d7df..8e0aae9 100644
--- a/kernel/Makefile
+++ b/kernel/Makefile
@@ -9,7 +9,7 @@ obj-y = sched.o fork.o exec_domain.o panic.o printk.o \
rcupdate.o extable.o params.o posix-timers.o \
kthread.o wait.o kfifo.o sys_ni.o posix-cpu-timers.o mutex.o \
hrtimer.o rwsem.o nsproxy.o srcu.o semaphore.o \
- notifier.o ksysfs.o pm_qos_params.o sched_clock.o
+ notifier.o ksysfs.o pm_qos_params.o sched_clock.o asynccall.o
CFLAGS_REMOVE_sched.o = -mno-spe
diff --git a/kernel/asynccall.c b/kernel/asynccall.c
new file mode 100644
index 0000000..1eb5930
--- /dev/null
+++ b/kernel/asynccall.c
@@ -0,0 +1,224 @@
+/*
+ * asynccall.c: Simple asynchronous function call mechanism
+ *
+ * Note: If you want to schedule some delayed work, this is not
+ * the place to look. You want to look in workqueue.c instead.
+ *
+ * callqueue's are aimed at running certain portions of the boot
+ * process asynchronous and not more than that. During regular
+ * kernel boot, these asynchronous calls are actually executed
+ * synchronously (for now).
+ *
+ * For those who wonder why async calls don't use work queues,
+ * the answer is that the object lifecycle rules of work queues
+ * don't work well for the async call purpose.
+ *
+ * (C) Copyright 2008 Intel Corporation
+ * Author: Arjan van de Ven <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; version 2
+ * of the License.
+ */
+
+#include <linux/list.h>
+#include <linux/workqueue.h>
+#include <linux/wait.h>
+#include <linux/hrtimer.h>
+#include <linux/slab.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+
+#include <stdarg.h>
+
+static int async_active = 0;
+
+typedef int (*async_func_t_0)(void);
+typedef int (*async_func_t_1)(void *);
+typedef int (*async_func_t_2)(void *, void *);
+typedef int (*async_func_t_3)(void *, void *, void *);
+typedef int (*async_func_t_4)(void *, void *, void *, void *);
+
+
+struct async_item {
+ struct list_head list;
+
+ async_func_t_0 func;
+ int argument_count;
+ void *arg1;
+ void *arg2;
+ void *arg3;
+ void *arg4;
+};
+
+
+static struct list_head list_pool[ASYNC_MAX_POOL + 1];
+static struct task_struct * thread_pool[ASYNC_MAX_POOL + 1];
+static wait_queue_head_t waitqueue_pool[ASYNC_MAX_POOL + 1];
+static int pool_count[ASYNC_MAX_POOL + 1];
+
+static spinlock_t pool_lock;
+
+extern int initcall_debug;
+
+static void do_async_item(struct async_item *item)
+{
+ ktime_t t0, t1, delta;
+ int result;
+ async_func_t_0 fn0 = item->func;
+ async_func_t_1 fn1 = (async_func_t_1)item->func;
+ async_func_t_2 fn2 = (async_func_t_2)item->func;
+ async_func_t_3 fn3 = (async_func_t_3)item->func;
+ async_func_t_4 fn4 = (async_func_t_4)item->func;
+
+ if (initcall_debug) {
+ printk("calling %pF @ %i\n", item->func,
+ task_pid_nr(current));
+ t0 = ktime_get();
+ }
+
+ switch (item->argument_count) {
+ case 0:
+ result = fn0();
+ break;
+ case 1:
+ result = fn1(item->arg1);
+ break;
+ case 2:
+ result = fn2(item->arg1, item->arg2);
+ break;
+ case 3:
+ result = fn3(item->arg1, item->arg2, item->arg3);
+ break;
+ case 4:
+ result = fn4(item->arg1, item->arg2, item->arg3, item->arg4);
+ break;
+ default:
+ result = 0;
+ WARN_ON(1);
+ }
+ if (initcall_debug) {
+ t1 = ktime_get();
+ delta = ktime_sub(t1, t0);
+
+ printk("asynccall %pF returned %d after %Ld msecs\n",
+ item->func, result,
+ (unsigned long long) delta.tv64 >> 20);
+ }
+}
+
+
+static int async_thread(void *data)
+{
+ int pool = (unsigned long) data;
+
+ DECLARE_WAITQUEUE(wq, current);
+
+ add_wait_queue(&waitqueue_pool[pool], &wq);
+
+ while (!kthread_should_stop()) {
+ struct async_item *item = NULL;
+
+ spin_lock(&pool_lock);
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (!list_empty(&list_pool[pool])) {
+ item = list_first_entry(&list_pool[pool], struct async_item, list);
+ list_del(&item->list);
+ pool_count[pool]--;
+ }
+ spin_unlock(&pool_lock);
+
+ if (!item) {
+ schedule();
+ continue;
+ }
+ __set_current_state(TASK_RUNNING);
+ do_async_item(item);
+ kfree(item);
+ wake_up(&waitqueue_pool[pool]);
+ }
+ return 0;
+}
+
+
+void init_async_calls(void)
+{
+ unsigned long i;
+ spin_lock_init(&pool_lock);
+ for (i = 0; i <= ASYNC_MAX_POOL; i++) {
+ INIT_LIST_HEAD(&list_pool[i]);
+ init_waitqueue_head(&waitqueue_pool[i]);
+ thread_pool[i] = kthread_run(&async_thread, (void *)i, "kasyncd/%li", i);
+ }
+ async_active = 1;
+}
+
+
+void call_async(int pool_number, int argc, ...)
+{
+ struct async_item *item;
+ va_list ap;
+
+ if (argc > 4 || argc < 0) {
+ WARN(1, KERN_ERR "Too many arguments to async function! Skipping...\n");
+ return;
+ }
+
+ item = kmalloc(sizeof(struct async_item), GFP_ATOMIC);
+ item->argument_count = argc;
+ va_start(ap, argc);
+ item->func = va_arg(ap, async_func_t_0);
+ if (argc > 0)
+ item->arg1 = va_arg(ap, void *);
+ if (argc > 1)
+ item->arg2 = va_arg(ap, void *);
+ if (argc > 2)
+ item->arg3 = va_arg(ap, void *);
+ if (argc > 4)
+ item->arg3 = va_arg(ap, void *);
+ va_end(ap);
+
+ /* If we're not yet or no longer active, just process the work item in place */
+ if (!async_active) {
+ do_async_item(item);
+ kfree(item);
+ return;
+ }
+ spin_lock(&pool_lock);
+ pool_count[pool_number]++;
+ list_add_tail(&item->list, &list_pool[pool_number]);
+ wake_up(&waitqueue_pool[pool_number]);
+ spin_unlock(&pool_lock);
+}
+EXPORT_SYMBOL_GPL(call_async);
+
+static void wait_until_done(int pool)
+{
+ wait_event(waitqueue_pool[pool], pool_count[pool] == 0);
+}
+
+/**
+ * end_async_calls - Shut down the async call subsystem
+ *
+ */
+void end_async_calls(void)
+{
+ int i;
+
+ if (!async_active)
+ return;
+
+ async_active = 0;
+ /*
+ * make sure all CPUs see the "no more queueing" before we empty the queue
+ */
+ wmb();
+
+ for (i = 0; i <= ASYNC_MAX_POOL; i++)
+ wait_until_done(i);
+
+ for (i = 0; i <= ASYNC_MAX_POOL; i++)
+ kthread_stop(thread_pool[i]);
+}
--
1.5.5.1
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
not for merge! Just for "show that it works"
diff --git a/Makefile b/Makefile
index 16e3fbb..2402f6b 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 27
-EXTRAVERSION =
+EXTRAVERSION = -async
NAME = Rotary Wombat
# *DOCUMENTATION*
diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index b1c723f..f93c3c9 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -896,6 +896,12 @@ static int __init acpi_battery_init(void)
return 0;
}
+static int acpi_battery_init_prime(void)
+{
+ call_async(ASYNC_POOL_MISC, 0, acpi_battery_init);
+ return 0;
+}
+
static void __exit acpi_battery_exit(void)
{
acpi_bus_unregister_driver(&acpi_battery_driver);
@@ -904,5 +910,5 @@ static void __exit acpi_battery_exit(void)
#endif
}
-module_init(acpi_battery_init);
+module_init(acpi_battery_init_prime);
module_exit(acpi_battery_exit);
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 1ee9499..0205eca 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5937,10 +5937,7 @@ int ata_host_activate(struct ata_host *host, int irq,
for (i = 0; i < host->n_ports; i++)
ata_port_desc(host->ports[i], "irq %d", irq);
- rc = ata_host_register(host, sht);
- /* if failed, just free the IRQ and leave ports alone */
- if (rc)
- devm_free_irq(host->dev, irq, host);
+ call_async(ASYNC_POOL_SCSI, 2, ata_host_register, host, sht);
return rc;
}
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Hi Arjan,
On Sun, Oct 12, 2008 at 07:44:27PM -0400, Arjan van de Ven wrote:
> +#define ASYNC_POOL_SCSI 0 /* for everything using the scsi device space */
> +#define ASYNC_POOL_IDE 1 /* for everything using the ide device space */
> +#define ASYNC_POOL_USB 2 /* for everything using the usb device space */
> +#define ASYNC_POOL_MISC 4 /* for everything using the misc device space */
> +#define ASYNC_POOL_SOUND 5 /* for everything using the ALSA device space */
> +#define ASYNC_POOL_AGP 6 /* for everything using the AGP device space */
> +#define ASYNC_MAX_POOL 7
I suspect this list will grow with more general adoption. Wouldn't it be
better to use an enum above ?
Willy
Hi Arjan,
On Mon, Oct 13, 2008 at 2:44 AM, Arjan van de Ven <[email protected]> wrote:
> +void call_async(int pool_number, int argc, ...)
> +{
> + struct async_item *item;
> + va_list ap;
> +
> + if (argc > 4 || argc < 0) {
> + WARN(1, KERN_ERR "Too many arguments to async function! Skipping...\n");
> + return;
> + }
> +
> + item = kmalloc(sizeof(struct async_item), GFP_ATOMIC);
Why is there no check for NULL here?
> + item->argument_count = argc;
> + va_start(ap, argc);
> + item->func = va_arg(ap, async_func_t_0);
> + if (argc > 0)
> + item->arg1 = va_arg(ap, void *);
> + if (argc > 1)
> + item->arg2 = va_arg(ap, void *);
> + if (argc > 2)
> + item->arg3 = va_arg(ap, void *);
> + if (argc > 4)
> + item->arg3 = va_arg(ap, void *);
> + va_end(ap);
> +
> + /* If we're not yet or no longer active, just process the work item in place */
> + if (!async_active) {
> + do_async_item(item);
> + kfree(item);
> + return;
> + }
> + spin_lock(&pool_lock);
> + pool_count[pool_number]++;
> + list_add_tail(&item->list, &list_pool[pool_number]);
> + wake_up(&waitqueue_pool[pool_number]);
> + spin_unlock(&pool_lock);
> +}
> +EXPORT_SYMBOL_GPL(call_async);
* Arjan van de Ven <[email protected]> wrote:
> not for merge! Just for "show that it works"
please propagate something like this into tests/ and create a series of
unit tests like the lockdep self-tests that prints out a series of
WARN()n if it doesnt work or so. It's useful for you too when you are
hacking on it.
Plus an anti-NIH suggestion: you could reuse much of the syslets series
i did - sans the ugly syscall ABI bits. The submit/completion bits were
pretty mature.
Ingo
On Mon, 13 Oct 2008 10:15:03 +0300
"Pekka Enberg" <[email protected]> wrote:
> Hi Arjan,
>
> On Mon, Oct 13, 2008 at 2:44 AM, Arjan van de Ven
> <[email protected]> wrote:
> > +void call_async(int pool_number, int argc, ...)
> > +{
> > + struct async_item *item;
> > + va_list ap;
> > +
> > + if (argc > 4 || argc < 0) {
> > + WARN(1, KERN_ERR "Too many arguments to async
> > function! Skipping...\n");
> > + return;
> > + }
> > +
> > + item = kmalloc(sizeof(struct async_item), GFP_ATOMIC);
>
> Why is there no check for NULL here?
because at 35000 feet it won't return NULL ;-)
but yeah thanks for spotting, I'll fix it.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> On Sun, 12 Oct 2008 19:44:27 -0400 Arjan van de Ven <[email protected]> wrote:
> after the discussion on fastboot I came up with the following patch
> (this was all done at 35000 feet so if it's h0rked .. I'll claim lack of oxygen)
>
> I'll also reply to this email with 2 users of the new infrastructure just to show how it'd be used.
>
>
>
> >From c5fd398d7210bcdc726dc813523d8b4c58481553 Mon Sep 17 00:00:00 2001
> From: Arjan van de Ven <[email protected]>
> Date: Sun, 12 Oct 2008 15:27:22 -0400
> Subject: [PATCH] fastboot: Introduce an asynchronous function call mechanism
>
> During the system boot there are many things that take a long time
> and also can be done asynchronous; this patch introduces a call_async()
> function that implements a pool of threads to execute the asynchronous
> calls.
>
> The calls are divided into pools, and within a pool the calls are processed
> in order; this is done to preserve stable device numbers.
>
> ...
>
> +#include <linux/list.h>
> +#include <linux/workqueue.h>
> +#include <linux/wait.h>
> +#include <linux/hrtimer.h>
> +#include <linux/slab.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +
> +#include <stdarg.h>
> +
> +static int async_active = 0;
?
> +typedef int (*async_func_t_0)(void);
> +typedef int (*async_func_t_1)(void *);
> +typedef int (*async_func_t_2)(void *, void *);
> +typedef int (*async_func_t_3)(void *, void *, void *);
> +typedef int (*async_func_t_4)(void *, void *, void *, void *);
> +
> +
> +struct async_item {
> + struct list_head list;
> +
> + async_func_t_0 func;
> + int argument_count;
> + void *arg1;
> + void *arg2;
> + void *arg3;
> + void *arg4;
> +};
> +
> +
> +static struct list_head list_pool[ASYNC_MAX_POOL + 1];
> +static struct task_struct * thread_pool[ASYNC_MAX_POOL + 1];
> +static wait_queue_head_t waitqueue_pool[ASYNC_MAX_POOL + 1];
> +static int pool_count[ASYNC_MAX_POOL + 1];
> +
> +static spinlock_t pool_lock;
DEFINE_SPINLOCK(), remove spin_lock_init()
> +extern int initcall_debug;
no externs in C.
> +static void do_async_item(struct async_item *item)
> +{
> + ktime_t t0, t1, delta;
> + int result;
> + async_func_t_0 fn0 = item->func;
> + async_func_t_1 fn1 = (async_func_t_1)item->func;
> + async_func_t_2 fn2 = (async_func_t_2)item->func;
> + async_func_t_3 fn3 = (async_func_t_3)item->func;
> + async_func_t_4 fn4 = (async_func_t_4)item->func;
> +
> + if (initcall_debug) {
> + printk("calling %pF @ %i\n", item->func,
> + task_pid_nr(current));
> + t0 = ktime_get();
> + }
> +
> + switch (item->argument_count) {
> + case 0:
> + result = fn0();
> + break;
> + case 1:
> + result = fn1(item->arg1);
> + break;
> + case 2:
> + result = fn2(item->arg1, item->arg2);
> + break;
> + case 3:
> + result = fn3(item->arg1, item->arg2, item->arg3);
> + break;
> + case 4:
> + result = fn4(item->arg1, item->arg2, item->arg3, item->arg4);
> + break;
> + default:
> + result = 0;
> + WARN_ON(1);
> + }
> + if (initcall_debug) {
> + t1 = ktime_get();
> + delta = ktime_sub(t1, t0);
> +
> + printk("asynccall %pF returned %d after %Ld msecs\n",
> + item->func, result,
> + (unsigned long long) delta.tv64 >> 20);
> + }
> +}
> +
> +
> +static int async_thread(void *data)
> +{
> + int pool = (unsigned long) data;
> +
> + DECLARE_WAITQUEUE(wq, current);
random newline in locals
> + add_wait_queue(&waitqueue_pool[pool], &wq);
never gets removed
> + while (!kthread_should_stop()) {
> + struct async_item *item = NULL;
> +
> + spin_lock(&pool_lock);
> + set_current_state(TASK_INTERRUPTIBLE);
> + if (!list_empty(&list_pool[pool])) {
> + item = list_first_entry(&list_pool[pool], struct async_item, list);
> + list_del(&item->list);
> + pool_count[pool]--;
> + }
> + spin_unlock(&pool_lock);
> +
> + if (!item) {
> + schedule();
> + continue;
> + }
> + __set_current_state(TASK_RUNNING);
> + do_async_item(item);
> + kfree(item);
> + wake_up(&waitqueue_pool[pool]);
> + }
> + return 0;
> +}
> +
> +
> +void init_async_calls(void)
> +{
> + unsigned long i;
> + spin_lock_init(&pool_lock);
> + for (i = 0; i <= ASYNC_MAX_POOL; i++) {
> + INIT_LIST_HEAD(&list_pool[i]);
> + init_waitqueue_head(&waitqueue_pool[i]);
> + thread_pool[i] = kthread_run(&async_thread, (void *)i, "kasyncd/%li", i);
> + }
> + async_active = 1;
> +}
Please talk to Evgeniy about his new thread_pool stuff which I've suggested
become a generic library.
(looks at changelog, doesn't really understand the need for the thread pool
thing, doesn't understand the comment about device numbers)
> +
> +void call_async(int pool_number, int argc, ...)
This looks like schedule_work(). Why cannot that be used (perhaps after
suitable modification)?
Hi Andrew, Arjan.
Actually Arjan will not see my reply, since he added my mail to the
blacklist :)
On Wed, Oct 15, 2008 at 01:41:17AM -0700, Andrew Morton ([email protected]) wrote:
> > On Sun, 12 Oct 2008 19:44:27 -0400 Arjan van de Ven <[email protected]> wrote:
> > after the discussion on fastboot I came up with the following patch
> > (this was all done at 35000 feet so if it's h0rked .. I'll claim lack of oxygen)
> >
> > I'll also reply to this email with 2 users of the new infrastructure just to show how it'd be used.
> >
> >
> >
> > >From c5fd398d7210bcdc726dc813523d8b4c58481553 Mon Sep 17 00:00:00 2001
> > From: Arjan van de Ven <[email protected]>
> > Date: Sun, 12 Oct 2008 15:27:22 -0400
> > Subject: [PATCH] fastboot: Introduce an asynchronous function call mechanism
> >
> > During the system boot there are many things that take a long time
> > and also can be done asynchronous; this patch introduces a call_async()
> > function that implements a pool of threads to execute the asynchronous
> > calls.
> >
> > The calls are divided into pools, and within a pool the calls are processed
> > in order; this is done to preserve stable device numbers.
> >
> > ...
> >
> > +#include <linux/list.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/wait.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/slab.h>
> > +#include <linux/kthread.h>
> > +#include <linux/module.h>
> > +#include <linux/spinlock.h>
> > +
> > +#include <stdarg.h>
> > +
> > +static int async_active = 0;
>
> ?
That's to prevent users of this system, which were registered earlier,
to access yet uninitialized memory and fail.
> > +void init_async_calls(void)
> > +{
> > + unsigned long i;
> > + spin_lock_init(&pool_lock);
> > + for (i = 0; i <= ASYNC_MAX_POOL; i++) {
> > + INIT_LIST_HEAD(&list_pool[i]);
> > + init_waitqueue_head(&waitqueue_pool[i]);
> > + thread_pool[i] = kthread_run(&async_thread, (void *)i, "kasyncd/%li", i);
> > + }
> > + async_active = 1;
> > +}
>
> Please talk to Evgeniy about his new thread_pool stuff which I've suggested
> become a generic library.
>
> (looks at changelog, doesn't really understand the need for the thread pool
> thing, doesn't understand the comment about device numbers)
Well, yes, this case can be implemented via my thread pool code in DST.
> > +
> > +void call_async(int pool_number, int argc, ...)
>
> This looks like schedule_work(). Why cannot that be used (perhaps after
> suitable modification)?
Thread pool is a better way than a workqueue, when we have number of
tasks to be completed on behalf of some process we do not care which.
With workqueue you either have to have one workqueue per requested work,
or implement non-trivial policy on adding workqueues on demand. With
thread pool you can add new thread and schedule new work, if all other
threads are used. When all event threads are used, no new work can be
executed, since there is no way to extend number of those threads on
demand.
--
Evgeniy Polyakov
On Wed, 15 Oct 2008 01:41:17 -0700
Andrew Morton <[email protected]> wrote:
> > +static int async_active = 0;
>
> ?
ok will add comment
>
> Please talk to Evgeniy about his new thread_pool stuff which I've
> suggested become a generic library.
>
> (looks at changelog, doesn't really understand the need for the
> thread pool thing, doesn't understand the comment about device
> numbers)
it's very simple. Take the case of doing each sata port probe as an
asynchronous item. In order to keep a stable device naming/ordering,
all the sata probes need to be done in sequence, but, say, the USB
probing can be done in parallel. This means that we need to assign the
various async tasks to specific pools (based on which device number
space they will put devices in) and can't just arbitrarily spawn them
to arbitrary threads.
>
> > +
> > +void call_async(int pool_number, int argc, ...)
>
> This looks like schedule_work(). Why cannot that be used (perhaps
> after suitable modification)?
as I said in the comments... the object lifetimes don't work.
We need to be able to schedule one per sata port (as per Linus'
suggest) which means we need a new work struct per function call. And
then we need something that garbage collects these, now dynamically
allocated, work structs. Which work queues just don't do. If you make
them do that... you end up with the code I wrote.
>
>
>
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> it's very simple. Take the case of doing each sata port probe as an
> asynchronous item. In order to keep a stable device naming/ordering,
> all the sata probes need to be done in sequence
Not really, you need the results to be ordered which simply means you
need a rendezvous and ordered dispatch from that point.
For ATA stability the only serialization points are handling out ataxx:
numbers and handing out device identifiers. You can find all the ports in
parallel and you can probe them all for disks in parallel (Power supply
permitting).
In fact in both those cases you could do it entirely in parallel as you
can order the probe sequence at start time. That in turn means that
you can defer assignation of names, but that might make the boot messages
a bit confusing.
I'd also question call_async as we don't have infinite thread resources
(or anywhere near sufficient) so in fact what you have is
queue_async(arguments)/complete_async_work(result). Also the async call
is exactly that so we need to be very careful people realise that and
don't pass pointers to local stack objects or we will have some truely
hellish debugging to do. In fact I suspect passing a single
'private_data' object as we do for irqs and other async event handlers is
far safer.
Implementing the queue_async_sequenced(arguments) /
complete_async_sequenced() / finalize_sequenced() model would make life a
lot easier for stuff like device probing with ordering as well.
queue_async_sequenced(sequence, call, finalize, private_data)
queue asynchronous task and if resources exist start it as call(sequence,
private_data)
complete_async_sequenced - finish this task, store result and if
possible despatch the next one
finalize_sequenced - wait for all completions in this sequence
then run the finalize method of each strictly in the order that they were
added orginally by queue_async_sequenced.
gives us exactly what is needed for stuff like SATA probe. After the
finalize the ability to restart_async_sequenced() with a new
call/finalize function would be elegant but not essential ...
Alan
On Wed, 15 Oct 2008 07:52:46 -0400 Arjan van de Ven <[email protected]> wrote:
> On Wed, 15 Oct 2008 01:41:17 -0700
> Andrew Morton <[email protected]> wrote:
> > > +static int async_active = 0;
> >
> > ?
> ok will add comment
I was actually "?"ing at the "= 0". I thought that would be obvious
but it's whizzed past two people so far :(
> >
> > Please talk to Evgeniy about his new thread_pool stuff which I've
> > suggested become a generic library.
^^ this?
On Wed, Oct 15, 2008 at 09:59:05AM -0700, Andrew Morton wrote:
> On Wed, 15 Oct 2008 07:52:46 -0400 Arjan van de Ven <[email protected]> wrote:
>
> > On Wed, 15 Oct 2008 01:41:17 -0700
> > Andrew Morton <[email protected]> wrote:
> > > > +static int async_active = 0;
> > >
> > > ?
> > ok will add comment
>
> I was actually "?"ing at the "= 0". I thought that would be obvious
> but it's whizzed past two people so far :(
Is there evidence that some gccs will not add such variable to .bss?
Because "= 0;" is more readable.
> > I was actually "?"ing at the "= 0". I thought that would be obvious
> > but it's whizzed past two people so far :(
>
> Is there evidence that some gccs will not add such variable to .bss?
>
> Because "= 0;" is more readable.
From: http://gcc.gnu.org/gcc-3.3/changes.html
GCC 3.3.1 automatically places zero-initialized variables in the .bss
section on some operating systems. Versions of GNU Emacs up to (and
including) 21.3 will not work correctly when using this optimization;
you can use -fno-zero-initialized-in-bss to disable it.
so presumably gcc 3.2 (which we still support, right?) does not do this
(and puts such variables in .data).
- R.
On Wed, Oct 15, 2008 at 10:55:15AM -0700, Roland Dreier wrote:
> > > I was actually "?"ing at the "= 0". I thought that would be obvious
> > > but it's whizzed past two people so far :(
> >
> > Is there evidence that some gccs will not add such variable to .bss?
> >
> > Because "= 0;" is more readable.
>
> From: http://gcc.gnu.org/gcc-3.3/changes.html
>
> GCC 3.3.1 automatically places zero-initialized variables in the .bss
> section on some operating systems. Versions of GNU Emacs up to (and
> including) 21.3 will not work correctly when using this optimization;
> you can use -fno-zero-initialized-in-bss to disable it.
>
> so presumably gcc 3.2 (which we still support, right?) does not do this
> (and puts such variables in .data).
So we're doing this for one low-end gcc version, nobody except Andi uses
(it must be patched surely).
On Wed, 15 Oct 2008 21:52:52 +0400 Alexey Dobriyan <[email protected]> wrote:
> On Wed, Oct 15, 2008 at 09:59:05AM -0700, Andrew Morton wrote:
> > On Wed, 15 Oct 2008 07:52:46 -0400 Arjan van de Ven <[email protected]> wrote:
> >
> > > On Wed, 15 Oct 2008 01:41:17 -0700
> > > Andrew Morton <[email protected]> wrote:
> > > > > +static int async_active = 0;
> > > >
> > > > ?
> > > ok will add comment
> >
> > I was actually "?"ing at the "= 0". I thought that would be obvious
> > but it's whizzed past two people so far :(
>
> Is there evidence that some gccs will not add such variable to .bss?
It does get placed in bss.
> Because "= 0;" is more readable.
Only to someone who doesn't know anything about C.
For the rest of us it is inconsistent, is a visual distraction and
wastes space which would be better taken up by a comment explaining the
variable's function (lol).
On Wed, 15 Oct 2008 09:59:05 -0700
Andrew Morton <[email protected]> wrote:
> > >
> > > Please talk to Evgeniy about his new thread_pool stuff which I've
> > > suggested become a generic library.
>
> ^^ this?
if thread pools can deliver me the same ordering guarantees.....
which I kinda doubt given the point of the concept of them..
Hi Arjan.
[ I've switched to the different mail system recently, you could update
you blacklist filter, or we can bury the war axe: ipw2100 still does not
work with power tricks :) ]
On Wed, Oct 15, 2008 at 04:23:16PM -0400, Arjan van de Ven ([email protected]) wrote:
> On Wed, 15 Oct 2008 09:59:05 -0700
> Andrew Morton <[email protected]> wrote:
>
> > > >
> > > > Please talk to Evgeniy about his new thread_pool stuff which I've
> > > > suggested become a generic library.
> >
> > ^^ this?
>
> if thread pools can deliver me the same ordering guarantees.....
> which I kinda doubt given the point of the concept of them..
One can schedule execution on given thread in the pool (actually on
given set of threads, which have the same private pointer), if needed
threads are busy, caller will sleep, if there are no such threads,
execution fails. Although I'm not quite sure this is exactly what you
asked for.
--
Evgeniy Polyakov