Subject: [RFC 0/4] Cpu-Hotplug: Use per subsystem hot-cpu mutexes.

Hello everyone,

Since 2.6.18-something, the community has been bugged by the problem to
provide a clean and a stable mechanism to postpone a cpu-hotplug event
as lock_cpu_hotplug was badly broken.

This is another proposal towards solving that problem. This one is
along the lines of the solution provided in kernel/workqueue.c

Instead of having a global mechanism like lock_cpu_hotplug,
we allow the subsytems to define their own per-subsystem hot cpu
mutexes. These would be taken(released) where ever we are currently
calling lock_cpu_hotplug(unlock_cpu_hotplug).

Also, in the per-subsystem hotcpu callback function,we take
this mutex before we handle any pre-cpu-hotplug events and release
it once we finish handling the post-cpu-hotplug events. A standard
means for doing this has been provided in [PATCH 2/4] and demonstrated
in [PATCH 3/4].

The ordering of these per-subsystem mutexes might still prove to be a
problem, but hopefully lockdep should help us get out of that muddle.

The patch set to be applied against linux-2.6.19-rc5 is as follows:

[PATCH 1/4] : Extend notifier_call_chain with an option to specify the
number of notifications to be sent and also count the
number of notifications actually sent.

[PATCH 2/4] : Define events CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE
and send out notifications for these in _cpu_up and
_cpu_down. This would help us standardise the acquire and
release of the subsystem locks in the hotcpu
callback functions of these subsystems.

[PATCH 3/4] : Eliminate lock_cpu_hotplug from kernel/sched.c.

[PATCH 4/4] : In workqueue_cpu_callback function, acquire(release) the
workqueue_mutex while handling
CPU_LOCK_ACQUIRE(CPU_LOCK_RELEASE).

If the per-subsystem-locking approach survives the test of time,
we can expect a slow phasing out of lock_cpu_hotplug, which has not
yet been eliminated in these patches :)

Awaiting your feedback.

Thanks,
gautham.

PS: These patches are intended for post 2.6.19, since most of the warnings
with respect to cpu_hotplug_locking (including the cpufreq ones) seem to
have disappeared in 2.6.19-rc5.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"


Subject: [PATCH 2/4] Define and use new events,CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE.

This is an attempt to provide an alternate mechanism for postponing
a hotplug event instead of using a global mechanism like lock_cpu_hotplug.

The proposal is to add two new events namely CPU_LOCK_ACQUIRE and
CPU_LOCK_RELEASE. The notification for these two events would be sent
out before and after a cpu_hotplug event respectively.

During the CPU_LOCK_ACQUIRE event, a cpu-hotplug-aware subsystem is
supposed to acquire any per-subsystem hotcpu mutex ( Eg. workqueue_mutex
in kernel/workqueue.c ).

During the CPU_LOCK_RELEASE release event the cpu-hotplug-aware subsystem
is supposed to release the per-subsystem hotcpu mutex.

The reasons for defining new events as opposed to reusing the existing events
like CPU_UP_PREPARE/CPU_UP_FAILED/CPU_ONLINE for locking/unlocking of
per-subsystem hotcpu mutexes are as follow:

- CPU_LOCK_ACQUIRE: All hotcpu mutexes are taken before subsystems
start handling pre-hotplug events like CPU_UP_PREPARE/CPU_DOWN_PREPARE
etc, thus ensuring a clean handling of these events.

- CPU_LOCK_RELEASE: The hotcpu mutexes will be released only after
all subsystems have handled post-hotplug events like CPU_DOWN_FAILED,
CPU_DEAD,CPU_ONLINE etc thereby ensuring that there are no subsequent
clashes amongst the interdependent subsystems after a cpu hotplugs.

This patch also uses __raw_notifier_call chain in _cpu_up to take care
of the dependency between the two consequetive calls to
raw_notifier_call_chain.

Signed-off-by: Gautham R Shenoy <[email protected]>

--
include/linux/notifier.h | 2 ++
kernel/cpu.c | 15 +++++++++++----
2 files changed, 13 insertions(+), 4 deletions(-)

Index: hotplug/kernel/cpu.c
===================================================================
--- hotplug.orig/kernel/cpu.c
+++ hotplug/kernel/cpu.c
@@ -132,6 +132,8 @@ static int _cpu_down(unsigned int cpu)
if (!cpu_online(cpu))
return -EINVAL;

+ raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE,
+ (void *)(long)cpu);
err = raw_notifier_call_chain(&cpu_chain, CPU_DOWN_PREPARE,
(void *)(long)cpu);
if (err == NOTIFY_BAD) {
@@ -185,6 +187,8 @@ out_thread:
err = kthread_stop(p);
out_allowed:
set_cpus_allowed(current, old_allowed);
+ raw_notifier_call_chain(&cpu_chain, CPU_LOCK_RELEASE,
+ (void *)(long)cpu);
return err;
}

@@ -206,13 +210,15 @@ int cpu_down(unsigned int cpu)
/* Requires cpu_add_remove_lock to be held */
static int __devinit _cpu_up(unsigned int cpu)
{
- int ret;
+ int ret, nr_calls = 0;
void *hcpu = (void *)(long)cpu;

if (cpu_online(cpu) || !cpu_present(cpu))
return -EINVAL;

- ret = raw_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu);
+ raw_notifier_call_chain(&cpu_chain, CPU_LOCK_ACQUIRE, hcpu);
+ ret = __raw_notifier_call_chain(&cpu_chain, CPU_UP_PREPARE, hcpu,
+ -1, &nr_calls);
if (ret == NOTIFY_BAD) {
printk("%s: attempt to bring up CPU %u failed\n",
__FUNCTION__, cpu);
@@ -233,8 +239,9 @@ static int __devinit _cpu_up(unsigned in

out_notify:
if (ret != 0)
- raw_notifier_call_chain(&cpu_chain,
- CPU_UP_CANCELED, hcpu);
+ __raw_notifier_call_chain(&cpu_chain,
+ CPU_UP_CANCELED, hcpu, nr_calls, NULL);
+ raw_notifier_call_chain(&cpu_chain, CPU_LOCK_RELEASE, hcpu);

return ret;
}
Index: hotplug/include/linux/notifier.h
===================================================================
--- hotplug.orig/include/linux/notifier.h
+++ hotplug/include/linux/notifier.h
@@ -194,6 +194,8 @@ extern int __srcu_notifier_call_chain(st
#define CPU_DOWN_PREPARE 0x0005 /* CPU (unsigned)v going down */
#define CPU_DOWN_FAILED 0x0006 /* CPU (unsigned)v NOT going down */
#define CPU_DEAD 0x0007 /* CPU (unsigned)v dead */
+#define CPU_LOCK_ACQUIRE 0x0008 /* Acquire all hotcpu locks */
+#define CPU_LOCK_RELEASE 0x0009 /* Release all hotcpu locks */

#endif /* __KERNEL__ */
#endif /* _LINUX_NOTIFIER_H */
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Subject: [PATCH 3/4] Eliminate lock_cpu_hotplug in kernel/sched.c

Eliminate lock_cpu_hotplug from kernel/sched.c and use sched_hotcpu_mutex
instead to postpone a hotplug event.

In the migration_call hotcpu callback function, take sched_hotcpu_mutex
while handling the event CPU_LOCK_ACQUIRE and release it while handling
CPU_LOCK_RELEASE event.

Signed-off-by: Gautham R Shenoy <[email protected]>

--
kernel/sched.c | 29 +++++++++++++++++++----------
1 files changed, 19 insertions(+), 10 deletions(-)

Index: hotplug/kernel/sched.c
===================================================================
--- hotplug.orig/kernel/sched.c
+++ hotplug/kernel/sched.c
@@ -267,6 +267,7 @@ struct rq {
};

static DEFINE_PER_CPU(struct rq, runqueues);
+static DEFINE_MUTEX(sched_hotcpu_mutex);

static inline int cpu_of(struct rq *rq)
{
@@ -4327,13 +4328,13 @@ long sched_setaffinity(pid_t pid, cpumas
struct task_struct *p;
int retval;

- lock_cpu_hotplug();
+ mutex_lock(&sched_hotcpu_mutex);
read_lock(&tasklist_lock);

p = find_process_by_pid(pid);
if (!p) {
read_unlock(&tasklist_lock);
- unlock_cpu_hotplug();
+ mutex_unlock(&sched_hotcpu_mutex);
return -ESRCH;
}

@@ -4360,7 +4361,7 @@ long sched_setaffinity(pid_t pid, cpumas

out_unlock:
put_task_struct(p);
- unlock_cpu_hotplug();
+ mutex_unlock(&sched_hotcpu_mutex);
return retval;
}

@@ -4417,7 +4418,7 @@ long sched_getaffinity(pid_t pid, cpumas
struct task_struct *p;
int retval;

- lock_cpu_hotplug();
+ mutex_lock(&sched_hotcpu_mutex);
read_lock(&tasklist_lock);

retval = -ESRCH;
@@ -4433,7 +4434,7 @@ long sched_getaffinity(pid_t pid, cpumas

out_unlock:
read_unlock(&tasklist_lock);
- unlock_cpu_hotplug();
+ mutex_unlock(&sched_hotcpu_mutex);
if (retval)
return retval;

@@ -5225,6 +5226,10 @@ migration_call(struct notifier_block *nf
struct rq *rq;

switch (action) {
+ case CPU_LOCK_ACQUIRE:
+ mutex_lock(&sched_hotcpu_mutex);
+ break;
+
case CPU_UP_PREPARE:
p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
if (IS_ERR(p))
@@ -5270,7 +5275,7 @@ migration_call(struct notifier_block *nf
BUG_ON(rq->nr_running != 0);

/* No need to migrate the tasks: it was best-effort if
- * they didn't do lock_cpu_hotplug(). Just wake up
+ * they didn't take sched_hotcpu_mutex. Just wake up
* the requestors. */
spin_lock_irq(&rq->lock);
while (!list_empty(&rq->migration_queue)) {
@@ -5283,6 +5288,10 @@ migration_call(struct notifier_block *nf
}
spin_unlock_irq(&rq->lock);
break;
+
+ case CPU_LOCK_RELEASE:
+ mutex_unlock(&sched_hotcpu_mutex);
+ break;
#endif
}
return NOTIFY_OK;
@@ -6652,10 +6661,10 @@ int arch_reinit_sched_domains(void)
{
int err;

- lock_cpu_hotplug();
+ mutex_lock(&sched_hotcpu_mutex);
detach_destroy_domains(&cpu_online_map);
err = arch_init_sched_domains(&cpu_online_map);
- unlock_cpu_hotplug();
+ mutex_unlock(&sched_hotcpu_mutex);

return err;
}
@@ -6763,12 +6772,12 @@ void __init sched_init_smp(void)
{
cpumask_t non_isolated_cpus;

- lock_cpu_hotplug();
+ mutex_lock(&sched_hotcpu_mutex);
arch_init_sched_domains(&cpu_online_map);
cpus_andnot(non_isolated_cpus, cpu_online_map, cpu_isolated_map);
if (cpus_empty(non_isolated_cpus))
cpu_set(smp_processor_id(), non_isolated_cpus);
- unlock_cpu_hotplug();
+ mutex_unlock(&sched_hotcpu_mutex);
/* XXX: Theoretical race here - CPU may be hotplugged now */
hotcpu_notifier(update_sched_domains, 0);

--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Subject: [PATCH 4/4] Handle CPU_LOCK_ACQUIRE and CPU_LOCK_RELEASE in workqueue_cpu_callback.

In hot-cpu callback function workqueue_cpu_callback, lock the workqueue_mutex
under CPU_LOCK_ACQUIRE and release it under CPU_LOCK_RELEASE.

This eliminates handling of redundant events namely CPU_DOWN_PREPARE and
CPU_DOWN_FAILED.

Signed-off-by: Gautham R Shenoy <[email protected]>

--
kernel/workqueue.c | 18 +++++++-----------
1 files changed, 7 insertions(+), 11 deletions(-)

Index: hotplug/kernel/workqueue.c
===================================================================
--- hotplug.orig/kernel/workqueue.c
+++ hotplug/kernel/workqueue.c
@@ -638,8 +638,11 @@ static int __devinit workqueue_cpu_callb
struct workqueue_struct *wq;

switch (action) {
- case CPU_UP_PREPARE:
+ case CPU_LOCK_ACQUIRE:
mutex_lock(&workqueue_mutex);
+ break;
+
+ case CPU_UP_PREPARE:
/* Create a new workqueue thread for it. */
list_for_each_entry(wq, &workqueues, list) {
if (!create_workqueue_thread(wq, hotcpu)) {
@@ -658,7 +661,6 @@ static int __devinit workqueue_cpu_callb
kthread_bind(cwq->thread, hotcpu);
wake_up_process(cwq->thread);
}
- mutex_unlock(&workqueue_mutex);
break;

case CPU_UP_CANCELED:
@@ -670,15 +672,6 @@ static int __devinit workqueue_cpu_callb
any_online_cpu(cpu_online_map));
cleanup_workqueue_thread(wq, hotcpu);
}
- mutex_unlock(&workqueue_mutex);
- break;
-
- case CPU_DOWN_PREPARE:
- mutex_lock(&workqueue_mutex);
- break;
-
- case CPU_DOWN_FAILED:
- mutex_unlock(&workqueue_mutex);
break;

case CPU_DEAD:
@@ -686,6 +679,9 @@ static int __devinit workqueue_cpu_callb
cleanup_workqueue_thread(wq, hotcpu);
list_for_each_entry(wq, &workqueues, list)
take_over_work(wq, hotcpu);
+ break;
+
+ case CPU_LOCK_RELEASE:
mutex_unlock(&workqueue_mutex);
break;
}
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Subject: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

Provide notifier_call_chain with an option to call only a specified number of
notifiers and also record the number of call to notifiers made.

The need for this enhancement was identified in the post entitled
"Slab - Eliminate lock_cpu_hotplug from slab"
(http://lkml.org/lkml/2006/10/28/92) by Ravikiran G Thirumalai and
Andrew Morton.

This patch adds two additional parameters to notifier_call_chain API namely
- int nr_to_calls : Number of notifier_functions to be called.
The don't care value is -1.

- unsigned int *nr_calls : Records the total number of notifier_funtions
called by notifier_call_chain. The don't care
value is NULL.

Credit : Andrew Morton <[email protected]>
Signed-off-by: Gautham R Shenoy <[email protected]>

--
include/linux/notifier.h | 8 +++
kernel/sys.c | 97 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 89 insertions(+), 16 deletions(-)

Index: hotplug/kernel/sys.c
===================================================================
--- hotplug.orig/kernel/sys.c
+++ hotplug/kernel/sys.c
@@ -134,19 +134,41 @@ static int notifier_chain_unregister(str
return -ENOENT;
}

+/*
+ * notifier_call_chain - Informs the registered notifiers about an event.
+ *
+ * @nl: Pointer to head of the blocking notifier chain
+ * @val: Value passed unmodified to notifier function
+ * @v: Pointer passed unmodified to notifier function
+ * @nr_to_call: Number of notifier functions to be called. Don't care
+ * value of this parameter is -1.
+ * @nr_calls: Records the number of notifications sent. Don't care
+ * value of this field is NULL.
+ *
+ * RETURN VALUE: notifier_call_chain returns the value returned by the
+ * last notifier function called.
+ */
+
static int __kprobes notifier_call_chain(struct notifier_block **nl,
- unsigned long val, void *v)
+ unsigned long val, void *v,
+ int nr_to_call, unsigned int *nr_calls)
{
int ret = NOTIFY_DONE;
struct notifier_block *nb, *next_nb;

nb = rcu_dereference(*nl);
- while (nb) {
+
+ while (nb && nr_to_call) {
next_nb = rcu_dereference(nb->next);
ret = nb->notifier_call(nb, val, v);
+
+ if (nr_calls)
+ *nr_calls ++;
+
if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
break;
nb = next_nb;
+ nr_to_call--;
}
return ret;
}
@@ -205,10 +227,13 @@ int atomic_notifier_chain_unregister(str
EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);

/**
- * atomic_notifier_call_chain - Call functions in an atomic notifier chain
+ * __atomic_notifier_call_chain - Call functions in an atomic notifier
+ * chain
* @nh: Pointer to head of the atomic notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
+ * @nr_to_call: See the comment for notifier_call_chain.
+ * @nr_calls: See the comment for notifier_call_chain.
*
* Calls each function in a notifier chain in turn. The functions
* run in an atomic context, so they must not block.
@@ -222,19 +247,27 @@ EXPORT_SYMBOL_GPL(atomic_notifier_chain_
* of the last notifier function called.
*/

-int __kprobes atomic_notifier_call_chain(struct atomic_notifier_head *nh,
- unsigned long val, void *v)
+int __kprobes __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
+ unsigned long val, void *v,
+ int nr_to_call, unsigned int *nr_calls)
{
int ret;

rcu_read_lock();
- ret = notifier_call_chain(&nh->head, val, v);
+ ret = notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
rcu_read_unlock();
return ret;
}

-EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
+EXPORT_SYMBOL_GPL(__atomic_notifier_call_chain);

+int __kprobes atomic_notifier_call_chain(struct atomic_notifier_head *nh,
+ unsigned long val, void *v)
+{
+ return __atomic_notifier_call_chain(nh, val, v, -1, NULL);
+}
+
+EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
/*
* Blocking notifier chain routines. All access to the chain is
* synchronized by an rwsem.
@@ -304,10 +337,13 @@ int blocking_notifier_chain_unregister(s
EXPORT_SYMBOL_GPL(blocking_notifier_chain_unregister);

/**
- * blocking_notifier_call_chain - Call functions in a blocking notifier chain
+ * __blocking_notifier_call_chain - Call functions in a blocking notifier
+ * chain
* @nh: Pointer to head of the blocking notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
+ * @nr_to_call: See comment for notifier_call_chain.
+ * @nr_calls: See comment for notifier_call_chain.
*
* Calls each function in a notifier chain in turn. The functions
* run in a process context, so they are allowed to block.
@@ -320,17 +356,26 @@ EXPORT_SYMBOL_GPL(blocking_notifier_chai
* of the last notifier function called.
*/

-int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
- unsigned long val, void *v)
+int __blocking_notifier_call_chain(struct blocking_notifier_head *nh,
+ unsigned long val, void *v,
+ int nr_to_call, unsigned int * nr_calls)
{
int ret;

down_read(&nh->rwsem);
- ret = notifier_call_chain(&nh->head, val, v);
+ ret = notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
up_read(&nh->rwsem);
return ret;
}

+EXPORT_SYMBOL_GPL(__blocking_notifier_call_chain);
+
+int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
+ unsigned long val, void *v)
+{
+ return __blocking_notifier_call_chain(nh, val, v, -1, NULL);
+}
+
EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);

/*
@@ -376,10 +421,12 @@ int raw_notifier_chain_unregister(struct
EXPORT_SYMBOL_GPL(raw_notifier_chain_unregister);

/**
- * raw_notifier_call_chain - Call functions in a raw notifier chain
+ * __raw_notifier_call_chain - Call functions in a raw notifier chain
* @nh: Pointer to head of the raw notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
+ * @nr_to_call: See comment for notifier_call_chain.
+ * @nr_calls: See comment for notifier_call_chain
*
* Calls each function in a notifier chain in turn. The functions
* run in an undefined context.
@@ -393,10 +440,19 @@ EXPORT_SYMBOL_GPL(raw_notifier_chain_unr
* of the last notifier function called.
*/

+int __raw_notifier_call_chain(struct raw_notifier_head *nh,
+ unsigned long val, void *v,
+ int nr_to_call, unsigned int *nr_calls)
+{
+ return notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
+}
+
+EXPORT_SYMBOL_GPL(__raw_notifier_call_chain);
+
int raw_notifier_call_chain(struct raw_notifier_head *nh,
unsigned long val, void *v)
{
- return notifier_call_chain(&nh->head, val, v);
+ return __raw_notifier_call_chain(nh, val, v, -1, NULL);
}

EXPORT_SYMBOL_GPL(raw_notifier_call_chain);
@@ -475,6 +531,8 @@ EXPORT_SYMBOL_GPL(srcu_notifier_chain_un
* @nh: Pointer to head of the SRCU notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
+ * @nr_to_call: See comment for notifier_call_chain.
+ * @nr_calls: See comment for notifier_call_chain
*
* Calls each function in a notifier chain in turn. The functions
* run in a process context, so they are allowed to block.
@@ -487,18 +545,25 @@ EXPORT_SYMBOL_GPL(srcu_notifier_chain_un
* of the last notifier function called.
*/

-int srcu_notifier_call_chain(struct srcu_notifier_head *nh,
- unsigned long val, void *v)
+int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
+ unsigned long val, void *v,
+ int nr_to_call, unsigned int *nr_calls)
{
int ret;
int idx;

idx = srcu_read_lock(&nh->srcu);
- ret = notifier_call_chain(&nh->head, val, v);
+ ret = notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
srcu_read_unlock(&nh->srcu, idx);
return ret;
}
+EXPORT_SYMBOL_GPL(__srcu_notifier_call_chain);

+int srcu_notifier_call_chain(struct srcu_notifier_head *nh,
+ unsigned long val, void *v)
+{
+ return __srcu_notifier_call_chain(nh, val, v, -1, NULL);
+}
EXPORT_SYMBOL_GPL(srcu_notifier_call_chain);

/**
Index: hotplug/include/linux/notifier.h
===================================================================
--- hotplug.orig/include/linux/notifier.h
+++ hotplug/include/linux/notifier.h
@@ -132,12 +132,20 @@ extern int srcu_notifier_chain_unregiste

extern int atomic_notifier_call_chain(struct atomic_notifier_head *,
unsigned long val, void *v);
+extern int __atomic_notifier_call_chain(struct atomic_notifier_head *,
+ unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
extern int blocking_notifier_call_chain(struct blocking_notifier_head *,
unsigned long val, void *v);
+extern int __blocking_notifier_call_chain(struct blocking_notifier_head *,
+ unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
extern int raw_notifier_call_chain(struct raw_notifier_head *,
unsigned long val, void *v);
+extern int __raw_notifier_call_chain(struct raw_notifier_head *,
+ unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
extern int srcu_notifier_call_chain(struct srcu_notifier_head *,
unsigned long val, void *v);
+extern int __srcu_notifier_call_chain(struct srcu_notifier_head *,
+ unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);

#define NOTIFY_DONE 0x0000 /* Don't care */
#define NOTIFY_OK 0x0001 /* Suits me */
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

2006-11-14 18:18:52

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

On Tue, 14 Nov 2006 17:50:51 +0530 Gautham R Shenoy wrote:

> Provide notifier_call_chain with an option to call only a specified number of
> notifiers and also record the number of call to notifiers made.
>
> The need for this enhancement was identified in the post entitled
> "Slab - Eliminate lock_cpu_hotplug from slab"
> (http://lkml.org/lkml/2006/10/28/92) by Ravikiran G Thirumalai and
> Andrew Morton.
>
> This patch adds two additional parameters to notifier_call_chain API namely
> - int nr_to_calls : Number of notifier_functions to be called.
> The don't care value is -1.
>
> - unsigned int *nr_calls : Records the total number of notifier_funtions
> called by notifier_call_chain. The don't care
> value is NULL.

Those could (should?) be the same data type.

> Credit : Andrew Morton <[email protected]>
> Signed-off-by: Gautham R Shenoy <[email protected]>
>
> --
> include/linux/notifier.h | 8 +++
> kernel/sys.c | 97 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 89 insertions(+), 16 deletions(-)
>
> Index: hotplug/kernel/sys.c
> ===================================================================
> --- hotplug.orig/kernel/sys.c
> +++ hotplug/kernel/sys.c
> @@ -134,19 +134,41 @@ static int notifier_chain_unregister(str
> return -ENOENT;
> }
>
> +/*
> + * notifier_call_chain - Informs the registered notifiers about an event.
> + *
> + * @nl: Pointer to head of the blocking notifier chain
> + * @val: Value passed unmodified to notifier function
> + * @v: Pointer passed unmodified to notifier function
> + * @nr_to_call: Number of notifier functions to be called. Don't care
> + * value of this parameter is -1.
> + * @nr_calls: Records the number of notifications sent. Don't care
> + * value of this field is NULL.
> + *
> + * RETURN VALUE: notifier_call_chain returns the value returned by the
> + * last notifier function called.
> + */

You can make that comment block be kernel-doc format by using
/**
as the comment introduction and removing the blank line after the
function name & short description.

> static int __kprobes notifier_call_chain(struct notifier_block **nl,
> - unsigned long val, void *v)
> + unsigned long val, void *v,
> + int nr_to_call, unsigned int *nr_calls)
> {
> int ret = NOTIFY_DONE;
> struct notifier_block *nb, *next_nb;
...
> }
> @@ -205,10 +227,13 @@ int atomic_notifier_chain_unregister(str
> EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
>
> /**
> - * atomic_notifier_call_chain - Call functions in an atomic notifier chain
> + * __atomic_notifier_call_chain - Call functions in an atomic notifier
> + * chain

Don't break the short function description line; kernel-doc does not
support that.

> * @nh: Pointer to head of the atomic notifier chain
> * @val: Value passed unmodified to notifier function
> * @v: Pointer passed unmodified to notifier function
> + * @nr_to_call: See the comment for notifier_call_chain.
> + * @nr_calls: See the comment for notifier_call_chain.
> *
> * Calls each function in a notifier chain in turn. The functions
> * run in an atomic context, so they must not block.
> @@ -222,19 +247,27 @@ EXPORT_SYMBOL_GPL(atomic_notifier_chain_
> * of the last notifier function called.
> */
>
> -int __kprobes atomic_notifier_call_chain(struct atomic_notifier_head *nh,
> - unsigned long val, void *v)
> +int __kprobes __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
> + unsigned long val, void *v,
> + int nr_to_call, unsigned int *nr_calls)
> {
...
> }
>
> -EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
> +EXPORT_SYMBOL_GPL(__atomic_notifier_call_chain);
>
> +int __kprobes atomic_notifier_call_chain(struct atomic_notifier_head *nh,
> + unsigned long val, void *v)
> +{
> + return __atomic_notifier_call_chain(nh, val, v, -1, NULL);
> +}
> +
> +EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
> /*
> * Blocking notifier chain routines. All access to the chain is
> * synchronized by an rwsem.
> @@ -304,10 +337,13 @@ int blocking_notifier_chain_unregister(s
> EXPORT_SYMBOL_GPL(blocking_notifier_chain_unregister);
>
> /**
> - * blocking_notifier_call_chain - Call functions in a blocking notifier chain
> + * __blocking_notifier_call_chain - Call functions in a blocking notifier
> + * chain

kernel-doc requires that the function description fit on one source line,
so don't break it (even if it is > 80 columns; yes, I know it needs to be
fixed)

> * @nh: Pointer to head of the blocking notifier chain
> * @val: Value passed unmodified to notifier function
> * @v: Pointer passed unmodified to notifier function
> + * @nr_to_call: See comment for notifier_call_chain.
> + * @nr_calls: See comment for notifier_call_chain.
> *
> * Calls each function in a notifier chain in turn. The functions
> * run in a process context, so they are allowed to block.

> Index: hotplug/include/linux/notifier.h
> ===================================================================
> --- hotplug.orig/include/linux/notifier.h
> +++ hotplug/include/linux/notifier.h
> @@ -132,12 +132,20 @@ extern int srcu_notifier_chain_unregiste
>
> extern int atomic_notifier_call_chain(struct atomic_notifier_head *,
> unsigned long val, void *v);
> +extern int __atomic_notifier_call_chain(struct atomic_notifier_head *,

While you are changing these lines, please put a prototype parameter
name for all parameters; i.e., add something like "notifier"
or "nh" after the '*' on all of these.

> + unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
> extern int blocking_notifier_call_chain(struct blocking_notifier_head *,
> unsigned long val, void *v);
> +extern int __blocking_notifier_call_chain(struct blocking_notifier_head *,
> + unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
> extern int raw_notifier_call_chain(struct raw_notifier_head *,
> unsigned long val, void *v);
> +extern int __raw_notifier_call_chain(struct raw_notifier_head *,
> + unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
> extern int srcu_notifier_call_chain(struct srcu_notifier_head *,
> unsigned long val, void *v);
> +extern int __srcu_notifier_call_chain(struct srcu_notifier_head *,
> + unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
>
> #define NOTIFY_DONE 0x0000 /* Don't care */
> #define NOTIFY_OK 0x0001 /* Suits me */
> --

---
~Randy

2006-11-15 00:49:04

by Andrew Morton

[permalink] [raw]
Subject: Re: [RFC 0/4] Cpu-Hotplug: Use per subsystem hot-cpu mutexes.

On Tue, 14 Nov 2006 17:48:32 +0530
Gautham R Shenoy <[email protected]> wrote:

> Since 2.6.18-something, the community has been bugged by the problem to
> provide a clean and a stable mechanism to postpone a cpu-hotplug event
> as lock_cpu_hotplug was badly broken.
>
> This is another proposal towards solving that problem. This one is
> along the lines of the solution provided in kernel/workqueue.c

The approach seems sane to me. Sort-of direct, specific and transactional..

I applied this fixup:

diff -puN kernel/cpu.c~define-and-use-new-eventscpu_lock_acquire-and-cpu_lock_release-fix kernel/cpu.c
--- a/kernel/cpu.c~define-and-use-new-eventscpu_lock_acquire-and-cpu_lock_release-fix
+++ a/kernel/cpu.c
@@ -139,7 +139,8 @@ static int _cpu_down(unsigned int cpu)
if (err == NOTIFY_BAD) {
printk("%s: attempt to take down CPU %u failed\n",
__FUNCTION__, cpu);
- return -EINVAL;
+ err = -EINVAL;
+ goto out_release;
}

/* Ensure that we are not runnable on dying cpu */
@@ -187,6 +188,7 @@ out_thread:
err = kthread_stop(p);
out_allowed:
set_cpus_allowed(current, old_allowed);
+out_release:
raw_notifier_call_chain(&cpu_chain, CPU_LOCK_RELEASE,
(void *)(long)cpu);
return err;
_


please send a patch to fix up the kerneldoc things which Randy spotted,
thanks.

Subject: Re: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

On Tue, Nov 14, 2006 at 10:18:06AM -0800, Randy Dunlap wrote:
> On Tue, 14 Nov 2006 17:50:51 +0530 Gautham R Shenoy wrote:
>
> > Provide notifier_call_chain with an option to call only a specified number of
> > notifiers and also record the number of call to notifiers made.
> >
> > The need for this enhancement was identified in the post entitled
> > "Slab - Eliminate lock_cpu_hotplug from slab"
> > (http://lkml.org/lkml/2006/10/28/92) by Ravikiran G Thirumalai and
> > Andrew Morton.
> >
> > This patch adds two additional parameters to notifier_call_chain API namely
> > - int nr_to_calls : Number of notifier_functions to be called.
> > The don't care value is -1.
> >
> > - unsigned int *nr_calls : Records the total number of notifier_funtions
> > called by notifier_call_chain. The don't care
> > value is NULL.
>
> Those could (should?) be the same data type.

Whoops! Yes, they should be the same.
I was trying to solve this problem with only one parameter
(which we can, but the signature would look very confusing) and was
using the unsigned int* there. Will change it to int *.
Thanks for pointing that out.
>
> > Credit : Andrew Morton <[email protected]>
> > Signed-off-by: Gautham R Shenoy <[email protected]>
> >
> > --
> > include/linux/notifier.h | 8 +++
> > kernel/sys.c | 97 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 89 insertions(+), 16 deletions(-)
> >
> > Index: hotplug/kernel/sys.c
> > ===================================================================
> > --- hotplug.orig/kernel/sys.c
> > +++ hotplug/kernel/sys.c
> > @@ -134,19 +134,41 @@ static int notifier_chain_unregister(str
> > return -ENOENT;
> > }
> >
> > +/*
> > + * notifier_call_chain - Informs the registered notifiers about an event.
> > + *
> > + * @nl: Pointer to head of the blocking notifier chain
> > + * @val: Value passed unmodified to notifier function
> > + * @v: Pointer passed unmodified to notifier function
> > + * @nr_to_call: Number of notifier functions to be called. Don't care
> > + * value of this parameter is -1.
> > + * @nr_calls: Records the number of notifications sent. Don't care
> > + * value of this field is NULL.
> > + *
> > + * RETURN VALUE: notifier_call_chain returns the value returned by the
> > + * last notifier function called.
> > + */
>
> You can make that comment block be kernel-doc format by using
> /**
> as the comment introduction and removing the blank line after the
> function name & short description.

Will do that. But out of curiousity, do the comments of even static functions
get reflected in kernel doc ? :?

>
> > static int __kprobes notifier_call_chain(struct notifier_block **nl,
> > - unsigned long val, void *v)
> > + unsigned long val, void *v,
> > + int nr_to_call, unsigned int *nr_calls)
> > {
> > int ret = NOTIFY_DONE;
> > struct notifier_block *nb, *next_nb;
> ...
> > }
> > @@ -205,10 +227,13 @@ int atomic_notifier_chain_unregister(str
> > EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);
> >
> > /**
> > - * atomic_notifier_call_chain - Call functions in an atomic notifier chain
> > + * __atomic_notifier_call_chain - Call functions in an atomic notifier
> > + * chain
>
> Don't break the short function description line; kernel-doc does not
> support that.

Ok. Didn't know that. Will correct it.

>
> > * @nh: Pointer to head of the atomic notifier chain

[snip!]

> > Index: hotplug/include/linux/notifier.h
> > ===================================================================
> > --- hotplug.orig/include/linux/notifier.h
> > +++ hotplug/include/linux/notifier.h
> > @@ -132,12 +132,20 @@ extern int srcu_notifier_chain_unregiste
> >
> > extern int atomic_notifier_call_chain(struct atomic_notifier_head *,
> > unsigned long val, void *v);
> > +extern int __atomic_notifier_call_chain(struct atomic_notifier_head *,
>
> While you are changing these lines, please put a prototype parameter
> name for all parameters; i.e., add something like "notifier"
> or "nh" after the '*' on all of these.
>

Yup. Will do.


> ---
> ~Randy

thanks,
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

2006-11-15 06:01:48

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

Gautham R Shenoy wrote:
> On Tue, Nov 14, 2006 at 10:18:06AM -0800, Randy Dunlap wrote:
>> On Tue, 14 Nov 2006 17:50:51 +0530 Gautham R Shenoy wrote:
>>
>>> include/linux/notifier.h | 8 +++
>>> kernel/sys.c | 97 +++++++++++++++++++++++++++++++++++++++-------- 2 files changed, 89 insertions(+), 16 deletions(-)
>>>
>>> Index: hotplug/kernel/sys.c
>>> ===================================================================
>>> --- hotplug.orig/kernel/sys.c
>>> +++ hotplug/kernel/sys.c
>>> @@ -134,19 +134,41 @@ static int notifier_chain_unregister(str
>>> return -ENOENT;
>>> }
>>>
>>> +/*
>>> + * notifier_call_chain - Informs the registered notifiers about an event.
>>> + *
>>> + * @nl: Pointer to head of the blocking notifier chain
>>> + * @val: Value passed unmodified to notifier function
>>> + * @v: Pointer passed unmodified to notifier function
>>> + * @nr_to_call: Number of notifier functions to be called. Don't care
>>> + * value of this parameter is -1.
>>> + * @nr_calls: Records the number of notifications sent. Don't care
>>> + * value of this field is NULL.
>>> + *
>>> + * RETURN VALUE: notifier_call_chain returns the value returned by the
>>> + * last notifier function called.
>>> + */
>> You can make that comment block be kernel-doc format by using
>> /**
>> as the comment introduction and removing the blank line after the
>> function name & short description.
>
> Will do that. But out of curiousity, do the comments of even static functions
> get reflected in kernel doc ? :?

They can be, but static functions are low priority for kernel-doc
IMO, so it's not a big deal to me if you don't make that be kernel-doc.

>>> static int __kprobes notifier_call_chain(struct notifier_block **nl,
>>> - unsigned long val, void *v)
>>> + unsigned long val, void *v,
>>> + int nr_to_call, unsigned int *nr_calls)
>>> {
>>> int ret = NOTIFY_DONE;
>>> struct notifier_block *nb, *next_nb;
>> ...
>>> }

Thanks,
--
~Randy

Subject: [PATCH 1-fix/4] Fix extend notifier_call_chain to count nr_calls made.

Hi Andrew,

Please apply the following patch on top of
extend-notifier_call_chain-to-count-nr_calls-made.patch.
This patch incorporates the Randy's suggestions.

thanks,
gautham.


This patch

* Corrects the type of nr_calls to int * from unsigned int * in
notifier_call_chain and it's subsequent callers.

* Converts comments of notifier_call_chain to be compliant with kernel-docs
standards.

*Reverts the changes made to the comments of other *_notifier_call_chain.

*Adds parameter names to a few functions prototypes in
include/linux/notifier.h .

Depends on patch: extend-notifier_call_chain-to-count-nr_calls-made.patch


Cc: Randy Dunlap <[email protected]>
Signed-off-by: Gautham R Shenoy <[email protected]>

--
include/linux/notifier.h | 58 +++++++++++++++++++++++------------------------
kernel/sys.c | 24 ++++++++-----------
2 files changed, 39 insertions(+), 43 deletions(-)

Index: hotplug/kernel/sys.c
===================================================================
--- hotplug.orig/kernel/sys.c
+++ hotplug/kernel/sys.c
@@ -134,9 +134,8 @@ static int notifier_chain_unregister(str
return -ENOENT;
}

-/*
+/**
* notifier_call_chain - Informs the registered notifiers about an event.
- *
* @nl: Pointer to head of the blocking notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
@@ -144,14 +143,13 @@ static int notifier_chain_unregister(str
* value of this parameter is -1.
* @nr_calls: Records the number of notifications sent. Don't care
* value of this field is NULL.
- *
- * RETURN VALUE: notifier_call_chain returns the value returned by the
+ * @returns: notifier_call_chain returns the value returned by the
* last notifier function called.
*/

static int __kprobes notifier_call_chain(struct notifier_block **nl,
unsigned long val, void *v,
- int nr_to_call, unsigned int *nr_calls)
+ int nr_to_call, int *nr_calls)
{
int ret = NOTIFY_DONE;
struct notifier_block *nb, *next_nb;
@@ -227,8 +225,7 @@ int atomic_notifier_chain_unregister(str
EXPORT_SYMBOL_GPL(atomic_notifier_chain_unregister);

/**
- * __atomic_notifier_call_chain - Call functions in an atomic notifier
- * chain
+ * __atomic_notifier_call_chain - Call functions in an atomic notifier chain
* @nh: Pointer to head of the atomic notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
@@ -249,7 +246,7 @@ EXPORT_SYMBOL_GPL(atomic_notifier_chain_

int __kprobes __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
unsigned long val, void *v,
- int nr_to_call, unsigned int *nr_calls)
+ int nr_to_call, int *nr_calls)
{
int ret;

@@ -337,8 +334,7 @@ int blocking_notifier_chain_unregister(s
EXPORT_SYMBOL_GPL(blocking_notifier_chain_unregister);

/**
- * __blocking_notifier_call_chain - Call functions in a blocking notifier
- * chain
+ * __blocking_notifier_call_chain - Call functions in a blocking notifier chain
* @nh: Pointer to head of the blocking notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
@@ -358,7 +354,7 @@ EXPORT_SYMBOL_GPL(blocking_notifier_chai

int __blocking_notifier_call_chain(struct blocking_notifier_head *nh,
unsigned long val, void *v,
- int nr_to_call, unsigned int * nr_calls)
+ int nr_to_call, int *nr_calls)
{
int ret;

@@ -442,7 +438,7 @@ EXPORT_SYMBOL_GPL(raw_notifier_chain_unr

int __raw_notifier_call_chain(struct raw_notifier_head *nh,
unsigned long val, void *v,
- int nr_to_call, unsigned int *nr_calls)
+ int nr_to_call, int *nr_calls)
{
return notifier_call_chain(&nh->head, val, v, nr_to_call, nr_calls);
}
@@ -527,7 +523,7 @@ int srcu_notifier_chain_unregister(struc
EXPORT_SYMBOL_GPL(srcu_notifier_chain_unregister);

/**
- * srcu_notifier_call_chain - Call functions in an SRCU notifier chain
+ * __srcu_notifier_call_chain - Call functions in an SRCU notifier chain
* @nh: Pointer to head of the SRCU notifier chain
* @val: Value passed unmodified to notifier function
* @v: Pointer passed unmodified to notifier function
@@ -547,7 +543,7 @@ EXPORT_SYMBOL_GPL(srcu_notifier_chain_un

int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
unsigned long val, void *v,
- int nr_to_call, unsigned int *nr_calls)
+ int nr_to_call, int *nr_calls)
{
int ret;
int idx;
Index: hotplug/include/linux/notifier.h
===================================================================
--- hotplug.orig/include/linux/notifier.h
+++ hotplug/include/linux/notifier.h
@@ -112,40 +112,40 @@ extern void srcu_init_notifier_head(stru

#ifdef __KERNEL__

-extern int atomic_notifier_chain_register(struct atomic_notifier_head *,
- struct notifier_block *);
-extern int blocking_notifier_chain_register(struct blocking_notifier_head *,
- struct notifier_block *);
-extern int raw_notifier_chain_register(struct raw_notifier_head *,
- struct notifier_block *);
-extern int srcu_notifier_chain_register(struct srcu_notifier_head *,
- struct notifier_block *);
-
-extern int atomic_notifier_chain_unregister(struct atomic_notifier_head *,
- struct notifier_block *);
-extern int blocking_notifier_chain_unregister(struct blocking_notifier_head *,
- struct notifier_block *);
-extern int raw_notifier_chain_unregister(struct raw_notifier_head *,
- struct notifier_block *);
-extern int srcu_notifier_chain_unregister(struct srcu_notifier_head *,
- struct notifier_block *);
+extern int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
+ struct notifier_block *nb);
+extern int blocking_notifier_chain_register(struct blocking_notifier_head *nh,
+ struct notifier_block *nb);
+extern int raw_notifier_chain_register(struct raw_notifier_head *nh,
+ struct notifier_block *nb);
+extern int srcu_notifier_chain_register(struct srcu_notifier_head *nh,
+ struct notifier_block *nb);
+
+extern int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
+ struct notifier_block *nb);
+extern int blocking_notifier_chain_unregister(struct blocking_notifier_head *nh,
+ struct notifier_block *nb);
+extern int raw_notifier_chain_unregister(struct raw_notifier_head *nh,
+ struct notifier_block *nb);
+extern int srcu_notifier_chain_unregister(struct srcu_notifier_head *nh,
+ struct notifier_block *nb);

-extern int atomic_notifier_call_chain(struct atomic_notifier_head *,
+extern int atomic_notifier_call_chain(struct atomic_notifier_head *nh,
unsigned long val, void *v);
-extern int __atomic_notifier_call_chain(struct atomic_notifier_head *,
- unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
-extern int blocking_notifier_call_chain(struct blocking_notifier_head *,
+extern int __atomic_notifier_call_chain(struct atomic_notifier_head *nh,
+ unsigned long val, void *v, int nr_to_call, int *nr_calls);
+extern int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
unsigned long val, void *v);
-extern int __blocking_notifier_call_chain(struct blocking_notifier_head *,
- unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
-extern int raw_notifier_call_chain(struct raw_notifier_head *,
+extern int __blocking_notifier_call_chain(struct blocking_notifier_head *nh,
+ unsigned long val, void *v, int nr_to_call, int *nr_calls);
+extern int raw_notifier_call_chain(struct raw_notifier_head *nh,
unsigned long val, void *v);
-extern int __raw_notifier_call_chain(struct raw_notifier_head *,
- unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
-extern int srcu_notifier_call_chain(struct srcu_notifier_head *,
+extern int __raw_notifier_call_chain(struct raw_notifier_head *nh,
+ unsigned long val, void *v, int nr_to_call, int *nr_calls);
+extern int srcu_notifier_call_chain(struct srcu_notifier_head *nh,
unsigned long val, void *v);
-extern int __srcu_notifier_call_chain(struct srcu_notifier_head *,
- unsigned long val, void *v, int nr_to_call, unsigned int *nr_calls);
+extern int __srcu_notifier_call_chain(struct srcu_notifier_head *nh,
+ unsigned long val, void *v, int nr_to_call, int *nr_calls);

#define NOTIFY_DONE 0x0000 /* Don't care */
#define NOTIFY_OK 0x0001 /* Suits me */
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

2006-11-21 06:20:22

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

On Tue, 14 Nov 2006 17:50:51 +0530
Gautham R Shenoy <[email protected]> wrote:

> Provide notifier_call_chain with an option to call only a specified number of
> notifiers and also record the number of call to notifiers made.
>
> The need for this enhancement was identified in the post entitled
> "Slab - Eliminate lock_cpu_hotplug from slab"
> (http://lkml.org/lkml/2006/10/28/92) by Ravikiran G Thirumalai and
> Andrew Morton.
>
> This patch adds two additional parameters to notifier_call_chain API namely
> - int nr_to_calls : Number of notifier_functions to be called.
> The don't care value is -1.
>
> - unsigned int *nr_calls : Records the total number of notifier_funtions
> called by notifier_call_chain. The don't care
> value is NULL.
>
> ...
>
> +
> static int __kprobes notifier_call_chain(struct notifier_block **nl,
> - unsigned long val, void *v)
> + unsigned long val, void *v,
> + int nr_to_call, unsigned int *nr_calls)
> {
> int ret = NOTIFY_DONE;
> struct notifier_block *nb, *next_nb;
>
> nb = rcu_dereference(*nl);
> - while (nb) {
> +
> + while (nb && nr_to_call) {
> next_nb = rcu_dereference(nb->next);
> ret = nb->notifier_call(nb, val, v);
> +
> + if (nr_calls)
> + *nr_calls ++;

This gets

kernel/sys.c: In function 'notifier_call_chain':
kernel/sys.c:164: warning: value computed is not used


And indeed, this code doesn't work.

What happened?

Subject: Re: [PATCH 1/4] Extend notifier_call_chain to count nr_calls made.

Hi Andrew,

On Mon, Nov 20, 2006 at 10:19:41PM -0800, Andrew Morton wrote:

> > +
> > + if (nr_calls)
> > + *nr_calls ++;
>
> This gets
>
> kernel/sys.c: In function 'notifier_call_chain':
> kernel/sys.c:164: warning: value computed is not used
>
>
> And indeed, this code doesn't work.
>
> What happened?

I didn't get the warnings because my test box still has the prehistoric
version 3.4.4 of gcc.
I compiled the code with gcc 4.1.1 and got the warnings.

The code does not work because of my carelessness.
It should have been (*nr_calls)++ in the first place. I apologise.

Thanks for fixing it.

Regards
gautham.
--
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"