2.6.0-test7-mm1

After compiling 2.6.0-test7-mm1, I get the following error after installing the modules:

if [ -r System.map ]; then /sbin/depmod -ae -F System.map 2.6.0-test7-mm1; fi
WARNING: /lib/modules/2.6.0-test7-mm1/kernel/fs/ext2/ext2.ko needs unknown symbol __blockdev_direct_IO

I checked the filesystem code and the function __blockdev_direct_IO is defined at fs/direct-io.c, line 1020. However, the EXPORT_SYMBOL() definition uses "blockdev_direct_IO", not "__blockdev_direct_IO". Grepping the include/linux directory shows inline definitions for various wrappers for __blockdev_direct_IO, including "blockdev_direct_IO", in include/linux/fs.h at line 1355. Yet depmod can't seem to find it for the ext2 module.

Does this have anything to do with Jens Axboe's IDE write barrier/JBD write barrier work?

Brad

=====
Brad Chapman

Permanent e-mail: [email protected]

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Thanks.

 fs/direct-io.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN fs/direct-io.c~O_DIRECT-race-fixes-rework-XFS-fix-fix fs/direct-io.c
@@ -1089,4 +1089,4 @@ __blockdev_direct_IO(int rw, struct kioc
 out:
 	return retval;
 }
-EXPORT_SYMBOL(blockdev_direct_IO);
+EXPORT_SYMBOL(__blockdev_direct_IO);

_

Mr. Morton,

<SNIPPED>

You're welcome. Unfortunately I got this non-fatal Oops when I first booted:

ohci1394: $Rev: 1045 $ Ben Collins <[email protected]>
ohci1394_0: OHCI-1394 1.1 (PCI): IRQ=[10] MMIO=[e8207000-e82077ff] Max Packet=[2048]
Debug: sleeping function called from invalid context at mm/slab.c:1869 in_atomic():1, irqs_disabled():0

Call Trace:
 [<c0123fc6>] __might_sleep+0xa0/0xc2
 [<c0156bc4>] __kmalloc+0x204/0x216
 [<e08a9ed4>] hpsb_create_hostinfo+0x6b/0xe8 [ieee1394]
 [<e08af0e6>] nodemgr_add_host+0x23/0x1d2 [ieee1394]
 [<c0216bd4>] sprintf+0x1f/0x23
 [<e08aa789>] highlevel_add_host+0x6b/0x6f [ieee1394]
 [<e08a9cce>] hpsb_add_host+0x6d/0x95 [ieee1394]
 [<e08c0ba2>] ohci1394_pci_probe+0x512/0x620 [ohci1394]
 [<e08bdb41>] ohci_irq_handler+0x0/0x1129 [ohci1394]
 [<c021cf6a>] pci_device_probe_static+0x52/0x63
 [<c021cfb6>] __pci_device_probe+0x3b/0x4e
 [<c021cff5>] pci_device_probe+0x2c/0x4a
 [<c027ffaa>] bus_match+0x3f/0x6a
 [<c02800bc>] driver_attach+0x56/0x80
 [<c028038e>] bus_add_driver+0x9f/0xb1
 [<c02807f2>] driver_register+0x8c/0x90
 [<c021d1e1>] pci_register_driver+0x8c/0xab
 [<e0886013>] ohci1394_init+0x13/0x3d [ohci1394]
 [<c01482f9>] sys_init_module+0x213/0x3e6
 [<c0176191>] sys_read+0x42/0x63
 [<c03add36>] sysenter_past_esp+0x43/0x65

ieee1394: Host added: ID:BUS[0-00:1023] GUID[00e0b8060000db10]

I don't see any patches in your ChangeLog which could have caused this, since it didn't happen under 2.6.0-test7 or 2.6.0-test6-mm4. Just from looking at the stack trace, it looks like bugs in my IEEE1394 chipset. Here's what lspci -vv has to say:

02:05.0 FireWire (IEEE 1394): Texas Instruments TSB43AB21 IEEE-1394a-2000 Controller (PHY/Link) (prog-if 10 [OHCI])
Subsystem: Gateway 2000: Unknown device 0600
Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV+ VGASnoop- ParErr-
Stepping- SERR+ FastB2B-
Status: Cap+ 66Mhz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort- <TAbort-
<MAbort- >SERR- <PERR-
Latency: 64 (500ns min, 1000ns max), cache line size 08
Interrupt: pin A routed to IRQ 10
Region 0: Memory at e8207000 (32-bit, non-prefetchable) [size=2K] Region 1: Memory at e8200000 (32-bit, non-prefetchable) [size=16K]
Capabilities: [44] Power Management version 2
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA
PME(D0+,D1+,D2+,D3hot+,D3cold-)
Status: D0 PME-Enable- DSel=0 DScale=0 PME+

And the interesting part of my .config:

CONFIG_IEEE1394=m
CONFIG_IEEE1394_OHCI1394=m
CONFIG_IEEE1394_VIDEO1394=m
CONFIG_IEEE1394_ETH1394=m
CONFIG_IEEE1394_DV1394=m
CONFIG_IEEE1394_RAWIO=m
CONFIG_IEEE1394_CMP=m

Brad

=====
Brad Chapman

Permanent e-mail: [email protected]

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

[...]

"might_sleep-vs-jiffies-wrap.patch" pops to mind, it probably just didn't get reported in earlier kernels because starting with INITIAL_JIFFIES!=0 broke the rate limiting logic (sorry for that)

Tim

Mr. Schmielau,

Well, I don't use the IEEE1394 drivers (yet), so like I said, it's non-fatal. Are there any other debugging options I can enable that would help pinpoint this (i.e frame pointers?)

Interesting part of .config:

CONFIG_DEBUG_KERNEL=y
CONFIG_DEBUG_STACKOVERFLOW=y
CONFIG_DEBUG_SLAB=y
CONFIG_DEBUG_IOVIRT=y
CONFIG_MAGIC_SYSRQ=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_SPINLOCK_SLEEP=y
CONFIG_X86_EXTRA_IRQS=y
CONFIG_X86_FIND_SMP_CONFIG=y
CONFIG_X86_MPPARSE=y

Brad

=====
Brad Chapman

Permanent e-mail: [email protected]

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

highlevel_add_host() does read_lock() and then proceeds to do things like starting kernel threads under that lock. The locking is pretty broken in there :(

No, highlevel_add_host() itself doesn't start any threads. But it does pass around data that needs to be locked from changes, and one of the handlers happens to start a thread, and other things allocate memory (such as this case).

It's ugly, and I've been trying to clean it up. This case can be fixed quickly with a simple check in hpsb_create_hostinfo() to pass GFP_ATOMIC to kmalloc.

My problem right now, is I don't use any architectures that support preempt, so I don't see a lot of these problems, like I catch with CONFIG_SMP.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

nodemgr_add_host() looks like the hard one. Maybe make hl_drivers_lock a sleeping lock?

Anton had a ppc64 patch which implemented the preempt_count beancounting without actually implementing premption. So might_sleep() does the right thing.

Problem is, things like bus resets happen in interrupt, and while I can push off some things to occur in the nodemgr thread, a lot of other stuff has to happen in the interrupt, and they require the same lock.

It's all a matter of just tossing out my todo list for a good week and redoing all of this logic from the ground up. I have high hopes this will happen in the next few weeks, but I'm not signing any contracts to hold me to it :)

I might have to dig that out and try to make use of it. Thanks for the pointer.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Here is the patch with the ppc64 bits in it. (I already have might_sleep() littered in asm-ppc64/semaphore.h and asm-ppc64/uaccess.h)

Its a bit old but it seems to dump valid warnings.

Anton

Sleep with spinlock debugging.

 foo-anton/arch/ppc64/Kconfig          |    7 +++++++
 foo-anton/include/asm-ppc64/hardirq.h |    2 +-
 foo-anton/include/linux/preempt.h     |   17 +++++++++++------
 foo-anton/kernel/fork.c               |    2 +-
 foo-anton/kernel/sched.c              |    2 +-
 5 files changed, 21 insertions(+), 9 deletions(-)

diff -puN arch/ppc64/Kconfig~spinlock_sleep arch/ppc64/Kconfig
@@ -385,6 +385,13 @@ config DEBUG_PAGEALLOC
 	  This results in a large slowdown, but helps to find certain types
 	  of memory corruptions.
 
+config DEBUG_SPINLOCK_SLEEP
+	bool "Sleep-inside-spinlock checking"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here, various routines which may sleep will become very
+	  noisy if they are called with a spinlock held.
+
 endmenu
 
 source "security/Kconfig"

diff -puN include/asm-ppc64/hardirq.h~spinlock_sleep include/asm-ppc64/hardirq.h
@@ -82,7 +82,7 @@ typedef struct {
 
 #define irq_enter()		(preempt_count() += HARDIRQ_OFFSET)
 
-#ifdef CONFIG_PREEMPT
+#if defined(CONFIG_PREEMPT) || defined(CONFIG_DEBUG_SPINLOCK_SLEEP)
 # define in_atomic()	((preempt_count() & ~PREEMPT_ACTIVE) != kernel_locked())
 # define IRQ_EXIT_OFFSET (HARDIRQ_OFFSET-1)
 #else

diff -puN include/linux/preempt.h~spinlock_sleep include/linux/preempt.h
@@ -24,6 +24,17 @@ do { \
 
 extern void preempt_schedule(void);
 
+#define preempt_check_resched() \
+do { \
+	if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
+		preempt_schedule(); \
+} while (0)
+#else
+#define preempt_check_resched()		do { } while (0)
+#endif
+
+#if defined(CONFIG_PREEMPT) || defined(CONFIG_DEBUG_SPINLOCK_SLEEP)
+
 #define preempt_disable() \
 do { \
 	inc_preempt_count(); \

@@ -36,12 +47,6 @@ do { \
 	barrier(); \
 } while (0)
 
-#define preempt_check_resched() \
-do { \
-	if (unlikely(test_thread_flag(TIF_NEED_RESCHED))) \
-		preempt_schedule(); \
-} while (0)
-
 #define preempt_enable() \
 do { \
 	preempt_enable_no_resched(); \

diff -puN kernel/fork.c~spinlock_sleep kernel/fork.c
@@ -850,7 +850,7 @@ struct task_struct *copy_process(unsigne
 	if (p->binfmt && !try_module_get(p->binfmt->module))
 		goto bad_fork_cleanup_put_domain;
 
-#ifdef CONFIG_PREEMPT
+#if defined(CONFIG_PREEMPT) || defined(CONFIG_DEBUG_SPINLOCK_SLEEP)
 	/*
 	 * schedule_tail drops this_rq()->lock so we compensate with a count
 	 * of 1.  Also, we want to start with kernel preemption disabled.

diff -puN kernel/sched.c~spinlock_sleep kernel/sched.c
@@ -2533,7 +2533,7 @@ void __init init_idle(task_t *idle, int 
 	local_irq_restore(flags);
 
 	/* Set the preempt count _outside_ the spinlocks! */
-#ifdef CONFIG_PREEMPT
+#if defined(CONFIG_PREEMPT) || defined(CONFIG_DEBUG_SPINLOCK_SLEEP)
 	idle->thread_info->preempt_count = (idle->lock_depth >= 0);
 #else
 	idle->thread_info->preempt_count = 0;

_

I was looking briefly at this too, and as you say, the problem is that some things have to happen in interrupt, others happen in process context. I've attached a patch that implements one way to fix it: double book-keeping - we maintain two lists of the highlevel drivers, one protected by a semaphore another protected by the rw spinlock. The lists are identical, except between the two list_add_tail()'s (and the two list_del()'s), but that doesn't allow any harmful race conditions.

A more radical approach would be to split the highlevel interface into two interfaces add_host() + remove_host() in a hpsb_host_notification interface and the rest in another interface. The driver would have to register both interfaces if it needs them. Some drivers only use add_host() and remove_host(), so they could register only the hpsb_host_notification interface.

best regards,
Kristian

Index: highlevel.c
===================================================================
@@ -37,9 +37,18 @@
 };
 
 
+/* Double bookkeeping: hl_drivers and hl_sem_drivers are essentially
+ * the same lists, but hl_sem_list is protected by a semaphore and is
+ * used to notify highlevel drivers when we add and remove hosts. The
+ * other, hl_drivers is protected by an rw spinlock, and is used when
+ * we notify highlevel drivers of bus resets and incoming packets. */
+
 static LIST_HEAD(hl_drivers);
 static rwlock_t hl_drivers_lock = RW_LOCK_UNLOCKED;
 
+static LIST_HEAD(hl_sem_drivers);
+static DECLARE_MUTEX(hl_drivers_sem);
+
 static LIST_HEAD(addr_space);
 static rwlock_t addr_space_lock = RW_LOCK_UNLOCKED;
 

@@ -238,6 +247,10 @@
 
 	rwlock_init(&hl->host_info_lock);
 
+	down(&hl_drivers_sem);
+	list_add_tail(&hl->hl_sem_list, &hl_sem_drivers);
+	up(&hl_drivers_sem);
+
 	write_lock_irqsave(&hl_drivers_lock, flags);
         list_add_tail(&hl->hl_list, &hl_drivers);
 	write_unlock_irqrestore(&hl_drivers_lock, flags);

@@ -272,6 +285,10 @@
         list_del(&hl->hl_list);
 	write_unlock_irqrestore(&hl_drivers_lock, flags);
 
+	down(&hl_drivers_sem);
+	list_del(&hl->hl_sem_list);
+	up(&hl_drivers_sem);
+
         if (hl->remove_host) {
 		down(&hpsb_hosts_lock);
 		list_for_each(lh, &hpsb_hosts) {

@@ -391,33 +408,28 @@
 
 void highlevel_add_host(struct hpsb_host *host)
 {
-        struct list_head *entry;
         struct hpsb_highlevel *hl;
 
-        read_lock(&hl_drivers_lock);
-        list_for_each(entry, &hl_drivers) {
-                hl = list_entry(entry, struct hpsb_highlevel, hl_list);
+	down(&hl_drivers_sem);
+	list_for_each_entry(hl, &hl_sem_drivers, hl_sem_list) {
 		if (hl->add_host)
 			hl->add_host(host);
         }
-        read_unlock(&hl_drivers_lock);
+	up(&hl_drivers_sem);
 }
 
 void highlevel_remove_host(struct hpsb_host *host)
 {
-        struct list_head *entry;
         struct hpsb_highlevel *hl;
 
-	read_lock(&hl_drivers_lock);
-	list_for_each(entry, &hl_drivers) {
-                hl = list_entry(entry, struct hpsb_highlevel, hl_list);
-
+	down(&hl_drivers_sem);
+	list_for_each_entry(hl, &hl_sem_drivers, hl_sem_list) {
 		if (hl->remove_host) {
 			hl->remove_host(host);
 			hpsb_destroy_hostinfo(hl, host);
 		}
         }
-	read_unlock(&hl_drivers_lock);
+	up(&hl_drivers_sem);
 }
 
 void highlevel_host_reset(struct hpsb_host *host)

Index: highlevel.h
===================================================================
@@ -56,6 +56,7 @@
                              int cts, u8 *data, size_t length);
 
 
+	struct list_head hl_sem_list;
 	struct list_head hl_list;
 	struct list_head addr_list;
 

Actually I'm leaning toward getting rid of our internal locking and reference counting and relying heavily on the device model's reference counting and such. Take some of the work load off of our code.

Each host already has a device associated with it, so it just requires a revamp of some internals.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Mr. Collins,

JFYI, this bug also appears under 2.6.0-test8:

ohci1394: $Rev: 1045 $ Ben Collins <[email protected]>
ohci1394_0: OHCI-1394 1.1 (PCI): IRQ=[10] MMIO=[e8207000-e82077ff] Max Packet=[2048]
Debug: sleeping function called from invalid context at mm/slab.c:1857 in_atomic():1, irqs_disabled():0

Call Trace:
 [<c012186b>] __might_sleep+0xa0/0xc1
 [<c01531f4>] __kmalloc+0x204/0x216
 [<e08a9e2c>] hpsb_create_hostinfo+0x6b/0xe8 [ieee1394]
 [<e08aef8a>] nodemgr_add_host+0x23/0x1d2 [ieee1394]
 [<c0210abc>] sprintf+0x1f/0x23
 [<e08aa62d>] highlevel_add_host+0x6b/0x6f [ieee1394]
 [<e08a9c42>] hpsb_add_host+0x6d/0x95 [ieee1394]
 [<e08c0b4e>] ohci1394_pci_probe+0x512/0x620 [ohci1394]
 [<e08bdb18>] ohci_irq_handler+0x0/0x1129 [ohci1394]
 [<c0216d33>] pci_device_probe_static+0x52/0x63
 [<c0216d7f>] __pci_device_probe+0x3b/0x4e
 [<c0216dbe>] pci_device_probe+0x2c/0x4a
 [<c0279e72>] bus_match+0x3f/0x6a
 [<c0279f84>] driver_attach+0x56/0x80
 [<c027a256>] bus_add_driver+0x9f/0xb1
 [<c027a6ba>] driver_register+0x8c/0x90
 [<c0216faa>] pci_register_driver+0x8c/0xab
 [<e0886013>] ohci1394_init+0x13/0x3d [ohci1394]
 [<c0145a37>] sys_init_module+0x213/0x3e6
 [<c017228b>] sys_read+0x42/0x63
 [<c010a179>] sysenter_past_esp+0x52/0x71

ieee1394: Host added: ID:BUS[0-00:1023] GUID[00e0b8060000db10]

Since I don't use the OHCI1394 drivers yet, I can't really offer any assistance, except to test patches that make the debug message go away.

Brad

=====
Brad Chapman

Permanent e-mail: [email protected]

__________________________________
Do you Yahoo!?
The New Yahoo! Shopping - with improved product search
http://shopping.yahoo.com

Oh, man, please don't use the "Mr." :)

As would be expected considering I'm holding off changes to let 2.6 focus on core issues. The "bug" is pretty much harmless as-is, even if noisy, and wont show up under normal compiles with debugging disables.

Plus, my fix is a little more than trivial, so I'm keeping it for 2.6.1.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Oh great, let's wait for the "little more than trivial" bug fixes until after it's released....

I understand that you're busy, and your schedule may force you to wait until after 2.6.0, but 2.6-test is to help get the bugs out of a release, and this is a bug fix so it should be accepted...

Obviously you haven't been reading Linus' posts about what he'll accept into 2.6.0-test before 2.6.0 is final. He wont take my patches, and rightfully so.

This isn't a matter of my choice, it's a matter of timing. Get over it, and download the latest stuff from linux1394.org if you need it that badly. Here's a hint:

# cd /usr/src/linux-2.6.0-test9
# rm -rf drivers/ieee1394
# wget -o trunk.tar.gz http://www.linux1394.org/viewcvs/trunk/trunk.tar.gz?tarball=1
# tar zxf trunk.tar.gz 
# mv trunk drivers/ieee1394
# rm -f trunk.tar.gz

It doesn't get any simpler than that.

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Memory: 774008k/786240k available (2352k kernel code, 11468k reserved, 848k data, 180k init, 0k highmem) zapping low mappings.
Debug: sleeping function called from invalid context at mm/slab.c:1869
in_atomic():1, irqs_disabled():0

Call Trace:
 [<c0107000>] rest_init+0x0/0xf4
 [<c012a40f>] __might_sleep+0xa0/0xbd
 [<c015f39b>] kmem_cache_alloc+0x1c3/0x1c8
 [<c0107000>] rest_init+0x0/0xf4
 [<c015d2ec>] kmem_cache_create+0x16b/0x6a5
 [<c04331c3>] mem_init+0x1e4/0x30d
 [<c0107000>] rest_init+0x0/0xf4
 [<c0435bcb>] kmem_cache_init+0x182/0x32b
 [<c04246d2>] start_kernel+0x15d/0x280
 [<c042444a>] unknown_bootoption+0x0/0xf8

Calibrating delay loop... 3342.33 BogoMIPS
Security Scaffold v1.0.0 initialized

mvh,
A

Alexander Hoogerhuis                               | [email protected]
CCNP - CCDP - MCNE - CCSE                          | +47 908 21 485
"You have zero privacy anyway. Get over it."  --Scott McNealy

I was thinking of simply suppressing all these early warnings:

@@ -2855,7 +2855,7 @@ void __might_sleep(char *file, int line)
 #if defined(in_atomic)
 	static unsigned long prev_jiffy;	/* ratelimiting */
 
-	if (in_atomic() || irqs_disabled()) {
+	if (system_running && in_atomic() || irqs_disabled()) {
 		if (time_before(jiffies, prev_jiffy + HZ) && prev_jiffy)
 			return;
 		prev_jiffy = jiffies;

But really we shouldn't do that because it is possible that an early accidental enabling of interrupts can lock the box. That happens on ppc64 for example.

We should fix them up.