[RFC PATCH 2.6.17-mm1 1/3] ieee1394: reduce size of hpsb_host by 252 bytes

[RFC PATCH 2.6.17-mm1 1/3] ieee1394: reduce size of hpsb_host by 252 bytes

[Stefan Richter]

Struct hpsb_host contains struct hpsb_tlabel_pool 63 times, therefore
each word that can be shaved off of hpsb_tlabel_pool is a win.

Since hpsb_tlabel_pool.next is accessed twice as often as .allocations,
.next is put into the lower part of the word.  The value of .allocations
rolls over after about 67 million now but we don't care.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/ieee1394_transactions.c |   22 ++++++++++++----------
 drivers/ieee1394/ieee1394_types.h        |    9 +++++++--
 2 files changed, 19 insertions(+), 12 deletions(-)

Index: linux/drivers/ieee1394/ieee1394_types.h
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_types.h	2006-04-24 22:20:24.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_types.h	2006-06-24 09:52:37.000000000 +0200
@@ -17,8 +17,13 @@
 struct hpsb_tlabel_pool {
 	DECLARE_BITMAP(pool, 64);
 	spinlock_t lock;
-	u8 next;
-	u32 allocations;
+#ifdef __BIG_ENDIAN_BITFIELD
+	u32 allocations	:26 __attribute__((packed));
+	u32 next	:6  __attribute__((packed));
+#else
+	u32 next	:6  __attribute__((packed));
+	u32 allocations	:26 __attribute__((packed));
+#endif
 	struct semaphore count;
 };
 
Index: linux/drivers/ieee1394/ieee1394_transactions.c
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_transactions.c	2006-06-23 19:10:30.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 09:24:00.000000000 +0200
@@ -136,7 +136,7 @@ int hpsb_get_tlabel(struct hpsb_packet *
 {
 	unsigned long flags;
 	struct hpsb_tlabel_pool *tp;
-	int n = NODEID_TO_NODE(packet->node_id);
+	int tlabel, n = NODEID_TO_NODE(packet->node_id);
 
 	if (unlikely(n == ALL_NODES))
 		return 0;
@@ -151,15 +151,17 @@ int hpsb_get_tlabel(struct hpsb_packet *
 
 	spin_lock_irqsave(&tp->lock, flags);
 
-	packet->tlabel = find_next_zero_bit(tp->pool, 64, tp->next);
-	if (packet->tlabel > 63)
-		packet->tlabel = find_first_zero_bit(tp->pool, 64);
-	tp->next = (packet->tlabel + 1) % 64;
+	tlabel = find_next_zero_bit(tp->pool, 64, tp->next);
+	if (tlabel > 63)
+		tlabel = find_first_zero_bit(tp->pool, 64);
+	/* 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(packet->tlabel, tp->pool));
+	BUG_ON(test_and_set_bit(tlabel, tp->pool));
 	tp->allocations++;
-	spin_unlock_irqrestore(&tp->lock, flags);
 
+	spin_unlock_irqrestore(&tp->lock, flags);
+	packet->tlabel = tlabel;
 	return 0;
 }
 
@@ -178,16 +180,16 @@ void hpsb_free_tlabel(struct hpsb_packet
 {
 	unsigned long flags;
 	struct hpsb_tlabel_pool *tp;
-	int n = NODEID_TO_NODE(packet->node_id);
+	int tlabel = packet->tlabel, n = NODEID_TO_NODE(packet->node_id);
 
 	if (unlikely(n == ALL_NODES))
 		return;
 	tp = &packet->host->tpool[n];
 
-	BUG_ON(packet->tlabel > 63 || packet->tlabel < 0);
+	BUG_ON(tlabel > 63 || tlabel < 0);
 
 	spin_lock_irqsave(&tp->lock, flags);
-	BUG_ON(!test_and_clear_bit(packet->tlabel, tp->pool));
+	BUG_ON(!test_and_clear_bit(tlabel, tp->pool));
 	spin_unlock_irqrestore(&tp->lock, flags);
 
 	up(&tp->count);

[RFC PATCH 2.6.17-mm1 2/3] ieee1394: coarser locking for tlabel allocation

[Stefan Richter]

Full parallelism in getting and freeing transaction labels of different
nodes is not really required.  Therefore reduce the number of locks for
access to tlabel pools from 63 locks per host to 1 global lock.

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

Index: linux/drivers/ieee1394/ieee1394_types.h
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_types.h	2006-06-24 09:53:24.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_types.h	2006-06-24 10:02:15.000000000 +0200
@@ -16,7 +16,6 @@
 /* Transaction Label handling */
 struct hpsb_tlabel_pool {
 	DECLARE_BITMAP(pool, 64);
-	spinlock_t lock;
 #ifdef __BIG_ENDIAN_BITFIELD
 	u32 allocations	:26 __attribute__((packed));
 	u32 next	:6  __attribute__((packed));
@@ -30,7 +29,6 @@ struct hpsb_tlabel_pool {
 #define HPSB_TPOOL_INIT(_tp)			\
 do {						\
 	bitmap_zero((_tp)->pool, 64);		\
-	spin_lock_init(&(_tp)->lock);		\
 	(_tp)->next = 0;			\
 	(_tp)->allocations = 0;			\
 	sema_init(&(_tp)->count, 63);		\
Index: linux/drivers/ieee1394/ieee1394_transactions.c
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 09:24:00.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 10:08:35.000000000 +0200
@@ -31,6 +31,8 @@
         packet->header[1] = (packet->host->node_id << 16) | (addr >> 32); \
         packet->header[2] = addr & 0xffffffff
 
+static spinlock_t hpsb_tlabel_lock = SPIN_LOCK_UNLOCKED;
+
 static void fill_async_readquad(struct hpsb_packet *packet, u64 addr)
 {
 	PREP_ASYNC_HEAD_ADDRESS(TCODE_READQ);
@@ -149,7 +151,7 @@ int hpsb_get_tlabel(struct hpsb_packet *
 		down(&tp->count);
 	}
 
-	spin_lock_irqsave(&tp->lock, flags);
+	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
 
 	tlabel = find_next_zero_bit(tp->pool, 64, tp->next);
 	if (tlabel > 63)
@@ -160,7 +162,7 @@ int hpsb_get_tlabel(struct hpsb_packet *
 	BUG_ON(test_and_set_bit(tlabel, tp->pool));
 	tp->allocations++;
 
-	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
 	packet->tlabel = tlabel;
 	return 0;
 }
@@ -188,9 +190,9 @@ void hpsb_free_tlabel(struct hpsb_packet
 
 	BUG_ON(tlabel > 63 || tlabel < 0);
 
-	spin_lock_irqsave(&tp->lock, flags);
+	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
 	BUG_ON(!test_and_clear_bit(tlabel, tp->pool));
-	spin_unlock_irqrestore(&tp->lock, flags);
+	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
 
 	up(&tp->count);
 }

[RFC PATCH 2.6.17-mm1 3/3] ieee1394: nodemgr: read tlabel attributes atomically

[Stefan Richter]

A node_entry's sysfs attributes tlabels_free, tlabels_allocations, and
tlabels_mask may theoretically be sometimes wrong if they were read
without taking the respective lock.  Although this is not very serious
since these attributes are rarely accessed and not interesting outside
of driver or application development, why not do it properly.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
applies after patch "ieee1394: nodemgr: do not peek into struct semaphore"
http://marc.theaimsgroup.com/?l=linux-mm-commits&m=115113030025655

 drivers/ieee1394/ieee1394_transactions.c |    2 -
 drivers/ieee1394/ieee1394_transactions.h |    5 +++
 drivers/ieee1394/nodemgr.c               |   31 ++++++++++++++++++-----
 3 files changed, 31 insertions(+), 7 deletions(-)

Index: linux/drivers/ieee1394/ieee1394_transactions.h
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_transactions.h	2006-04-24 22:20:24.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_transactions.h	2006-06-24 11:00:23.000000000 +0200
@@ -1,6 +1,11 @@
 #ifndef _IEEE1394_TRANSACTIONS_H
 #define _IEEE1394_TRANSACTIONS_H
 
+#include <linux/spinlock_types.h>
+
+/* internal to ieee1394 core */
+extern spinlock_t hpsb_tlabel_lock;
+
 #include "ieee1394_core.h"
 
 
Index: linux/drivers/ieee1394/ieee1394_transactions.c
===================================================================
--- linux.orig/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 10:08:35.000000000 +0200
+++ linux/drivers/ieee1394/ieee1394_transactions.c	2006-06-24 10:30:00.000000000 +0200
@@ -31,7 +31,7 @@
         packet->header[1] = (packet->host->node_id << 16) | (addr >> 32); \
         packet->header[2] = addr & 0xffffffff
 
-static spinlock_t hpsb_tlabel_lock = SPIN_LOCK_UNLOCKED;
+spinlock_t hpsb_tlabel_lock = SPIN_LOCK_UNLOCKED;
 
 static void fill_async_readquad(struct hpsb_packet *packet, u64 addr)
 {
Index: linux/drivers/ieee1394/nodemgr.c
===================================================================
--- linux.orig/drivers/ieee1394/nodemgr.c	2006-06-23 19:10:32.000000000 +0200
+++ linux/drivers/ieee1394/nodemgr.c	2006-06-24 10:43:18.000000000 +0200
@@ -327,12 +327,17 @@ static ssize_t fw_show_ne_bus_options(st
 static DEVICE_ATTR(bus_options,S_IRUGO,fw_show_ne_bus_options,NULL);
 
 
-/* tlabels_free, tlabels_allocations, tlabels_mask are read non-atomically
- * here, therefore displayed values may be occasionally wrong. */
 static ssize_t fw_show_ne_tlabels_free(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct node_entry *ne = container_of(dev, struct node_entry, device);
-	return sprintf(buf, "%d\n", 64 - bitmap_weight(ne->tpool->pool, 64));
+	unsigned long flags;
+	int tf;
+
+	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
+	tf = 64 - bitmap_weight(ne->tpool->pool, 64);
+	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
+
+	return sprintf(buf, "%d\n", tf);
 }
 static DEVICE_ATTR(tlabels_free,S_IRUGO,fw_show_ne_tlabels_free,NULL);
 
@@ -340,7 +345,14 @@ static DEVICE_ATTR(tlabels_free,S_IRUGO,
 static ssize_t fw_show_ne_tlabels_allocations(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct node_entry *ne = container_of(dev, struct node_entry, device);
-	return sprintf(buf, "%u\n", ne->tpool->allocations);
+	unsigned long flags;
+	int ta;
+
+	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
+	ta = ne->tpool->allocations;
+	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
+
+	return sprintf(buf, "%u\n", ta);
 }
 static DEVICE_ATTR(tlabels_allocations,S_IRUGO,fw_show_ne_tlabels_allocations,NULL);
 
@@ -348,11 +360,18 @@ static DEVICE_ATTR(tlabels_allocations,S
 static ssize_t fw_show_ne_tlabels_mask(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct node_entry *ne = container_of(dev, struct node_entry, device);
+	unsigned long flags;
+	u64 tm;
+
+	spin_lock_irqsave(&hpsb_tlabel_lock, flags);
 #if (BITS_PER_LONG <= 32)
-	return sprintf(buf, "0x%08lx%08lx\n", ne->tpool->pool[0], ne->tpool->pool[1]);
+	tm = ((u64)ne->tpool->pool[0] << 32) + ne->tpool->pool[1];
 #else
-	return sprintf(buf, "0x%016lx\n", ne->tpool->pool[0]);
+	tm = ne->tpool->pool[0];
 #endif
+	spin_unlock_irqrestore(&hpsb_tlabel_lock, flags);
+
+	return sprintf(buf, "0x%016llx\n", tm);
 }
 static DEVICE_ATTR(tlabels_mask, S_IRUGO, fw_show_ne_tlabels_mask, NULL);
 

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

[Stefan Richter]

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)

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

[Stefan Richter]

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

After this and patch "ieee1394: dv1394: sem2mutex conversion", the 
following semaphores remain in the ieee1394 subsystem:

highlevel.c:  hl_drivers_sem    (RW semaphore)
nodemgr.c:    subsys.rwsem      (driver core's RW semaphores)
raw1394.c:    fi->complete_sem  (completion semaphore)
-- 
Stefan Richter
-=====-=-==- -==- ==---
http://arcgraph.de/sr/

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

[Arjan van de Ven]

On Sat, 2006-06-24 at 19:45 +0200, Stefan Richter wrote:
> I wrote:
> > There were 63 instances of counting semaphores used for each hpsb_host.
> > All of them are now replaced by a single wait_queue.
> 
> After this and patch "ieee1394: dv1394: sem2mutex conversion", the 
> following semaphores remain in the ieee1394 subsystem:
> 
> highlevel.c:  hl_drivers_sem    (RW semaphore)
> nodemgr.c:    subsys.rwsem      (driver core's RW semaphores)
> raw1394.c:    fi->complete_sem  (completion semaphore)

Hi,

can this last one move to an actual completion? That would get rid of it
nicely ;)

Greetings,
   Arjan van de Ven

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

[Stefan Richter]

Arjan van de Ven wrote:
> On Sat, 2006-06-24 at 19:45 +0200, Stefan Richter wrote:
>>following semaphores remain in the ieee1394 subsystem:
>>
>>highlevel.c:  hl_drivers_sem    (RW semaphore)
>>nodemgr.c:    subsys.rwsem      (driver core's RW semaphores)
>>raw1394.c:    fi->complete_sem  (completion semaphore)

> can this last one move to an actual completion? That would get rid of it
> nicely ;)

Hmm. There are dozens of points in raw1394 which call 
__queue_complete_req() which up()s the semaphore. ("fi" is the 
private_data of a struct file. Multiple outstanding requests may be 
associated with a file.) Then there are two places which wait on the 
semaphore:

raw1394_read(), called when somebody reads /dev/raw1394:
	if (file->f_flags & O_NONBLOCK) {
		if (down_trylock(&fi->complete_sem))
			return -EAGAIN;
	} else {
		if (down_interruptible(&fi->complete_sem))
			return -ERESTARTSYS;
	}

raw1394_release(), called when somebody closes (releases) /dev/raw1394:
	done = 0;
	while (!done) {
		/* free all complete requests */
		/* set "done" if there are no more pending requests */
		if (!done)
			down_interruptible(&fi->complete_sem);
	}
	/* cleanup, free fi */

It looks like fi->complete_sem is a actually a counting semaphore. It 
could perhaps be replaced by a wait queue plus an atomic counter. There 
is even already a wait queue in "fi" for use with poll_wait() via 
raw1394_poll().
-- 
Stefan Richter
-=====-=-==- -==- ==---
http://arcgraph.de/sr/

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

[Stefan Richter]

I wrote:
> It looks like fi->complete_sem is a actually a counting semaphore. It 
> could perhaps be replaced by a wait queue plus an atomic counter.

...which would be a semaphore. %-|
-- 
Stefan Richter
-=====-=-==- -==- ==---
http://arcgraph.de/sr/

[RFC PATCH 2.6.17-mm1 5/3] ieee1394: raw1394: remove redundant counting semaphore

[Stefan Richter]

An already existing wait queue replaces raw1394's complete_sem which was
maintained in parallel to the wait queue.  The role of the semaphore's
counter is taken over by a direct check of what was really counted:  The
presence of items in the list of completed requests.

Notes:

 - raw1394_release() sleeps uninterruptibly until all requests were
   completed.  This is the same behaviour as before the patch.

 - The macros wait_event and wait_event_interruptible are called with a
   condition argument which has a side effect, i.e. manipulation of the
   requests list.  This side effect happens only if the condition is
   true.  The patch relies on the fact that wait_event[_interruptible]
   does not evaluate the condition again after it became true.

 - The diffstat looks unfavorable with respect to added lines of code.
   However 6 of them are comments, and some are due to separation of
   existing code blocks into two small helper functions.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/ieee1394/raw1394-private.h |    3 -
 drivers/ieee1394/raw1394.c         |   79 ++++++++++++++++-------------
 2 files changed, 46 insertions(+), 36 deletions(-)

This patch won't apply without the -mm patch because of the list_move
patch in -mm.  Variants of this and other 1394 patches for older kernels
can be found at http://me.in-berlin.de/~s5r6/linux1394/updates/.

I tested the patch on 2.6.16.x SMP PREEMPT x86 K7 uniprocessor with the
libraw1394 apps 1394commander and gscanbus, issuing asynchronous
transactions, bus resets, and PHY packets, which trigger raw1394_open,
raw1394_write, raw1394_read, raw1394_poll, raw1394_release.

Index: linux-2.6.17-mm2/drivers/ieee1394/raw1394-private.h
===================================================================
--- linux-2.6.17-mm2.orig/drivers/ieee1394/raw1394-private.h	2006-06-25 21:10:39.000000000 +0200
+++ linux-2.6.17-mm2/drivers/ieee1394/raw1394-private.h	2006-06-25 21:11:07.000000000 +0200
@@ -29,9 +29,8 @@ struct file_info {
 
         struct list_head req_pending;
         struct list_head req_complete;
-        struct semaphore complete_sem;
         spinlock_t reqlists_lock;
-        wait_queue_head_t poll_wait_complete;
+        wait_queue_head_t wait_complete;
 
         struct list_head addr_list;
 
Index: linux-2.6.17-mm2/drivers/ieee1394/raw1394.c
===================================================================
--- linux-2.6.17-mm2.orig/drivers/ieee1394/raw1394.c	2006-06-25 21:10:39.000000000 +0200
+++ linux-2.6.17-mm2/drivers/ieee1394/raw1394.c	2006-06-25 21:11:41.000000000 +0200
@@ -132,10 +132,9 @@ static void free_pending_request(struct 
 static void __queue_complete_req(struct pending_request *req)
 {
 	struct file_info *fi = req->file_info;
-	list_move_tail(&req->list, &fi->req_complete);
 
-	up(&fi->complete_sem);
-	wake_up_interruptible(&fi->poll_wait_complete);
+	list_move_tail(&req->list, &fi->req_complete);
+ 	wake_up(&fi->wait_complete);
 }
 
 static void queue_complete_req(struct pending_request *req)
@@ -463,13 +462,36 @@ raw1394_compat_read(const char __user *b
 
 #endif
 
+/* get next completed request  (caller must hold fi->reqlists_lock) */
+static inline struct pending_request *__next_complete_req(struct file_info *fi)
+{
+	struct list_head *lh;
+	struct pending_request *req = NULL;
+
+	if (!list_empty(&fi->req_complete)) {
+		lh = fi->req_complete.next;
+		list_del(lh);
+		req = list_entry(lh, struct pending_request, list);
+	}
+	return req;
+}
+
+/* atomically get next completed request */
+static struct pending_request *next_complete_req(struct file_info *fi)
+{
+	unsigned long flags;
+	struct pending_request *req;
+
+	spin_lock_irqsave(&fi->reqlists_lock, flags);
+	req = __next_complete_req(fi);
+	spin_unlock_irqrestore(&fi->reqlists_lock, flags);
+	return req;
+}
 
 static ssize_t raw1394_read(struct file *file, char __user * buffer,
 			    size_t count, loff_t * offset_is_ignored)
 {
-	unsigned long flags;
 	struct file_info *fi = (struct file_info *)file->private_data;
-	struct list_head *lh;
 	struct pending_request *req;
 	ssize_t ret;
 
@@ -487,22 +509,14 @@ static ssize_t raw1394_read(struct file 
 	}
 
 	if (file->f_flags & O_NONBLOCK) {
-		if (down_trylock(&fi->complete_sem)) {
+		if (!(req = next_complete_req(fi)))
 			return -EAGAIN;
-		}
 	} else {
-		if (down_interruptible(&fi->complete_sem)) {
+		if (wait_event_interruptible(fi->wait_complete,
+					     (req = next_complete_req(fi))))
 			return -ERESTARTSYS;
-		}
 	}
 
-	spin_lock_irqsave(&fi->reqlists_lock, flags);
-	lh = fi->req_complete.next;
-	list_del(lh);
-	spin_unlock_irqrestore(&fi->reqlists_lock, flags);
-
-	req = list_entry(lh, struct pending_request, list);
-
 	if (req->req.length) {
 		if (copy_to_user(int2ptr(req->req.recvb), req->data,
 				 req->req.length)) {
@@ -2744,7 +2758,7 @@ static unsigned int raw1394_poll(struct 
 	unsigned int mask = POLLOUT | POLLWRNORM;
 	unsigned long flags;
 
-	poll_wait(file, &fi->poll_wait_complete, pt);
+	poll_wait(file, &fi->wait_complete, pt);
 
 	spin_lock_irqsave(&fi->reqlists_lock, flags);
 	if (!list_empty(&fi->req_complete)) {
@@ -2769,9 +2783,8 @@ static int raw1394_open(struct inode *in
 	fi->state = opened;
 	INIT_LIST_HEAD(&fi->req_pending);
 	INIT_LIST_HEAD(&fi->req_complete);
-	sema_init(&fi->complete_sem, 0);
 	spin_lock_init(&fi->reqlists_lock);
-	init_waitqueue_head(&fi->poll_wait_complete);
+	init_waitqueue_head(&fi->wait_complete);
 	INIT_LIST_HEAD(&fi->addr_list);
 
 	file->private_data = fi;
@@ -2784,7 +2797,7 @@ static int raw1394_release(struct inode 
 	struct file_info *fi = file->private_data;
 	struct list_head *lh;
 	struct pending_request *req;
-	int done = 0, i, fail = 0;
+	int i, fail;
 	int retval = 0;
 	struct list_head *entry;
 	struct arm_addr *addr = NULL;
@@ -2864,25 +2877,23 @@ static int raw1394_release(struct inode 
 		       "error(s) occurred \n");
 	}
 
-	while (!done) {
+	for (;;) {
+		/* This locked section guarantees that neither
+		 * complete nor pending requests exist once i!=0 */
 		spin_lock_irqsave(&fi->reqlists_lock, flags);
-
-		while (!list_empty(&fi->req_complete)) {
-			lh = fi->req_complete.next;
-			list_del(lh);
-
-			req = list_entry(lh, struct pending_request, list);
-
+		while ((req = __next_complete_req(fi)))
 			free_pending_request(req);
-		}
-
-		if (list_empty(&fi->req_pending))
-			done = 1;
 
+		i = list_empty(&fi->req_pending);
 		spin_unlock_irqrestore(&fi->reqlists_lock, flags);
 
-		if (!done)
-			down_interruptible(&fi->complete_sem);
+		if (i)
+			break;
+
+		/* Sleep until more requests can be freed.
+		 * FIXME?  This sleeps uninterruptibly. */
+		wait_event(fi->wait_complete, (req = next_complete_req(fi)));
+		free_pending_request(req);
 	}
 
 	/* Remove any sub-trees left by user space programs */

Re: [RFC PATCH 2.6.17-mm1 5/3] ieee1394: raw1394: remove redundant counting semaphore

[Stefan Richter]

I wrote:
...
> @@ -2864,25 +2877,23 @@ static int raw1394_release(struct inode 
...
> +		/* Sleep until more requests can be freed.
> +		 * FIXME?  This sleeps uninterruptibly. */
> +		wait_event(fi->wait_complete, (req = next_complete_req(fi)));
> +		free_pending_request(req);

We certainly cannot allow anybody to close the file with pending or 
orphaned complete requests, so maybe I should white-out that FIXME.
-- 
Stefan Richter
-=====-=-==- -==- ==--=
http://arcgraph.de/sr/

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

[Stefan Richter]

I wrote on 2006-06-24:
> +	return wait_event_interruptible(tlabel_wq,
> +					!hpsb_get_tlabel_atomic(packet));

Hmm. "Linux Device Drivers" says about wait_event_interruptible(wq, 
condition): ''Note that @condition may be evaluated an arbitrary number 
of times, so it should not have any side effects.''

Alas the hpsb_get_tlabel_atomic() _does_ have a side effect, but only 
when !hpsb_get_tlabel_atomic(packet) is true.

The current implementation of wait_event_interruptible() seems to 
evaluate @condition multiple times if it is false but only _once_ while 
it is true. May I rely on this fact or do I have to rewrite the 
condition to be completely free of side effects?

I don't believe there would be ever a sensible implementation of 
wait_event_interruptible() which would evaluate @condition again after 
it became true.

Thanks,
-- 
Stefan Richter
-=====-=-==- -==- ==--=
http://arcgraph.de/sr/