Hi,
the following three patches fix two lockdep warnings I am receiving with
2.6.18-rc6-mm2 (but at least the first one has been already discussed in
the times of 2.6.17, reported by Dave Jones) and I can see the problem in
current mainline source too).
* [1/3] fixes this:
=============================================
[ INFO: possible recursive locking detected ]
2.6.18-rc6-mm2-dirty #4
---------------------------------------------
kseriod/140 is trying to acquire lock:
(&ps2dev->cmd_mutex/1){--..}, at: [<c02b973b>] ps2_command+0x5b/0x3a0
but task is already holding lock:
(&ps2dev->cmd_mutex/1){--..}, at: [<c02b973b>] ps2_command+0x5b/0x3a0
* [2/3] adds support for spin_lock_irqsave_nested(), which is needed by
[3/3]
* [3/3] fixes this:
=============================================
[ INFO: possible recursive locking detected ]
2.6.18-rc6-mm2-dirty #7
---------------------------------------------
swapper/0 is trying to acquire lock:
(&serio->lock){++..}, at: [<c02b7a20>] serio_interrupt+0x20/0x60
but task is already holding lock:
(&serio->lock){++..}, at: [<c02b7a20>] serio_interrupt+0x20/0x60
All three patches are based against 2.6.18-rc6-mm2, I can rebase them
against mainline, if needed.
Both warnings have been solved by splitting the respective functions to
nested and non-nested variants, and calling them from synpatics driver as
appropriate.
--
JiKos.
This patch introduces spin_lock_irqsave_nested(), as it is needed when
annotating nested irqsave-spinlocks for lockdep.
This is needed to cope with serio_interrupt() being recursively called
from synaptics_pass_pt_packet(). More users could arise.
Implementation stolen from Arjan [1], whose patch didn't make it neither
into mainline nor -mm.
If applicable, please apply.
[1] http://lkml.org/lkml/2006/6/1/122
Signed-off-by: Jiri Kosina <[email protected]>
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h 2006-09-14 01:24:08.000000000 +0200
@@ -32,6 +32,8 @@ void __lockfunc _read_lock_irq(rwlock_t
void __lockfunc _write_lock_irq(rwlock_t *lock) __acquires(lock);
unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
__acquires(lock);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+ __acquires(spinlock_t);
unsigned long __lockfunc _read_lock_irqsave(rwlock_t *lock)
__acquires(lock);
unsigned long __lockfunc _write_lock_irqsave(rwlock_t *lock)
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h 2006-09-14 01:24:05.000000000 +0200
@@ -59,6 +59,7 @@
#define _read_lock_irq(lock) __LOCK_IRQ(lock)
#define _write_lock_irq(lock) __LOCK_IRQ(lock)
#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
+#define _spin_lock_irqsave_nested(lock, flags, subclass) __LOCK_IRQSAVE(lock, flags, subclass)
#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _spin_trylock(lock) ({ __LOCK(lock); 1; })
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h linux-2.6.18-rc6-mm2/include/linux/spinlock.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock.h 2006-09-14 01:24:12.000000000 +0200
@@ -186,6 +186,11 @@ do { \
#define spin_lock_irqsave(lock, flags) flags = _spin_lock_irqsave(lock)
#define read_lock_irqsave(lock, flags) flags = _read_lock_irqsave(lock)
#define write_lock_irqsave(lock, flags) flags = _write_lock_irqsave(lock)
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave_nested(lock, subclass)
+#else
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave(lock)
+#endif
#else
#define spin_lock_irqsave(lock, flags) _spin_lock_irqsave(lock, flags)
#define read_lock_irqsave(lock, flags) _read_lock_irqsave(lock, flags)
diff -rup linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c linux-2.6.18-rc6-mm2/kernel/spinlock.c
--- linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/kernel/spinlock.c 2006-09-14 01:27:17.000000000 +0200
@@ -304,6 +304,27 @@ void __lockfunc _spin_lock_nested(spinlo
}
EXPORT_SYMBOL(_spin_lock_nested);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ preempt_disable();
+ spin_acquire(&lock->dep_map, subtype, 0, _RET_IP_);
+ /*
+ * On lockdep we dont want the hand-coded irq-enable of
+ * _raw_spin_lock_flags() code, because lockdep assumes
+ * that interrupts are not re-enabled during lock-acquire:
+ */
+#ifdef CONFIG_PROVE_SPIN_LOCKING
+ _raw_spin_lock(lock);
+#else
+ _raw_spin_lock_flags(lock, &flags);
+#endif
+ return flags;
+}
+
+EXPORT_SYMBOL(_spin_lock_irqsave_nested);
#endif
=============================================
[ INFO: possible recursive locking detected ]
2.6.18-rc6-mm2-dirty #7
---------------------------------------------
swapper/0 is trying to acquire lock:
(&serio->lock){++..}, at: [<c02b7a20>] serio_interrupt+0x20/0x60
but task is already holding lock:
(&serio->lock){++..}, at: [<c02b7a20>] serio_interrupt+0x20/0x60
other info that might help us debug this:
1 lock held by swapper/0:
#0: (&serio->lock){++..}, at: [<c02b7a20>] serio_interrupt+0x20/0x60
stack backtrace:
[<c01039e5>] dump_trace+0x225/0x240
[<c0103af0>] show_trace_log_lvl+0x30/0x50
[<c0103b38>] show_trace+0x28/0x30
[<c0103c72>] dump_stack+0x22/0x30
[<c0135130>] print_deadlock_bug+0xc0/0xd0
[<c01351b2>] check_deadlock+0x72/0x80
[<c0136a4d>] __lock_acquire+0x43d/0x990
[<c0137468>] lock_acquire+0x68/0x80
[<c036947d>] _spin_lock_irqsave+0x4d/0x70
[<c02b7a20>] serio_interrupt+0x20/0x60
[<c02c3f84>] synaptics_pass_pt_packet+0x44/0xd0
[<c02c4a9a>] synaptics_process_byte+0xba/0xd0
[<c02c017a>] psmouse_handle_byte+0x1a/0x110
[<c02c031a>] psmouse_interrupt+0xaa/0x2e0
[<c02b79ca>] __serio_interrupt+0x3a/0x70
[<c02b7a42>] serio_interrupt+0x42/0x60
[<c02b841e>] i8042_interrupt+0x10e/0x240
[<c014cab1>] handle_IRQ_event+0x31/0x80
[<c014dc5e>] handle_level_irq+0x8e/0x120
[<c010570a>] do_IRQ+0x5a/0xb0
[<c01035a2>] common_interrupt+0x2e/0x34
[<c024e964>] acpi_processor_idle+0x1e8/0x31b
[<c0101195>] cpu_idle+0x95/0xa0
[<c0100334>] rest_init+0x44/0x50
[<c04ca8cd>] start_kernel+0x1cd/0x220
=======================
This is caused by synaptics driver, in pass-through port situation,
recursively calling serio_interrupt(), which acquires the device lock
recursively. This is however OK in the synaptics pass-through port case,
as the per-device lock is acquired exactly twice - first for child, then
for parent device, so no deadlock occurs.
The patch splits serio_interrupt() function into nested and non-nested
versions, and changes calls from synaptics pass-through packet handler
accordingly.
This patch depends on the existence of spin_lock_irqsave_nested(), which
is provided by the previous patch.
Please apply, if applicable, to get rid of spurious lockdep warnings. This patch
was generated against 2.6.18-rc6-mm2.
Signed-off-by: Jiri Kosina <[email protected]>
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/synaptics.c linux-2.6.18-rc6-mm2/drivers/input/mouse/synaptics.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/synaptics.c 2006-09-14 01:05:30.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/mouse/synaptics.c 2006-09-14 02:03:38.000000000 +0200
@@ -216,13 +216,13 @@ static void synaptics_pass_pt_packet(str
struct psmouse *child = serio_get_drvdata(ptport);
if (child && child->state == PSMOUSE_ACTIVATED) {
- serio_interrupt(ptport, packet[1], 0, NULL);
- serio_interrupt(ptport, packet[4], 0, NULL);
- serio_interrupt(ptport, packet[5], 0, NULL);
+ serio_interrupt_nested(ptport, packet[1], 0, NULL);
+ serio_interrupt_nested(ptport, packet[4], 0, NULL);
+ serio_interrupt_nested(ptport, packet[5], 0, NULL);
if (child->pktsize == 4)
- serio_interrupt(ptport, packet[2], 0, NULL);
+ serio_interrupt_nested(ptport, packet[2], 0, NULL);
} else
- serio_interrupt(ptport, packet[1], 0, NULL);
+ serio_interrupt_nested(ptport, packet[1], 0, NULL);
}
static void synaptics_pt_activate(struct psmouse *psmouse)
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c 2006-09-14 01:51:00.000000000 +0200
@@ -41,6 +41,7 @@ MODULE_DESCRIPTION("Serio abstraction co
MODULE_LICENSE("GPL");
EXPORT_SYMBOL(serio_interrupt);
+EXPORT_SYMBOL(serio_interrupt_nested);
EXPORT_SYMBOL(__serio_register_port);
EXPORT_SYMBOL(serio_unregister_port);
EXPORT_SYMBOL(serio_unregister_child_port);
@@ -910,14 +911,11 @@ void serio_close(struct serio *serio)
serio_set_drv(serio, NULL);
}
-irqreturn_t serio_interrupt(struct serio *serio,
+irqreturn_t __serio_interrupt(struct serio *serio,
unsigned char data, unsigned int dfl, struct pt_regs *regs)
{
- unsigned long flags;
irqreturn_t ret = IRQ_NONE;
- spin_lock_irqsave(&serio->lock, flags);
-
if (likely(serio->drv)) {
ret = serio->drv->interrupt(serio, data, dfl, regs);
} else if (!dfl && serio->registered) {
@@ -925,8 +923,30 @@ irqreturn_t serio_interrupt(struct serio
ret = IRQ_HANDLED;
}
+ return ret;
+}
+
+irqreturn_t serio_interrupt(struct serio *serio,
+ unsigned char data, unsigned int dfl, struct pt_regs *regs)
+{
+ unsigned long flags;
+ irqreturn_t ret;
+
+ spin_lock_irqsave(&serio->lock, flags);
+ ret = __serio_interrupt(serio, data, dfl, regs);
spin_unlock_irqrestore(&serio->lock, flags);
+ return ret;
+}
+irqreturn_t serio_interrupt_nested(struct serio *serio,
+ unsigned char data, unsigned int dfl, struct pt_regs *regs)
+{
+ unsigned long flags;
+ irqreturn_t ret;
+
+ spin_lock_irqsave_nested(&serio->lock, flags, SINGLE_DEPTH_NESTING);
+ ret = __serio_interrupt(serio, data, dfl, regs);
+ spin_unlock_irqrestore(&serio->lock, flags);
return ret;
}
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/serio.h linux-2.6.18-rc6-mm2/include/linux/serio.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/serio.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/serio.h 2006-09-14 01:39:57.000000000 +0200
@@ -76,6 +76,7 @@ void serio_close(struct serio *serio);
void serio_rescan(struct serio *serio);
void serio_reconnect(struct serio *serio);
irqreturn_t serio_interrupt(struct serio *serio, unsigned char data, unsigned int flags, struct pt_regs *regs);
+irqreturn_t serio_interrupt_nested(struct serio *serio, unsigned char data, unsigned int flags, struct pt_regs *regs);
void __serio_register_port(struct serio *serio, struct module *owner);
static inline void serio_register_port(struct serio *serio)
diff -rup linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c linux-2.6.18-rc6-mm2/kernel/spinlock.c
--- linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c 2006-09-14 01:35:00.000000000 +0200
+++ linux-2.6.18-rc6-mm2/kernel/spinlock.c 2006-09-14 01:42:40.000000000 +0200
@@ -310,7 +310,7 @@ unsigned long __lockfunc _spin_lock_irqs
local_irq_save(flags);
preempt_disable();
- spin_acquire(&lock->dep_map, subtype, 0, _RET_IP_);
+ spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
/*
* On lockdep we dont want the hand-coded irq-enable of
* _raw_spin_lock_flags() code, because lockdep assumes
=============================================
[ INFO: possible recursive locking detected ]
2.6.18-rc6-mm2-dirty #4
---------------------------------------------
kseriod/140 is trying to acquire lock:
(&ps2dev->cmd_mutex/1){--..}, at: [<c02b973b>] ps2_command+0x5b/0x3a0
but task is already holding lock:
(&ps2dev->cmd_mutex/1){--..}, at: [<c02b973b>] ps2_command+0x5b/0x3a0
other info that might help us debug this:
4 locks held by kseriod/140:
#0: (serio_mutex){--..}, at: [<c0367c85>] mutex_lock+0x25/0x30
#1: (&serio->drv_mutex){--..}, at: [<c0367c85>] mutex_lock+0x25/0x30
#2: (psmouse_mutex){--..}, at: [<c0367c85>] mutex_lock+0x25/0x30
#3: (&ps2dev->cmd_mutex/1){--..}, at: [<c02b973b>] ps2_command+0x5b/0x3a0
stack backtrace:
[<c01039e5>] dump_trace+0x225/0x240
[<c0103af0>] show_trace_log_lvl+0x30/0x50
[<c0103b38>] show_trace+0x28/0x30
[<c0103c72>] dump_stack+0x22/0x30
[<c0135130>] print_deadlock_bug+0xc0/0xd0
[<c01351b2>] check_deadlock+0x72/0x80
[<c0136a4d>] __lock_acquire+0x43d/0x990
[<c0137468>] lock_acquire+0x68/0x80
[<c0368003>] mutex_lock_nested+0x93/0x2e0
[<c02b973b>] ps2_command+0x5b/0x3a0
[<c02c03fd>] psmouse_sliced_command+0x2d/0x90
[<c02c3cef>] synaptics_pt_write+0x2f/0x70
[<c02b9466>] ps2_sendbyte+0x86/0x130
[<c02b97b4>] ps2_command+0xd4/0x3a0
[<c02c0ca9>] psmouse_probe+0x29/0xa0
[<c02c15e2>] psmouse_connect+0x122/0x270
[<c02b63ac>] serio_connect_driver+0x2c/0x50
[<c02b7574>] serio_driver_probe+0x24/0x30
[<c027c489>] really_probe+0xc9/0xf0
[<c027c533>] driver_probe_device+0x63/0xe0
[<c027c5c8>] __device_attach+0x18/0x20
[<c027b607>] bus_for_each_drv+0x57/0x80
[<c027c641>] device_attach+0x71/0x90
[<c027b883>] bus_attach_device+0x23/0x50
[<c0279d99>] device_add+0x219/0x390
[<c02b7081>] serio_add_port+0x51/0x100
[<c02b69a8>] serio_handle_event+0x78/0xa0
[<c02b6ae3>] serio_thread+0x23/0x110
[<c012e9c5>] kthread+0xa5/0xf0
[<c0103703>] kernel_thread_helper+0x7/0x14
This warning has already been discussed in [1]. The substitution
mutex_lock() for mutex_lock_nested() inside the ps2_command() is not
enough (which already is in mainline) - as this is purely recursive
situation (the same line of code acquires the lock twice), the
SINGLE_DEPTH_NESTING doesn't help (as the lock is even in such case the
same class).
The following patch fixes it. I agree that it is not the prettiest patch
on Earth, but definitely belongs to make-lockdep-shut-up patches
cathegory.
This arises with pass-through synaptics port, as in such situation, the
lock is acquired twice - first for child, then for parent device. I have
introduced respective variants of functions with _nested prefix, and let
the synaptics pass-through port driver call them, when performing
recursive locking for parent device.
The patch is against 2.6.18-rc6-mm2. If applicable, please apply, to get
rid of spurious lockdep warnings.
[1] http://lkml.org/lkml/2006/7/6/191
Signed-off-by: Jiri Kosina <[email protected]>
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/psmouse-base.c linux-2.6.18-rc6-mm2/drivers/input/mouse/psmouse-base.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/psmouse-base.c 2006-09-14 00:49:30.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/mouse/psmouse-base.c 2006-09-14 00:24:53.000000000 +0200
@@ -377,6 +377,22 @@ int psmouse_sliced_command(struct psmous
return 0;
}
+int psmouse_sliced_command_nested(struct psmouse *psmouse, unsigned char command)
+{
+ int i;
+
+ if (ps2_command_nested(&psmouse->ps2dev, NULL, PSMOUSE_CMD_SETSCALE11))
+ return -1;
+
+ for (i = 6; i >= 0; i -= 2) {
+ unsigned char d = (command >> i) & 3;
+ if (ps2_command_nested(&psmouse->ps2dev, &d, PSMOUSE_CMD_SETRES))
+ return -1;
+ }
+
+ return 0;
+}
+
/*
* psmouse_reset() resets the mouse into power-on state.
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/psmouse.h linux-2.6.18-rc6-mm2/drivers/input/mouse/psmouse.h
--- linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/psmouse.h 2006-09-14 00:49:30.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/mouse/psmouse.h 2006-09-14 00:26:06.000000000 +0200
@@ -91,6 +91,7 @@ enum psmouse_type {
};
int psmouse_sliced_command(struct psmouse *psmouse, unsigned char command);
+int psmouse_sliced_command_nested(struct psmouse *psmouse, unsigned char command);
int psmouse_reset(struct psmouse *psmouse);
void psmouse_set_resolution(struct psmouse *psmouse, unsigned int resolution);
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/synaptics.c linux-2.6.18-rc6-mm2/drivers/input/mouse/synaptics.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/mouse/synaptics.c 2006-09-14 00:49:30.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/mouse/synaptics.c 2006-09-14 00:20:16.000000000 +0200
@@ -199,9 +199,9 @@ static int synaptics_pt_write(struct ser
struct psmouse *parent = serio_get_drvdata(serio->parent);
char rate_param = SYN_PS_CLIENT_CMD; /* indicates that we want pass-through port */
- if (psmouse_sliced_command(parent, c))
+ if (psmouse_sliced_command_nested(parent, c))
return -1;
- if (ps2_command(&parent->ps2dev, &rate_param, PSMOUSE_CMD_SETRATE))
+ if (ps2_command_nested(&parent->ps2dev, &rate_param, PSMOUSE_CMD_SETRATE))
return -1;
return 0;
}
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c 2006-09-14 00:44:00.000000000 +0200
@@ -31,6 +31,7 @@ EXPORT_SYMBOL(ps2_init);
EXPORT_SYMBOL(ps2_sendbyte);
EXPORT_SYMBOL(ps2_drain);
EXPORT_SYMBOL(ps2_command);
+EXPORT_SYMBOL(ps2_command_nested);
EXPORT_SYMBOL(ps2_schedule_command);
EXPORT_SYMBOL(ps2_handle_ack);
EXPORT_SYMBOL(ps2_handle_response);
@@ -157,20 +158,10 @@ static int ps2_adjust_timeout(struct ps2
return timeout;
}
-/*
- * ps2_command() sends a command and its parameters to the mouse,
- * then waits for the response and puts it in the param array.
- *
- * ps2_command() can only be called from a process context
- */
-
-int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
+int ps2_command_validate(struct ps2dev *ps2dev, unsigned char *param, int command)
{
- int timeout;
int send = (command >> 12) & 0xf;
int receive = (command >> 8) & 0xf;
- int rc = -1;
- int i;
if (receive > sizeof(ps2dev->cmdbuf)) {
WARN_ON(1);
@@ -181,9 +172,24 @@ int ps2_command(struct ps2dev *ps2dev, u
WARN_ON(1);
return -1;
}
+ return 0;
+}
- mutex_lock_nested(&ps2dev->cmd_mutex, SINGLE_DEPTH_NESTING);
+/*
+ * ps2_command() sends a command and its parameters to the mouse,
+ * then waits for the response and puts it in the param array.
+ *
+ * ps2_command() can only be called from a process context
+ */
+int __ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
+{
+ int timeout;
+ int send = (command >> 12) & 0xf;
+ int receive = (command >> 8) & 0xf;
+ int rc = -1;
+ int i;
+
serio_pause_rx(ps2dev->serio);
ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
ps2dev->cmdcnt = receive;
@@ -236,6 +242,27 @@ int ps2_command(struct ps2dev *ps2dev, u
mutex_unlock(&ps2dev->cmd_mutex);
return rc;
+
+}
+
+int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command)
+{
+ if (ps2_command_validate(ps2dev, param, command) == -1)
+ return -1;
+
+ mutex_lock(&ps2dev->cmd_mutex);
+ return __ps2_command(ps2dev, param, command);
+
+}
+
+int ps2_command_nested(struct ps2dev *ps2dev, unsigned char *param, int command)
+{
+ if (ps2_command_validate(ps2dev, param, command) == -1)
+ return -1;
+
+ mutex_lock_nested(&ps2dev->cmd_mutex, SINGLE_DEPTH_NESTING);
+ return __ps2_command(ps2dev, param, command);
+
}
/*
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/libps2.h linux-2.6.18-rc6-mm2/include/linux/libps2.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/libps2.h 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/libps2.h 2006-09-14 00:22:13.000000000 +0200
@@ -43,6 +43,7 @@ void ps2_init(struct ps2dev *ps2dev, str
int ps2_sendbyte(struct ps2dev *ps2dev, unsigned char byte, int timeout);
void ps2_drain(struct ps2dev *ps2dev, int maxbytes, int timeout);
int ps2_command(struct ps2dev *ps2dev, unsigned char *param, int command);
+int ps2_command_nested(struct ps2dev *ps2dev, unsigned char *param, int command);
int ps2_schedule_command(struct ps2dev *ps2dev, unsigned char *param, int command);
int ps2_handle_ack(struct ps2dev *ps2dev, unsigned char data);
int ps2_handle_response(struct ps2dev *ps2dev, unsigned char data);
Hi Jiri,
On Wednesday 13 September 2006 20:44, Jiri Kosina wrote:
> Both warnings have been solved by splitting the respective functions to
> nested and non-nested variants, and calling them from synpatics driver as
> appropriate.
> ?
Unfortunately these patches do not solve the problem in general but
rather fix one specific codepath. As far as I can see the warnings will
return as soon as we add another pass-through port to the link (and I am
considering adding a pass-through port to the trackpoint driver so you
will get chain like i8042-synaptics-ptport-trackpoint-ptport-psmouse).
Plus they are ugly and complicate serio and psmouse cores. I really
don't like this *_nested business as it makes the code aware of possible
usage patterns instead of just being re-entrant.
--
Dmitry
On Wed, 13 Sep 2006, Dmitry Torokhov wrote:
> Unfortunately these patches do not solve the problem in general but
> rather fix one specific codepath. As far as I can see the warnings will
> return as soon as we add another pass-through port to the link (and I am
> considering adding a pass-through port to the trackpoint driver so you
> will get chain like i8042-synaptics-ptport-trackpoint-ptport-psmouse).
> Plus they are ugly and complicate serio and psmouse cores. I really
> don't like this *_nested business as it makes the code aware of possible
> usage patterns instead of just being re-entrant.
Hi Dmitry,
I agree that these patches are ugly, but I wasn't able to think of any
other way how to get rid of those lockdep warnings.
Of course the lock validator could be extended to provide API such as
mutex_init_nolockdep(), as you already proposed before, but this also has
it's drawbacks (for example if any other future user of ps2_init() uses
the mutex in a really bad way, this would not be detected by lock
validator).
Another possibility that comes to mind is extending the ps2dev structure
with a field which would work as an subclass identifier for the device,
and this field will be then be used as an subclass argument to
mutex_lock_nested(). However, this requires proper setting of this field
on the very same places on which my _nested functions are called, so it
has the same level of generality.
Do you have any other idea? I think this should get fixed, otherwise we
will keep receiving these reports from users again and again.
Thanks,
--
JiKos.
On 9/14/06, Jiri Kosina <[email protected]> wrote:
> On Wed, 13 Sep 2006, Dmitry Torokhov wrote:
>
> > Unfortunately these patches do not solve the problem in general but
> > rather fix one specific codepath. As far as I can see the warnings will
> > return as soon as we add another pass-through port to the link (and I am
> > considering adding a pass-through port to the trackpoint driver so you
> > will get chain like i8042-synaptics-ptport-trackpoint-ptport-psmouse).
> > Plus they are ugly and complicate serio and psmouse cores. I really
> > don't like this *_nested business as it makes the code aware of possible
> > usage patterns instead of just being re-entrant.
>
> Hi Dmitry,
>
> I agree that these patches are ugly, but I wasn't able to think of any
> other way how to get rid of those lockdep warnings.
>
> Of course the lock validator could be extended to provide API such as
> mutex_init_nolockdep(), as you already proposed before, but this also has
> it's drawbacks (for example if any other future user of ps2_init() uses
> the mutex in a really bad way, this would not be detected by lock
> validator).
>
> Another possibility that comes to mind is extending the ps2dev structure
> with a field which would work as an subclass identifier for the device,
> and this field will be then be used as an subclass argument to
> mutex_lock_nested(). However, this requires proper setting of this field
> on the very same places on which my _nested functions are called, so it
> has the same level of generality.
>
Can we add lock_class_key to the struct psmouse and use it to define
per-device mutex class regardless of whether it is a child, grandchild
or a parent?
> Do you have any other idea? I think this should get fixed, otherwise we
> will keep receiving these reports from users again and again.
>
If we can't make lockdep shut up with minimal intervention I might
just change psmouse back to semaphores.
--
Dmitry
On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> Can we add lock_class_key to the struct psmouse and use it to define
> per-device mutex class regardless of whether it is a child, grandchild
> or a parent?
Hi Dmitry,
what do you think about the patches below? I have used a slightly
different approach, as we also need to get rid of the spurious lockdep
warning in case of recursive call of serio_interrupt(), which can't be
handled well with lock subclass stored in struct psmouse. What do you
think about this? It shuts up the lockdep, and seems much cleaner to me.
The first patch implements spin_lock_irqsave_nested(), which is needed by
the second one:
Signed-off-by: Jiri Kosina <[email protected]>
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_smp.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_smp.h 2006-09-14 01:24:08.000000000 +0200
@@ -32,6 +32,8 @@ void __lockfunc _read_lock_irq(rwlock_t
void __lockfunc _write_lock_irq(rwlock_t *lock) __acquires(lock);
unsigned long __lockfunc _spin_lock_irqsave(spinlock_t *lock)
__acquires(lock);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+ __acquires(spinlock_t);
unsigned long __lockfunc _read_lock_irqsave(rwlock_t *lock)
__acquires(lock);
unsigned long __lockfunc _write_lock_irqsave(rwlock_t *lock)
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock_api_up.h 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock_api_up.h 2006-09-14 01:24:05.000000000 +0200
@@ -59,6 +59,7 @@
#define _read_lock_irq(lock) __LOCK_IRQ(lock)
#define _write_lock_irq(lock) __LOCK_IRQ(lock)
#define _spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
+#define _spin_lock_irqsave_nested(lock, flags, subclass) __LOCK_IRQSAVE(lock, flags, subclass)
#define _read_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _write_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
#define _spin_trylock(lock) ({ __LOCK(lock); 1; })
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h linux-2.6.18-rc6-mm2/include/linux/spinlock.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/spinlock.h 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/spinlock.h 2006-09-14 01:24:12.000000000 +0200
@@ -186,6 +186,11 @@ do { \
#define spin_lock_irqsave(lock, flags) flags = _spin_lock_irqsave(lock)
#define read_lock_irqsave(lock, flags) flags = _read_lock_irqsave(lock)
#define write_lock_irqsave(lock, flags) flags = _write_lock_irqsave(lock)
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave_nested(lock, subclass)
+#else
+#define spin_lock_irqsave_nested(lock, flags, subclass) flags = _spin_lock_irqsave(lock)
+#endif
#else
#define spin_lock_irqsave(lock, flags) _spin_lock_irqsave(lock, flags)
#define read_lock_irqsave(lock, flags) _read_lock_irqsave(lock, flags)
diff -rup linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c linux-2.6.18-rc6-mm2/kernel/spinlock.c
--- linux-2.6.18-rc6-mm2.orig/kernel/spinlock.c 2006-09-14 00:49:35.000000000 +0200
+++ linux-2.6.18-rc6-mm2/kernel/spinlock.c 2006-09-14 01:27:17.000000000 +0200
@@ -304,6 +304,27 @@ void __lockfunc _spin_lock_nested(spinlo
}
EXPORT_SYMBOL(_spin_lock_nested);
+unsigned long __lockfunc _spin_lock_irqsave_nested(spinlock_t *lock, int subclass)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+ preempt_disable();
+ spin_acquire(&lock->dep_map, subclass, 0, _RET_IP_);
+ /*
+ * On lockdep we dont want the hand-coded irq-enable of
+ * _raw_spin_lock_flags() code, because lockdep assumes
+ * that interrupts are not re-enabled during lock-acquire:
+ */
+#ifdef CONFIG_PROVE_SPIN_LOCKING
+ _raw_spin_lock(lock);
+#else
+ _raw_spin_lock_flags(lock, &flags);
+#endif
+ return flags;
+}
+
+EXPORT_SYMBOL(_spin_lock_irqsave_nested);
#endif
And this second one actually fixes the locking with respect to lockdep
validator:
Signed-off-by: Jiri Kosina <[email protected]>
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/libps2.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/libps2.c 2006-09-14 16:16:06.000000000 +0200
@@ -182,7 +182,7 @@ int ps2_command(struct ps2dev *ps2dev, u
return -1;
}
- mutex_lock_nested(&ps2dev->cmd_mutex, SINGLE_DEPTH_NESTING);
+ mutex_lock_nested(&ps2dev->cmd_mutex, ps2dev->serio->depth);
serio_pause_rx(ps2dev->serio);
ps2dev->flags = command == PS2_CMD_GETID ? PS2_FLAG_WAITID : 0;
diff -rup linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c
--- linux-2.6.18-rc6-mm2.orig/drivers/input/serio/serio.c 2006-09-04 04:19:48.000000000 +0200
+++ linux-2.6.18-rc6-mm2/drivers/input/serio/serio.c 2006-09-14 16:16:00.000000000 +0200
@@ -553,9 +553,11 @@ static void serio_add_port(struct serio
if (serio->parent) {
serio_pause_rx(serio->parent);
serio->parent->child = serio;
+ serio->depth = serio->parent->depth + 1;
serio_continue_rx(serio->parent);
- }
-
+ } else
+ serio->depth = 0;
+
list_add_tail(&serio->node, &serio_list);
if (serio->start)
serio->start(serio);
@@ -916,7 +918,7 @@ irqreturn_t serio_interrupt(struct serio
unsigned long flags;
irqreturn_t ret = IRQ_NONE;
- spin_lock_irqsave(&serio->lock, flags);
+ spin_lock_irqsave_nested(&serio->lock, flags, serio->depth);
if (likely(serio->drv)) {
ret = serio->drv->interrupt(serio, data, dfl, regs);
diff -rup linux-2.6.18-rc6-mm2.orig/include/linux/serio.h linux-2.6.18-rc6-mm2/include/linux/serio.h
--- linux-2.6.18-rc6-mm2.orig/include/linux/serio.h 2006-09-14 16:20:56.000000000 +0200
+++ linux-2.6.18-rc6-mm2/include/linux/serio.h 2006-09-14 15:15:41.000000000 +0200
@@ -41,6 +41,7 @@ struct serio {
void (*stop)(struct serio *);
struct serio *parent, *child;
+ unsigned int depth; /* level of nesting in parent-child hierarichy of serios */
struct serio_driver *drv; /* accessed from interrupt, must be protected by serio->lock and serio->sem */
struct mutex drv_mutex; /* protects serio->drv so attributes can pin driver */
--
JiKos.
On 9/14/06, Jiri Kosina <[email protected]> wrote:
> On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
>
> > Can we add lock_class_key to the struct psmouse and use it to define
> > per-device mutex class regardless of whether it is a child, grandchild
> > or a parent?
>
> Hi Dmitry,
>
> what do you think about the patches below? I have used a slightly
> different approach, as we also need to get rid of the spurious lockdep
> warning in case of recursive call of serio_interrupt(), which can't be
> handled well with lock subclass stored in struct psmouse. What do you
> think about this? It shuts up the lockdep, and seems much cleaner to me.
>
Yes, this is much, much better. Could you please tell me if depth
should be a true depth or just an unique number? The reason I am
asking is that I hope to get rid of parent/child pointers in serio
(they were introduced when driver core could not handle recursive
addition/removing of devices on the same bus).
--
Dmitry
On Thu, 2006-09-14 at 10:58 -0400, Dmitry Torokhov wrote:
> On 9/14/06, Jiri Kosina <[email protected]> wrote:
> > On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> >
> > > Can we add lock_class_key to the struct psmouse and use it to define
> > > per-device mutex class regardless of whether it is a child, grandchild
> > > or a parent?
> >
> > Hi Dmitry,
> >
> > what do you think about the patches below? I have used a slightly
> > different approach, as we also need to get rid of the spurious lockdep
> > warning in case of recursive call of serio_interrupt(), which can't be
> > handled well with lock subclass stored in struct psmouse. What do you
> > think about this? It shuts up the lockdep, and seems much cleaner to me.
> >
>
> Yes, this is much, much better. Could you please tell me if depth
> should be a true depth or just an unique number? The reason I am
> asking is that I hope to get rid of parent/child pointers in serio
> (they were introduced when driver core could not handle recursive
> addition/removing of devices on the same bus).
lockdep sort of expects the depth to be a number between 0 and 7.
Other than that lockdep does not assume an ordering based on numerical
value at all; it figures that out at runtime.
On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> Yes, this is much, much better. Could you please tell me if depth should
> be a true depth or just an unique number? The reason I am asking is that
> I hope to get rid of parent/child pointers in serio (they were
> introduced when driver core could not handle recursive addition/removing
> of devices on the same bus).
I am afraid you can't generate just any unique number to represent the
lock class, as the lockdep validator fails if the class number is higher
than MAX_LOCKDEP_SUBCLASSES, which is by default 8.
Regarding the patches - should I submit them upstream, or will you?
Thanks,
--
JiKos.
On 9/14/06, Jiri Kosina <[email protected]> wrote:
> On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
>
> > Yes, this is much, much better. Could you please tell me if depth should
> > be a true depth or just an unique number? The reason I am asking is that
> > I hope to get rid of parent/child pointers in serio (they were
> > introduced when driver core could not handle recursive addition/removing
> > of devices on the same bus).
>
> I am afraid you can't generate just any unique number to represent the
> lock class, as the lockdep validator fails if the class number is higher
> than MAX_LOCKDEP_SUBCLASSES, which is by default 8.
>
> Regarding the patches - should I submit them upstream, or will you?
>
Not yet ;) Is there a way to hide the depth in the spinlock/mutex
structure itself so that initialization code could do
spin_lock_init_nested() and spare the rest of the code from that
knowledge?
--
Dmitry
On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> Not yet ;) Is there a way to hide the depth in the spinlock/mutex
> structure itself so that initialization code could do
> spin_lock_init_nested() and spare the rest of the code from that
> knowledge?
(shortened CC list a bit)
In fact I am not sure what you mean. On every lock and unlock operation,
in case of recursive locking (which our case is), you have to provide
class identifier, which is used to distinguish if the lock is of the same
instance, or a different one (deeper or higher in the locking hierarchy).
There is no way how spin_lock() or mutex_lock() can know this
"automatically", you always have to provide the nesting level from
outside, as it depends on the ordering hierarchy, which locking primitives
are totally unaware of.
Or did I misunderstand you?
Thanks,
--
JiKos.
On 9/14/06, Jiri Kosina <[email protected]> wrote:
> On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
>
> > Not yet ;) Is there a way to hide the depth in the spinlock/mutex
> > structure itself so that initialization code could do
> > spin_lock_init_nested() and spare the rest of the code from that
> > knowledge?
>
> (shortened CC list a bit)
>
> In fact I am not sure what you mean. On every lock and unlock operation,
> in case of recursive locking (which our case is), you have to provide
> class identifier, which is used to distinguish if the lock is of the same
> instance, or a different one (deeper or higher in the locking hierarchy).
> There is no way how spin_lock() or mutex_lock() can know this
> "automatically", you always have to provide the nesting level from
> outside, as it depends on the ordering hierarchy, which locking primitives
> are totally unaware of.
>
Well, we do not really care about nestiness do we? What we trying to
achieve is to teach lockdep that 2 locks while appear as the lame lock
in fact are different and protect 2 different structures. Ideally
lockdep should track every lock individually (based for example on its
address) but that would be too taxing so we need to help it. In your
implementation you embed this data into structure/code using lock, but
this information could be instilled into the lock itself upon
initialization and spin_[un]lock() implementation could be taught to
use this data thus making specialized spin_[un]lock*_nested()
functions unnecessary.
--
Dmitry
On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
> Well, we do not really care about nestiness do we? What we trying to
> achieve is to teach lockdep that 2 locks while appear as the lame lock
> in fact are different and protect 2 different structures. Ideally
> lockdep should track every lock individually (based for example on its
> address) but that would be too taxing so we need to help it. In your
> implementation you embed this data into structure/code using lock, but
> this information could be instilled into the lock itself upon
> initialization and spin_[un]lock() implementation could be taught to use
> this data thus making specialized spin_[un]lock*_nested() functions
> unnecessary.
Hi Dmitry,
IMHO this is exactly what the nested locking primitives were introduced in
lockdep for (we even have natural hierarchy here), so I am not sure if
this is compliant with lockdep design. I definitely could do a patch that
would introduce {spin,mutex..}_lock_init_subclass(), which would
initialize the lock together with defining it's 'class', so that it could
be distinguishable from any other lock of the same type during proving of
correctness ... but this is a step towards distinguishing every single
lock from all others (even of a same type), which I am not sure is the
right direction.
I added Ingo to CC.
Thanks,
--
JiKos.
On 9/14/06, Jiri Kosina <[email protected]> wrote:
> On Thu, 14 Sep 2006, Dmitry Torokhov wrote:
>
> > Well, we do not really care about nestiness do we? What we trying to
> > achieve is to teach lockdep that 2 locks while appear as the lame lock
> > in fact are different and protect 2 different structures. Ideally
> > lockdep should track every lock individually (based for example on its
> > address) but that would be too taxing so we need to help it. In your
> > implementation you embed this data into structure/code using lock, but
> > this information could be instilled into the lock itself upon
> > initialization and spin_[un]lock() implementation could be taught to use
> > this data thus making specialized spin_[un]lock*_nested() functions
> > unnecessary.
>
> Hi Dmitry,
>
> IMHO this is exactly what the nested locking primitives were introduced in
> lockdep for (we even have natural hierarchy here), so I am not sure if
> this is compliant with lockdep design. I definitely could do a patch that
> would introduce {spin,mutex..}_lock_init_subclass(), which would
> initialize the lock together with defining it's 'class', so that it could
> be distinguishable from any other lock of the same type during proving of
> correctness ... but this is a step towards distinguishing every single
> lock from all others (even of a same type), which I am not sure is the
> right direction.
>
I think it is - as far as I understand the reason for not tracking
every lock individually is just that it is too expensive to do by
default.
--
Dmitry
>
> I think it is - as far as I understand the reason for not tracking
> every lock individually is just that it is too expensive to do by
> default.
that is not correct. While it certainly plays a role,
the other reason is that you can find out "class" level locking rules
(such as inode->i_mutex comes before <other lock>) for all inodes at a
time; eg no need to see every inode do this before you can find the
deadlock.
On 9/14/06, Arjan van de Ven <[email protected]> wrote:
>
> >
> > I think it is - as far as I understand the reason for not tracking
> > every lock individually is just that it is too expensive to do by
> > default.
>
> that is not correct. While it certainly plays a role,
> the other reason is that you can find out "class" level locking rules
> (such as inode->i_mutex comes before <other lock>) for all inodes at a
> time; eg no need to see every inode do this before you can find the
> deadlock.
>
OK, I can see that. However you must agree that for certain locks we
do want to track them individually, right?
--
Dmitry
* Dmitry Torokhov <[email protected]> wrote:
> I think it is - as far as I understand the reason for not tracking
> every lock individually is just that it is too expensive to do by
> default.
that is not at all the reason! The reason is that we want to find
deadlocks _as early as mathematically possible_ (in a running system,
where locking patterns are observed). That is we want to gather the
_most generic_ locking rules.
For example, if there are lock_1A, lock_1B of the same lock class, and
lock_2A and lock_2B of another lock class. If we observed the following
usage patterns:
acquire(lock_1A);
acquire(lock_2A);
release(lock_2A);
release(lock_1A);
and another piece of kernel code did:
acquire(lock_2B);
acquire(lock_1B);
release(lock_1B);
release(lock_1B);
with per-lock rules there's no problem detected, because the 4 locks are
independent and we only observed a 1A->2A and a 2B->1B dependency.
But with per-class rule gather we'd observe the 1->2 and the 2->1
dependency, and we'd warn that there's a deadlock.
So we want to create as broad, as generic rules as possible, to catch
deadlocks as soon as it's _provable_ that they could occur. In that
sense lockdep wants to have a '100% proof' of correctness: the first
time a bad even happens we flag it.
Ingo
On Thu, 2006-09-14 at 15:11 -0400, Dmitry Torokhov wrote:
> On 9/14/06, Arjan van de Ven <[email protected]> wrote:
> >
> > >
> > > I think it is - as far as I understand the reason for not tracking
> > > every lock individually is just that it is too expensive to do by
> > > default.
> >
> > that is not correct. While it certainly plays a role,
> > the other reason is that you can find out "class" level locking rules
> > (such as inode->i_mutex comes before <other lock>) for all inodes at a
> > time; eg no need to see every inode do this before you can find the
> > deadlock.
> >
>
> OK, I can see that. However you must agree that for certain locks we
> do want to track them individually, right?
I agree that if locks really represent different objects with different
locking semantics they should not share the class. Lockdep provides a
mechanism for that; however I'm very afraid that for the input layer,
they really are not that, they are not different objects with different
semantics; they are the same objects with nesting semantics! In that
case the "separate lock class" stuff has only disadvantages.
The worst thing is that as I understand it this separate class is
*dynamic*. Eg it's not even "one class per driver" ;(
On 9/15/06, Arjan van de Ven <[email protected]> wrote:
> On Thu, 2006-09-14 at 15:11 -0400, Dmitry Torokhov wrote:
> > On 9/14/06, Arjan van de Ven <[email protected]> wrote:
> > >
> > > >
> > > > I think it is - as far as I understand the reason for not tracking
> > > > every lock individually is just that it is too expensive to do by
> > > > default.
> > >
> > > that is not correct. While it certainly plays a role,
> > > the other reason is that you can find out "class" level locking rules
> > > (such as inode->i_mutex comes before <other lock>) for all inodes at a
> > > time; eg no need to see every inode do this before you can find the
> > > deadlock.
> > >
> >
> > OK, I can see that. However you must agree that for certain locks we
> > do want to track them individually, right?
>
> I agree that if locks really represent different objects with different
> locking semantics they should not share the class. Lockdep provides a
> mechanism for that; however I'm very afraid that for the input layer,
> they really are not that, they are not different objects with different
> semantics; they are the same objects with nesting semantics! In that
> case the "separate lock class" stuff has only disadvantages.
I'd say they are different objects with the same semantics...
> The worst thing is that as I understand it this separate class is
> *dynamic*. Eg it's not even "one class per driver" ;(
>
You are saying this as if was a bad thing. Pass-through ports just
implement PS/2 over PS/2 protocols and as such it is very natural that
the same driver that serves parent serves the child as well. That was
the goal - to reuse psmouse module instead of re-implementing all
re-probing and protocol decoding in synaptics driver. And trackpint
driver. And maybe somethng else down the road.
I also wonder that even if we had several drivers lockdep would still
complain about nestiness just because all PS/2 devices are initialized
via ps2_init (which initializes command mutex) and end up in the same
lock class.
I suspect that other driver implementing X-over-X or X-over-Y-over-X
may get hit the same way by lockdep.
I understand what Ingo is saying about detecting deadlocks across the
pool of locks of the same class not waiting till they really clash, it
is really useful. I also want to make my code as independent of
lockdep as possible. Having a speciall marking on the locks themselves
(done upon creation) instead of altering call sites is the cleanest
way IMHO. Can we have a flag in the lock structure that would tell
lockdep that it is OK for the given lock to be taken several times
(i.e. the locks are really on the different objects)? This would still
allow to detect incorrect locking across different classes.
--
Dmitry
On Fri, 15 Sep 2006, Dmitry Torokhov wrote:
> I understand what Ingo is saying about detecting deadlocks across the
> pool of locks of the same class not waiting till they really clash, it
> is really useful. I also want to make my code as independent of lockdep
> as possible. Having a speciall marking on the locks themselves (done
> upon creation) instead of altering call sites is the cleanest way IMHO.
> Can we have a flag in the lock structure that would tell lockdep that it
> is OK for the given lock to be taken several times (i.e. the locks are
> really on the different objects)? This would still allow to detect
> incorrect locking across different classes.
Yes, but unfortunately marking the lock as 'can-be-taken-multiple-times'
is weaker than using the nested locking provided by lockdep.
i.e. if you mark a lock this way, it opens door for having deadlock, which
won't be detected by lockdep. This will happen if the code, by mistake,
really takes the _very same_ lock twice. lockdep will not be able to
detect this, when the lock is marked in a way you propose, but is able to
detect this when using the nested semantics.
--
JiKos.
On 9/15/06, Jiri Kosina <[email protected]> wrote:
> On Fri, 15 Sep 2006, Dmitry Torokhov wrote:
>
> > I understand what Ingo is saying about detecting deadlocks across the
> > pool of locks of the same class not waiting till they really clash, it
> > is really useful. I also want to make my code as independent of lockdep
> > as possible. Having a speciall marking on the locks themselves (done
> > upon creation) instead of altering call sites is the cleanest way IMHO.
> > Can we have a flag in the lock structure that would tell lockdep that it
> > is OK for the given lock to be taken several times (i.e. the locks are
> > really on the different objects)? This would still allow to detect
> > incorrect locking across different classes.
>
> Yes, but unfortunately marking the lock as 'can-be-taken-multiple-times'
> is weaker than using the nested locking provided by lockdep.
>
> i.e. if you mark a lock this way, it opens door for having deadlock, which
> won't be detected by lockdep. This will happen if the code, by mistake,
> really takes the _very same_ lock twice. lockdep will not be able to
> detect this, when the lock is marked in a way you propose, but is able to
> detect this when using the nested semantics.
>
But nested semantics breaks the notion of the locks belonging to the
same pool so both solutions have tradeoffs. I could use either one of
these as long as details are hidden and callers do not have to care.
--
Dmitry
On 9/15/06, Dmitry Torokhov <[email protected]> wrote:
> On 9/15/06, Jiri Kosina <[email protected]> wrote:
> > On Fri, 15 Sep 2006, Dmitry Torokhov wrote:
> >
> > > I understand what Ingo is saying about detecting deadlocks across the
> > > pool of locks of the same class not waiting till they really clash, it
> > > is really useful. I also want to make my code as independent of lockdep
> > > as possible. Having a speciall marking on the locks themselves (done
> > > upon creation) instead of altering call sites is the cleanest way IMHO.
> > > Can we have a flag in the lock structure that would tell lockdep that it
> > > is OK for the given lock to be taken several times (i.e. the locks are
> > > really on the different objects)? This would still allow to detect
> > > incorrect locking across different classes.
> >
> > Yes, but unfortunately marking the lock as 'can-be-taken-multiple-times'
> > is weaker than using the nested locking provided by lockdep.
> >
> > i.e. if you mark a lock this way, it opens door for having deadlock, which
> > won't be detected by lockdep. This will happen if the code, by mistake,
> > really takes the _very same_ lock twice. lockdep will not be able to
> > detect this, when the lock is marked in a way you propose, but is able to
> > detect this when using the nested semantics.
> >
>
> But nested semantics breaks the notion of the locks belonging to the
> same pool so both solutions have tradeoffs. I could use either one of
> these as long as details are hidden and callers do not have to care.
>
One more thing I forgot to add - how will we deal with lockdep in
cases when we have X-over-Y-over-X protocol, when there is no tight
coupling between the X parts so it is impossible to know when to apply
special marking on the lock or callers?
--
Dmitry