[RFC PATCH 2.6.17-mm1 4/3] ieee1394: convert ieee1394_transactions from semaphores to waitqueue

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

There were 63 instances of counting semaphores used for each hpsb_host.
All of them are now replaced by a single wait_queue.

Note, it is rather unlikely that a process has to wait for a transaction
label to become free.  This condition occurs only if more than 64
transactions are initiated before the receiving node is able to complete
any of them.

A slight change in behaviour is introduced:  Allocation of a tlabel will
fail if a signal was received while the process had to sleep until a
tlabel becomes available.  Previously, the process slept uninterruptibly
on the semaphore until availability of a tlabel.  Furthermore, error
return values of hpsb_get_tlabel() are now -ERESTARTSYS or -EAGAIN
instead of 1.  However an error return value has always been defined as
"non-zero" in the API and all in-tree callers respect that.

The macro HPSB_TPOOL_INIT is no longer necessary since struct hpsb_host
is kzalloc'd.

The spinlock-protected code which accesses tlabel pools can use the
non-atomic variants of bitmap manipulation functions.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/hosts.c                 |    3
 drivers/ieee1394/ieee1394_transactions.c |   82 ++++++++++++-----------
 drivers/ieee1394/ieee1394_types.h        |   10 --
 3 files changed, 46 insertions(+), 49 deletions(-)

Index: linux-2.6.17-mm1/drivers/ieee1394/ieee1394_types.h
===================================================================
--- linux-2.6.17-mm1.orig/drivers/ieee1394/ieee1394_types.h	2006-06-24 11:11:55.000000000 +0200
+++ linux-2.6.17-mm1/drivers/ieee1394/ieee1394_types.h	2006-06-24 18:03:24.000000000 +0200
@@ -23,18 +23,8 @@ struct hpsb_tlabel_pool {
 	u32 next	:6  __attribute__((packed));
 	u32 allocations	:26 __attribute__((packed));
 #endif
-	struct semaphore count;
 };
 
-#define HPSB_TPOOL_INIT(_tp)			\
-do {						\
-	bitmap_zero((_tp)->pool, 64);		\
-	(_tp)->next = 0;			\
-	(_tp)->allocations = 0;			\
-	sema_init(&(_tp)->count, 63);		\
-} while (0)
-
-
 typedef u32 quadlet_t;
 typedef u64 octlet_t;
 typedef u16 nodeid_t;
Index: linux-2.6.17-mm1/drivers/ieee1394/hosts.c
===================================================================
--- linux-2.6.17-mm1.orig/drivers/ieee1394/hosts.c	2006-06-23 17:58:03.000000000 +0200
+++ linux-2.6.17-mm1/drivers/ieee1394/hosts.c	2006-06-24 18:03:24.000000000 +0200
@@ -143,9 +143,6 @@ struct hpsb_host *hpsb_alloc_host(struct
 	for (i = 2; i < 16; i++)
 		h->csr.gen_timestamp[i] = jiffies - 60 * HZ;
 
-	for (i = 0; i < ARRAY_SIZE(h->tpool); i++)
-		HPSB_TPOOL_INIT(&h->tpool[i]);
-
 	atomic_set(&h->generation, 0);
 
 	INIT_WORK(&h->delayed_reset, delayed_reset_bus, h);
Index: linux-2.6.17-mm1/drivers/ieee1394/ieee1394_transactions.c
===================================================================
--- linux-2.6.17-mm1.orig/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 11:12:01.000000000 +0200
+++ linux-2.6.17-mm1/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 18:03:24.000000000 +0200
@@ -9,10 +9,9 @@
  * directory of the kernel sources for details.
  */
 
-#include <linux/sched.h>
 #include <linux/bitops.h>
-#include <linux/smp_lock.h>
-#include <linux/interrupt.h>
+#include <linux/spinlock.h>
+#include <linux/wait.h>
 
 #include <asm/errno.h>
 
@@ -20,8 +19,6 @@
 #include "ieee1394_types.h"
 #include "hosts.h"
 #include "ieee1394_core.h"
-#include "highlevel.h"
-#include "nodemgr.h"
 #include "ieee1394_transactions.h"
 
 #define PREP_ASYNC_HEAD_ADDRESS(tc) \
@@ -32,6 +29,7 @@
         packet->header[2] = addr & 0xffffffff
 
 spinlock_t hpsb_tlabel_lock = SPIN_LOCK_UNLOCKED;
+static DECLARE_WAIT_QUEUE_HEAD(tlabel_wq);
 
 static void fill_async_readquad(struct hpsb_packet *packet, u64 addr)
 {
@@ -116,50 +114,35 @@ static void fill_async_stream_packet(str
 	packet->tcode = TCODE_ISO_DATA;
 }
 
-/**
- * hpsb_get_tlabel - allocate a transaction label
- * @packet: the packet who's tlabel/tpool we set
- *
- * Every asynchronous transaction on the 1394 bus needs a transaction
- * label to match the response to the request.  This label has to be
- * different from any other transaction label in an outstanding request to
- * the same node to make matching possible without ambiguity.
- *
- * There are 64 different tlabels, so an allocated tlabel has to be freed
- * with hpsb_free_tlabel() after the transaction is complete (unless it's
- * reused again for the same target node).
- *
- * Return value: Zero on success, otherwise non-zero. A non-zero return
- * generally means there are no available tlabels. If this is called out
- * of interrupt or atomic context, then it will sleep until can return a
- * tlabel.
- */
-int hpsb_get_tlabel(struct hpsb_packet *packet)
+/* same as hpsb_get_tlabel, except that it returns immediately */
+static int hpsb_get_tlabel_atomic(struct hpsb_packet *packet)
 {
 	unsigned long flags;
 	struct hpsb_tlabel_pool *tp;
 	int tlabel, n = NODEID_TO_NODE(packet->node_id);
 
-	if (unlikely(n == ALL_NODES))
+	/* Broadcast transactions are complete once the request has been sent.
+	 * Use the same transaction label for all broadcast transactions. */
+	if (unlikely(n == ALL_NODES)) {
+		packet->tlabel = 0;
 		return 0;
-	tp = &packet->host->tpool[n];
-
-	if (irqs_disabled() || in_atomic()) {
-		if (down_trylock(&tp->count))
-			return 1;
-	} else {
-		down(&tp->count);
 	}
+	BUG_ON(n > ARRAY_SIZE(packet->host->tpool));
+	tp = &packet->host->tpool[n];
 
 	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
 
 	tlabel = find_next_zero_bit(tp->pool, 64, tp->next);
 	if (tlabel > 63)
 		tlabel = find_first_zero_bit(tp->pool, 64);
+	if (tlabel > 63) {
+		spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
+		return -EAGAIN;
+	}
+	__set_bit(tlabel, tp->pool);
+
 	/* tp->next is 6 bits wide, thus rolls over from 63 to 0 */
 	tp->next = (tlabel + 1);
-	/* Should _never_ happen */
-	BUG_ON(test_and_set_bit(tlabel, tp->pool));
 	tp->allocations++;
 
 	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
@@ -168,6 +151,33 @@ int hpsb_get_tlabel(struct hpsb_packet *
 }
 
 /**
+ * hpsb_get_tlabel - allocate a transaction label
+ * @packet: the packet who's tlabel/tpool we set
+ *
+ * Every asynchronous transaction on the 1394 bus needs a transaction
+ * label to match the response to the request.  This label has to be
+ * different from any other transaction label in an outstanding request to
+ * the same node to make matching possible without ambiguity.
+ *
+ * There are 64 different tlabels, so an allocated tlabel has to be freed
+ * with hpsb_free_tlabel() after the transaction is complete (unless it's
+ * reused again for the same target node).
+ *
+ * Return value: Zero on success, otherwise non-zero. A non-zero return
+ * generally means there are no available tlabels. If this is called out
+ * of interrupt or atomic context, then it will sleep until can return a
+ * tlabel or a signal is received.
+ */
+int hpsb_get_tlabel(struct hpsb_packet *packet)
+{
+	if (irqs_disabled() || in_atomic())
+		return hpsb_get_tlabel_atomic(packet);
+
+	return wait_event_interruptible(tlabel_wq,
+					!hpsb_get_tlabel_atomic(packet));
+}
+
+/**
  * hpsb_free_tlabel - free an allocated transaction label
  * @packet: packet whos tlabel/tpool needs to be cleared
  *
@@ -191,10 +201,10 @@ void hpsb_free_tlabel(struct hpsb_packet
 	BUG_ON(tlabel > 63 || tlabel < 0);
 
 	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
-	BUG_ON(!test_and_clear_bit(tlabel, tp->pool));
+	BUG_ON(!__test_and_clear_bit(tlabel, tp->pool));
 	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
 
-	up(&tp->count);
+	wake_up_interruptible(&tlabel_wq);
 }
 
 int hpsb_packet_success(struct hpsb_packet *packet)


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/