[PATCH] fallout from atomic_long_t patch

[PATCH] fallout from atomic_long_t patch

[Al Viro]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 5e7cd45..4cec1a8 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -135,8 +135,7 @@ static int cn_call_callback(struct cn_ms
 	spin_lock_bh(&dev->cbdev->queue_lock);
 	list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
-			if (likely(!test_bit(WORK_STRUCT_PENDING,
-					     &__cbq->work.work.management) &&
+			if (likely(!work_pending(&__cbq->work.work) &&
 					__cbq->data.ddata == NULL)) {
 				__cbq->data.callback_priv = msg;
 

Re: [PATCH] fallout from atomic_long_t patch

[Linus Torvalds]

On Sun, 17 Dec 2006, Al Viro wrote:
> -			if (likely(!test_bit(WORK_STRUCT_PENDING,
> -					     &__cbq->work.work.management) &&
> +			if (likely(!work_pending(&__cbq->work.work) &&

That should properly be

	if (likely(!delayed_work_pending(&__cbq->work) && ...

and why the heck was it doing that open-coded int he first place?

HOWEVER, looking even more, why is that thing a "delayed work" at all? All 
the queuing seems to happen with a timeout of zero..

So I _think_ that the proper patch is actually the following, but somebody 
who knows and uses the connector thing should double-check. Please?

			Linus
---
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index b418b16..296f510 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -34,7 +34,7 @@
 void cn_queue_wrapper(struct work_struct *work)
 {
 	struct cn_callback_entry *cbq =
-		container_of(work, struct cn_callback_entry, work.work);
+		container_of(work, struct cn_callback_entry, work);
 	struct cn_callback_data *d = &cbq->data;
 
 	d->callback(d->callback_priv);
@@ -59,13 +59,12 @@ static struct cn_callback_entry *cn_queue_alloc_callback_entry(char *name, struc
 	memcpy(&cbq->id.id, id, sizeof(struct cb_id));
 	cbq->data.callback = callback;
 	
-	INIT_DELAYED_WORK(&cbq->work, &cn_queue_wrapper);
+	INIT_WORK(&cbq->work, &cn_queue_wrapper);
 	return cbq;
 }
 
 static void cn_queue_free_callback(struct cn_callback_entry *cbq)
 {
-	cancel_delayed_work(&cbq->work);
 	flush_workqueue(cbq->pdev->cn_queue);
 
 	kfree(cbq);
diff --git a/drivers/connector/connector.c b/drivers/connector/connector.c
index 5e7cd45..416de8c 100644
--- a/drivers/connector/connector.c
+++ b/drivers/connector/connector.c
@@ -135,17 +135,16 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 	spin_lock_bh(&dev->cbdev->queue_lock);
 	list_for_each_entry(__cbq, &dev->cbdev->queue_list, callback_entry) {
 		if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
-			if (likely(!test_bit(WORK_STRUCT_PENDING,
-					     &__cbq->work.work.management) &&
+			if (likely(!work_pending(&__cbq->work) &&
 					__cbq->data.ddata == NULL)) {
 				__cbq->data.callback_priv = msg;
 
 				__cbq->data.ddata = data;
 				__cbq->data.destruct_data = destruct_data;
 
-				if (queue_delayed_work(
+				if (queue_work(
 					    dev->cbdev->cn_queue,
-					    &__cbq->work, 0))
+					    &__cbq->work))
 					err = 0;
 			} else {
 				struct cn_callback_data *d;
@@ -159,12 +158,12 @@ static int cn_call_callback(struct cn_msg *msg, void (*destruct_data)(void *), v
 					d->destruct_data = destruct_data;
 					d->free = __cbq;
 
-					INIT_DELAYED_WORK(&__cbq->work,
+					INIT_WORK(&__cbq->work,
 							  &cn_queue_wrapper);
 					
-					if (queue_delayed_work(
+					if (queue_work(
 						    dev->cbdev->cn_queue,
-						    &__cbq->work, 0))
+						    &__cbq->work))
 						err = 0;
 					else {
 						kfree(__cbq);
diff --git a/include/linux/connector.h b/include/linux/connector.h
index 3ea1cd5..10eb56b 100644
--- a/include/linux/connector.h
+++ b/include/linux/connector.h
@@ -133,7 +133,7 @@ struct cn_callback_data {
 struct cn_callback_entry {
 	struct list_head callback_entry;
 	struct cn_callback *cb;
-	struct delayed_work work;
+	struct work_struct work;
 	struct cn_queue_dev *pdev;
 
 	struct cn_callback_id id;

Re: [PATCH] fallout from atomic_long_t patch

[Evgeniy Polyakov]

On Sun, Dec 17, 2006 at 09:24:30AM -0800, Linus Torvalds (torvalds@osdl.org) wrote:
> 
> 
> On Sun, 17 Dec 2006, Al Viro wrote:
> > -			if (likely(!test_bit(WORK_STRUCT_PENDING,
> > -					     &__cbq->work.work.management) &&
> > +			if (likely(!work_pending(&__cbq->work.work) &&
> 
> That should properly be
> 
> 	if (likely(!delayed_work_pending(&__cbq->work) && ...
> 
> and why the heck was it doing that open-coded int he first place?
>
> HOWEVER, looking even more, why is that thing a "delayed work" at all? All 
> the queuing seems to happen with a timeout of zero..
> 
> So I _think_ that the proper patch is actually the following, but somebody 
> who knows and uses the connector thing should double-check. Please?

Delayed work was used to play with different timeouts and thus allow to
smooth performance peaks, but then I dropped that idea, so timeout is always
zero.

I posted similar patch today to netdev@, which directly used
work_pending instead of delayed_work_pending(), but if you will figure
this out itself, I'm ok with proposed patch.


> 			Linus

-- 
	Evgeniy Polyakov

Re: [PATCH] fallout from atomic_long_t patch

[Linus Torvalds]

On Sun, 17 Dec 2006, Evgeniy Polyakov wrote:
> 
> Delayed work was used to play with different timeouts and thus allow to
> smooth performance peaks, but then I dropped that idea, so timeout is always
> zero.

Ok, thanks for the explanation.

> I posted similar patch today to netdev@, which directly used
> work_pending instead of delayed_work_pending(), but if you will figure
> this out itself, I'm ok with proposed patch.

If I'm going to get the proper patch from the proper network trees, I'll 
just drop my patch. Whether you replace "delayed_work" with "work_struct" 
or not is not something I really care about - if you think you may want to 
play with the timeout idea in the future, please feel free to continue 
using delayed_work.

But if you do use delayed work, please use the "delayed_work_pending(&x)" 
function, rather than doing "work_pending(&x->work)" and knowing about the 
internals of how the delayed-work structure looks.

So with that out of the way, I'll just expect that I'll get whatever you 
decide on through Davem's git tree, once his drunken holiday revelry is 
over ;)

		Linus

Re: [PATCH] fallout from atomic_long_t patch

[Evgeniy Polyakov]

On Sun, Dec 17, 2006 at 10:08:49AM -0800, Linus Torvalds (torvalds@osdl.org) wrote:
> So with that out of the way, I'll just expect that I'll get whatever you 
> decide on through Davem's git tree, once his drunken holiday revelry is 
> over ;)

This is important process - never interrupt it for things like patches
from external developers :)
I will push it through his tree.

> 		Linus

-- 
	Evgeniy Polyakov