[PATCH] make attribute_container_unregister() unconditionally wait for list empty

[PATCH] make attribute_container_unregister() unconditionally wait for list empty

[James Smart]

James,

>> > > Isn't a better way to handle it simply to give
>> > > transport_container_unregister() the semantics everyone is expecting
>> > > (i.e. to wait for everything to be tidied up and gone)?  That way none
>> > > of the transport classes needs updating, and we don't have to handle the
>> > > rather nasty release and unload races.
> 
> I was thinking of a wait_event driven system checking for 
> list_empty(cont->containers.k_list)

I hope this is more in line with what you were thinking.....

-- james s


Signed-off-by: James Smart <James.Smart@emulex.com>


diff -upNr a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
--- a/drivers/base/attribute_container.c	2007-08-16 02:59:50.000000000 -0400
+++ b/drivers/base/attribute_container.c	2007-08-17 05:02:03.000000000 -0400
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/sched.h>
 
 #include "base.h"
 
@@ -75,6 +76,8 @@ int
 attribute_container_register(struct attribute_container *cont)
 {
 	INIT_LIST_HEAD(&cont->node);
+	init_waitqueue_head(&cont->cont_wait);
+	cont->flags &= ~ATTRIBUTE_CONTAINER_EVENT_PENDING;
 	klist_init(&cont->containers,internal_container_klist_get,
 		   internal_container_klist_put);
 		
@@ -89,23 +92,26 @@ EXPORT_SYMBOL_GPL(attribute_container_re
 /**
  * attribute_container_unregister - remove a container registration
  *
+ * Semantics have now changed - we never return until the list goes empty
+ * 
  * @cont: previously registered container to remove
  */
 int
 attribute_container_unregister(struct attribute_container *cont)
 {
-	int retval = -EBUSY;
 	mutex_lock(&attribute_container_mutex);
 	spin_lock(&cont->containers.k_lock);
-	if (!list_empty(&cont->containers.k_list))
-		goto out;
-	retval = 0;
-	list_del(&cont->node);
- out:
+	if (!list_empty(&cont->containers.k_list)) 
+		cont->flags |= ATTRIBUTE_CONTAINER_EVENT_PENDING;
 	spin_unlock(&cont->containers.k_lock);
 	mutex_unlock(&attribute_container_mutex);
-	return retval;
-		
+	wait_event(cont->cont_wait,
+			 !(cont->flags & ATTRIBUTE_CONTAINER_EVENT_PENDING));
+	mutex_lock(&attribute_container_mutex);
+	list_del(&cont->node);
+	mutex_unlock(&attribute_container_mutex);
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(attribute_container_unregister);
 
@@ -234,6 +240,15 @@ attribute_container_remove_device(struct
 				class_device_unregister(&ic->classdev);
 			}
 		}
+
+		if (cont->flags & ATTRIBUTE_CONTAINER_EVENT_PENDING) {
+			spin_lock(&cont->containers.k_lock);
+			if (list_empty(&cont->containers.k_list)) 
+				cont->flags &= 
+					~ATTRIBUTE_CONTAINER_EVENT_PENDING;
+			spin_unlock(&cont->containers.k_lock);
+			wake_up(&cont->cont_wait);
+		}
 	}
 	mutex_unlock(&attribute_container_mutex);
 }
diff -upNr a/include/linux/attribute_container.h b/include/linux/attribute_container.h
--- a/include/linux/attribute_container.h	2007-08-16 03:00:10.000000000 -0400
+++ b/include/linux/attribute_container.h	2007-08-17 03:46:01.000000000 -0400
@@ -20,7 +20,9 @@ struct attribute_container {
 	struct class_device_attribute **attrs;
 	int (*match)(struct attribute_container *, struct device *);
 #define	ATTRIBUTE_CONTAINER_NO_CLASSDEVS	0x01
+#define	ATTRIBUTE_CONTAINER_EVENT_PENDING	0x02
 	unsigned long		flags;
+	wait_queue_head_t       cont_wait;
 };
 
 static inline int

Re: [PATCH] make attribute_container_unregister() unconditionally wait for list empty

[James Bottomley]

On Wed, 2007-08-15 at 11:40 -0400, James Smart wrote:
> James,
> 
> >> > > Isn't a better way to handle it simply to give
> >> > > transport_container_unregister() the semantics everyone is expecting
> >> > > (i.e. to wait for everything to be tidied up and gone)?  That way none
> >> > > of the transport classes needs updating, and we don't have to handle the
> >> > > rather nasty release and unload races.
> > 
> > I was thinking of a wait_event driven system checking for 
> > list_empty(cont->containers.k_list)
> 
> I hope this is more in line with what you were thinking.....

Almost .. the event is so rare, it's easier to do it globally (and not
waste the storage in the containers).  Plus, the signal is an empty
list, which we can move into a separate function.  Finally, I kept the
old API just in case, but made a new unregister_and_wait one which the
transport classes use (and changed the return to be void ... which tells
me absolutely no-one was cheking it).

This is the patch I've been testing.

James

diff --git a/drivers/base/attribute_container.c b/drivers/base/attribute_container.c
index 7370d7c..5cbfb4c 100644
--- a/drivers/base/attribute_container.c
+++ b/drivers/base/attribute_container.c
@@ -19,9 +19,16 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
 
 #include "base.h"
 
+static DECLARE_WAIT_QUEUE_HEAD(busy_containers_waitq);
+
+/* wait up to 30s for containers being unregistered to become free */
+#define BUSY_CONTAINERS_TIMEOUT (30*HZ)
+
 /* This is a private structure used to tie the classdev and the
  * container .. it should never be visible outside this file */
 struct internal_container {
@@ -86,29 +93,58 @@ attribute_container_register(struct attribute_container *cont)
 }
 EXPORT_SYMBOL_GPL(attribute_container_register);
 
+static int
+attribute_container_in_use(struct attribute_container *cont)
+{
+	int retval;
+
+	spin_lock(&cont->containers.k_lock);
+	retval = !!list_empty(&cont->containers.k_list);
+	spin_unlock(&cont->containers.k_lock);
+
+	return retval;
+}
 /**
  * attribute_container_unregister - remove a container registration
  *
  * @cont: previously registered container to remove
+ *
+ * May return -EBUSY if the container still contains live attributes
+ * that cannot be released.
  */
 int
 attribute_container_unregister(struct attribute_container *cont)
 {
 	int retval = -EBUSY;
 	mutex_lock(&attribute_container_mutex);
-	spin_lock(&cont->containers.k_lock);
-	if (!list_empty(&cont->containers.k_list))
+	if (attribute_container_in_use(cont))
 		goto out;
 	retval = 0;
 	list_del(&cont->node);
  out:
-	spin_unlock(&cont->containers.k_lock);
 	mutex_unlock(&attribute_container_mutex);
 	return retval;
 		
 }
 EXPORT_SYMBOL_GPL(attribute_container_unregister);
 
+/**
+ * attribute_container_unregister_and_wait - remove a container registration
+ *
+ * @cont: previously registered container to remove
+ *
+ * Waits for everything to be released before returning.
+ */
+void
+attribute_container_unregister_and_wait(struct attribute_container *cont)
+{
+	while (attribute_container_unregister(cont) != -EBUSY)
+		WARN_ON(wait_event_timeout(busy_containers_waitq,
+					   !attribute_container_in_use(cont),
+					   BUSY_CONTAINERS_TIMEOUT) == 0);
+}
+EXPORT_SYMBOL_GPL(attribute_container_unregister_and_wait);
+
 /* private function used as class release */
 static void attribute_container_release(struct class_device *classdev)
 {
@@ -227,6 +263,8 @@ attribute_container_remove_device(struct device *dev,
 			if (dev != ic->classdev.dev)
 				continue;
 			klist_del(&ic->node);
+			if (!attribute_container_in_use(cont))
+				wake_up(&busy_containers_waitq);
 			if (fn)
 				fn(cont, dev, &ic->classdev);
 			else {
diff --git a/include/linux/attribute_container.h b/include/linux/attribute_container.h
index 8ff2749..1025557 100644
--- a/include/linux/attribute_container.h
+++ b/include/linux/attribute_container.h
@@ -37,6 +37,7 @@ attribute_container_set_no_classdevs(struct attribute_container *atc)
 
 int attribute_container_register(struct attribute_container *cont);
 int attribute_container_unregister(struct attribute_container *cont);
+void attribute_container_unregister_and_wait(struct attribute_container *cont);
 void attribute_container_create_device(struct device *dev,
 				       int (*fn)(struct attribute_container *,
 						 struct device *,
diff --git a/include/linux/transport_class.h b/include/linux/transport_class.h
index 1d6cc22..63d4bd6 100644
--- a/include/linux/transport_class.h
+++ b/include/linux/transport_class.h
@@ -86,9 +86,9 @@ static inline int transport_container_register(struct transport_container *tc)
 	return attribute_container_register(&tc->ac);
 }
 
-static inline int transport_container_unregister(struct transport_container *tc)
+static inline void transport_container_unregister(struct transport_container *tc)
 {
-	return attribute_container_unregister(&tc->ac);
+	attribute_container_unregister_and_wait(&tc->ac);
 }
 
 int transport_class_register(struct transport_class *);

Re: [PATCH] make attribute_container_unregister() unconditionally wait for list empty

[James Bottomley]

On Wed, 2007-08-15 at 10:54 -0500, James Bottomley wrote:
> This is the patch I've been testing.

Actually, that patch was wrong ... it has two subtle bugs that conspire
to make it seem correct.  Can you try this one instead?  I don't have
any fibre equipment, and the other transport classes aren't in use when
they're released.

Thanks,

James

Index: BUILD-2.6/drivers/base/attribute_container.c
===================================================================
--- BUILD-2.6.orig/drivers/base/attribute_container.c	2007-08-15 12:09:32.000000000 -0500
+++ BUILD-2.6/drivers/base/attribute_container.c	2007-08-15 18:01:44.000000000 -0500
@@ -19,9 +19,16 @@
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/sched.h>
+#include <linux/wait.h>
 
 #include "base.h"
 
+static DECLARE_WAIT_QUEUE_HEAD(busy_containers_waitq);
+
+/* wait up to 30s for containers being unregistered to become free */
+#define BUSY_CONTAINERS_TIMEOUT (30*HZ)
+
 /* This is a private structure used to tie the classdev and the
  * container .. it should never be visible outside this file */
 struct internal_container {
@@ -86,29 +93,58 @@ attribute_container_register(struct attr
 }
 EXPORT_SYMBOL_GPL(attribute_container_register);
 
+static int
+attribute_container_in_use(struct attribute_container *cont)
+{
+	int retval;
+
+	spin_lock(&cont->containers.k_lock);
+	retval = !list_empty(&cont->containers.k_list);
+	spin_unlock(&cont->containers.k_lock);
+
+	return retval;
+}
 /**
  * attribute_container_unregister - remove a container registration
  *
  * @cont: previously registered container to remove
+ *
+ * May return -EBUSY if the container still contains live attributes
+ * that cannot be released.
  */
 int
 attribute_container_unregister(struct attribute_container *cont)
 {
 	int retval = -EBUSY;
 	mutex_lock(&attribute_container_mutex);
-	spin_lock(&cont->containers.k_lock);
-	if (!list_empty(&cont->containers.k_list))
+	if (attribute_container_in_use(cont))
 		goto out;
 	retval = 0;
 	list_del(&cont->node);
  out:
-	spin_unlock(&cont->containers.k_lock);
 	mutex_unlock(&attribute_container_mutex);
 	return retval;
 		
 }
 EXPORT_SYMBOL_GPL(attribute_container_unregister);
 
+/**
+ * attribute_container_unregister_and_wait - remove a container registration
+ *
+ * @cont: previously registered container to remove
+ *
+ * Waits for everything to be released before returning.
+ */
+void
+attribute_container_unregister_and_wait(struct attribute_container *cont)
+{
+	while (attribute_container_unregister(cont) == -EBUSY)
+		WARN_ON(wait_event_timeout(busy_containers_waitq,
+					   !attribute_container_in_use(cont),
+					   BUSY_CONTAINERS_TIMEOUT) == 0);
+}
+EXPORT_SYMBOL_GPL(attribute_container_unregister_and_wait);
+
 /* private function used as class release */
 static void attribute_container_release(struct class_device *classdev)
 {
@@ -227,6 +263,8 @@ attribute_container_remove_device(struct
 			if (dev != ic->classdev.dev)
 				continue;
 			klist_del(&ic->node);
+			if (!attribute_container_in_use(cont))
+				wake_up(&busy_containers_waitq);
 			if (fn)
 				fn(cont, dev, &ic->classdev);
 			else {
Index: BUILD-2.6/include/linux/attribute_container.h
===================================================================
--- BUILD-2.6.orig/include/linux/attribute_container.h	2007-08-15 12:09:32.000000000 -0500
+++ BUILD-2.6/include/linux/attribute_container.h	2007-08-15 15:59:14.000000000 -0500
@@ -37,6 +37,7 @@ attribute_container_set_no_classdevs(str
 
 int attribute_container_register(struct attribute_container *cont);
 int attribute_container_unregister(struct attribute_container *cont);
+void attribute_container_unregister_and_wait(struct attribute_container *cont);
 void attribute_container_create_device(struct device *dev,
 				       int (*fn)(struct attribute_container *,
 						 struct device *,
Index: BUILD-2.6/include/linux/transport_class.h
===================================================================
--- BUILD-2.6.orig/include/linux/transport_class.h	2007-08-15 12:09:32.000000000 -0500
+++ BUILD-2.6/include/linux/transport_class.h	2007-08-15 15:59:14.000000000 -0500
@@ -86,9 +86,9 @@ static inline int transport_container_re
 	return attribute_container_register(&tc->ac);
 }
 
-static inline int transport_container_unregister(struct transport_container *tc)
+static inline void transport_container_unregister(struct transport_container *tc)
 {
-	return attribute_container_unregister(&tc->ac);
+	attribute_container_unregister_and_wait(&tc->ac);
 }
 
 int transport_class_register(struct transport_class *);