Re: [RFC/PATCH 0/22] W1: sysfs, lifetime and other fixes

[Dmitry Torokhov]

On Tuesday 26 April 2005 02:06, Evgeniy Polyakov wrote:
> On Tue, 2005-04-26 at 01:50 -0500, Dmitry Torokhov wrote:
> > 
> > If you program so that you wait in module_exit for object release - you
> > get what you deserve. 
> 
> But we can remove objects not from rmmod path.
> You pointed right example in one previous e-mail.

Right, and you need to be careful so that thread does not hold any references
to the resource it tries to free. rmmod is just one pf the common examples.
But you need to consider this scenario whether you using driver model or your
separate refcount - the basic problem is still the same.

> Using above "waiting for device..." message is for debug only.
> 
> > > > BTW, I am looking at the connector code ATM and I am just amazed at
> > > > all wied refounting stuff that is going on there. what a single
> > > > actomic_dec_and_test() call without checkng reurn vaue is supposed to
> > > > do again?
> > > 
> > > It has explicit barrieres, which guarantees that
> > > there will not be atomic operation vs. non atomic
> > > reordering. 
> > 
> > And you can't use explicit barriers - why exactly?
> 
> I used them - code was following:
> smp_mb__before_atomic_dec();
> atomic_dec();
> smp_mb__after_atomic_dec();
> 
> I think simple atomic_dec_and_test() or even atomic_dec_and_lock()
> is better.

This is usually indicates that there some kiond of a problem. Consider
following fragment:

> +static void cn_queue_wrapper(void *data)
> +{
> +ÂÂÂÂÂÂÂstruct cn_callback_entry *cbq = (struct cn_callback_entry *)data;
> +
> +ÂÂÂÂÂÂÂatomic_inc_and_test(&cbq->cb->refcnt);
> +ÂÂÂÂÂÂÂcbq->cb->callback(cbq->cb->priv);
> +ÂÂÂÂÂÂÂatomic_dec_and_test(&cbq->cb->refcnt);
> 

What exactly this refcount protects? Can it be that other code decrements
refcount and frees the object right when one CPU is entering this function?
If not that means that cb structure is protected by some other means, so
why we need to increment refcout here and consider ordering?

Btw, cb refcount can be complelely removed, something like the patch below
(won't apply cleanly as I have some other stuff).

-- 
Dmitry

 drivers/connector/cn_queue.c |   85 +++++++++++--------------------------------
 include/linux/connector.h    |    2 -
 2 files changed, 23 insertions(+), 64 deletions(-)

Index: linux-2.6.11/drivers/connector/cn_queue.c
===================================================================
--- linux-2.6.11.orig/drivers/connector/cn_queue.c
+++ linux-2.6.11/drivers/connector/cn_queue.c
@@ -33,49 +33,12 @@
 
 static void cn_queue_wrapper(void *data)
 {
-	struct cn_callback_entry *cbq = (struct cn_callback_entry *)data;
+	struct cn_callback_entry *cbq = data;
 
-	atomic_inc_and_test(&cbq->cb->refcnt);
 	cbq->cb->callback(cbq->cb->priv);
-	atomic_dec_and_test(&cbq->cb->refcnt);
-
 	cbq->destruct_data(cbq->ddata);
 }
 
-static struct cn_callback_entry *cn_queue_alloc_callback_entry(struct cn_callback *cb)
-{
-	struct cn_callback_entry *cbq;
-
-	cbq = kmalloc(sizeof(*cbq), GFP_KERNEL);
-	if (!cbq) {
-		printk(KERN_ERR "Failed to create new callback queue.\n");
-		return NULL;
-	}
-
-	memset(cbq, 0, sizeof(*cbq));
-
-	cbq->cb = cb;
-
-	INIT_WORK(&cbq->work, &cn_queue_wrapper, cbq);
-
-	return cbq;
-}
-
-static void cn_queue_free_callback(struct cn_callback_entry *cbq)
-{
-	cancel_delayed_work(&cbq->work);
-	flush_workqueue(cbq->pdev->cn_queue);
-
-	while (atomic_read(&cbq->cb->refcnt)) {
-		printk(KERN_INFO "Waiting for %s to become free: refcnt=%d.\n",
-		       cbq->pdev->name, atomic_read(&cbq->cb->refcnt));
-
-		msleep(1000);
-	}
-
-	kfree(cbq);
-}
-
 int cn_cb_equal(struct cb_id *i1, struct cb_id *i2)
 {
 #if 0
@@ -90,40 +53,37 @@ int cn_cb_equal(struct cb_id *i1, struct
 int cn_queue_add_callback(struct cn_queue_dev *dev, struct cn_callback *cb)
 {
 	struct cn_callback_entry *cbq, *__cbq;
-	int found = 0;
+	int retval = 0;
 
-	cbq = cn_queue_alloc_callback_entry(cb);
-	if (!cbq)
+	cbq = kmalloc(sizeof(*cbq), GFP_KERNEL);
+	if (!cbq) {
+		printk(KERN_ERR "Failed to create new callback queue.\n");
 		return -ENOMEM;
+	}
 
 	atomic_inc(&dev->refcnt);
+
+	memset(cbq, 0, sizeof(*cbq));
+	INIT_WORK(&cbq->work, &cn_queue_wrapper, cbq);
+	cbq->cb = cb;
 	cbq->pdev = dev;
+	cbq->nls = dev->nls;
+	cbq->seq = 0;
+	cbq->group = cbq->cb->id.idx;
 
 	spin_lock_bh(&dev->queue_lock);
+
 	list_for_each_entry(__cbq, &dev->queue_list, callback_entry) {
 		if (cn_cb_equal(&__cbq->cb->id, &cb->id)) {
-			found = 1;
-			break;
+			retval = -EEXIST;
+			kfree(cbq);
+			goto out;
 		}
 	}
-	if (!found) {
-		atomic_set(&cbq->cb->refcnt, 1);
-		list_add_tail(&cbq->callback_entry, &dev->queue_list);
-	}
+	list_add_tail(&cbq->callback_entry, &dev->queue_list);
+ out:
 	spin_unlock_bh(&dev->queue_lock);
-
-	if (found) {
-		atomic_dec(&dev->refcnt);
-		atomic_set(&cbq->cb->refcnt, 0);
-		cn_queue_free_callback(cbq);
-		return -EINVAL;
-	}
-
-	cbq->nls = dev->nls;
-	cbq->seq = 0;
-	cbq->group = cbq->cb->id.idx;
-
-	return 0;
+	return retval;
 }
 
 void cn_queue_del_callback(struct cn_queue_dev *dev, struct cb_id *id)
@@ -142,8 +102,9 @@ void cn_queue_del_callback(struct cn_que
 	spin_unlock_bh(&dev->queue_lock);
 
 	if (found) {
-		atomic_dec(&cbq->cb->refcnt);
-		cn_queue_free_callback(cbq);
+		cancel_delayed_work(&cbq->work);
+		flush_workqueue(cbq->pdev->cn_queue);
+		kfree(cbq);
 		atomic_dec_and_test(&dev->refcnt);
 	}
 }
Index: linux-2.6.11/include/linux/connector.h
===================================================================
--- linux-2.6.11.orig/include/linux/connector.h
+++ linux-2.6.11/include/linux/connector.h
@@ -115,8 +115,6 @@ struct cn_callback
 	struct cb_id		id;
 	void			(* callback)(void *);
 	void			*priv;
-
-	atomic_t		refcnt;
 };
 
 struct cn_callback_entry
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/