Re: 2.6.0-test7-mm1

["Kristian Høgsberg" <krh@bitplanet.net>]

[-- Attachment #1: Type: text/plain, Size: 1948 bytes --]

Ben Collins wrote:
> On Wed, Oct 15, 2003 at 10:53:59AM -0700, Andrew Morton wrote:
> 
>>Ben Collins <bcollins@debian.org> wrote:
>>
>>>>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.
>>
>>nodemgr_add_host() looks like the hard one.  Maybe make hl_drivers_lock a
>>sleeping lock?
> 
> 
> 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.

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

[-- Attachment #2: double-list-patch --]
[-- Type: text/plain, Size: 2789 bytes --]

Index: highlevel.c
===================================================================
--- highlevel.c	(revision 1073)
+++ highlevel.c	(working copy)
@@ -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
===================================================================
--- highlevel.h	(revision 1073)
+++ highlevel.h	(working copy)
@@ -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;