Skip to contentSkip to conversation

firewire: remove global lock around address handlers, convert to RCU

PATCH In review Stefan Richter 2 participants 5 messages Sep 27, 2012 – Sep 28, 2012 4 messages not in the archive Firewire

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): 1345359002.6089.3.camel@thor

A message that is not in the archive (sent off-list or not yet received): 20120819122759.30f9981f@stein

A message that is not in the archive (sent off-list or not yet received): 1345403610.7706.72.camel@thor

A message that is not in the archive (sent off-list or not yet received): 20120820030450.7f5bfa37@stein

#4 lore raw

PATCH 1/1 firewire: remove global lock around address handlers, convert to RCU

Date: Sun, 19 Aug 2012 02:50:02 -0400
From: Peter Hurley <[email protected]>

Upper-layer handlers for inbound requests were called with a spinlock held by firewire-core. Calling into upper layers with a lower layer lock held is generally a bad idea.

What's more, since commit ea102d0ec475 "firewire: core: convert AR-req handler lock from _irqsave to _bh", a caller of fw_send_request() i.e. initiator of outbound request could no longer do that while having interrupts disabled, if the local node was addressed by that request.

In order to make all this more flexible, convert the management of address ranges and handlers from a global lock around readers and writers to RCU (and a remaining global lock for writers). As a minor side effect, handling of inbound requests at different cards are now no longer serialized. (There is still per-card serialization since firewire-ohci uses a single DMA tasklet for inbound request events.)

In other words, address handlers are now called in an RCU read-side critical section instead of from within a spin_lock_bh serialized section.

(Changelog rewritten by Stefan R.)

---
Peter, are you OK with the new changelog?

 drivers/firewire/core-transaction.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
drivers/firewire/core-transaction.c +10 -9
@@ -489,7 +489,7 @@ static struct fw_address_handler *lookup
 {
 	struct fw_address_handler *handler;
 
-	list_for_each_entry(handler, list, link) {
+	list_for_each_entry_rcu(handler, list, link) {
 		if (handler->offset < offset + length &&
 		    offset < handler->offset + handler->length)
 			return handler;
@@ -510,7 +510,7 @@ static struct fw_address_handler *lookup
 {
 	struct fw_address_handler *handler;
 
-	list_for_each_entry(handler, list, link) {
+	list_for_each_entry_rcu(handler, list, link) {
 		if (is_enclosing_handler(handler, offset, length))
 			return handler;
 	}
@@ -588,7 +588,7 @@ int fw_core_add_address_handler(struct f
 		if (other != NULL) {
 			handler->offset += other->length;
 		} else {
-			list_add_tail(&handler->link, &address_handler_list);
+			list_add_tail_rcu(&handler->link, &address_handler_list);
 			ret = 0;
 			break;
 		}
@@ -609,8 +609,9 @@ EXPORT_SYMBOL(fw_core_add_address_handle
 void fw_core_remove_address_handler(struct fw_address_handler *handler)
 {
 	spin_lock_bh(&address_handler_lock);
-	list_del(&handler->link);
+	list_del_rcu(&handler->link);
 	spin_unlock_bh(&address_handler_lock);
+	synchronize_rcu();
 }
 EXPORT_SYMBOL(fw_core_remove_address_handler);
 
@@ -844,7 +845,7 @@ static void handle_exclusive_region_requ
 	if (tcode == TCODE_LOCK_REQUEST)
 		tcode = 0x10 + HEADER_GET_EXTENDED_TCODE(p->header[3]);
 
-	spin_lock_bh(&address_handler_lock);
+	rcu_read_lock();
 	handler = lookup_enclosing_address_handler(&address_handler_list,
 						   offset, request->length);
 	if (handler)
@@ -853,7 +854,7 @@ static void handle_exclusive_region_requ
 					  p->generation, offset,
 					  request->data, request->length,
 					  handler->callback_data);
-	spin_unlock_bh(&address_handler_lock);
+	rcu_read_unlock();
 
 	if (!handler)
 		fw_send_response(card, request, RCODE_ADDRESS_ERROR);
@@ -886,8 +887,8 @@ static void handle_fcp_region_request(st
 		return;
 	}
 
-	spin_lock_bh(&address_handler_lock);
-	list_for_each_entry(handler, &address_handler_list, link) {
+	rcu_read_lock();
+	list_for_each_entry_rcu(handler, &address_handler_list, link) {
 		if (is_enclosing_handler(handler, offset, request->length))
 			handler->address_callback(card, NULL, tcode,
 						  destination, source,
@@ -896,7 +897,7 @@ static void handle_fcp_region_request(st
 						  request->length,
 						  handler->callback_data);
 	}
-	spin_unlock_bh(&address_handler_lock);
+	rcu_read_unlock();
 
 	fw_send_response(card, request, RCODE_COMPLETE);
 }
Signature · 3 lines
Stefan Richter
-=====-===-- =--= ==-==
http://arcgraph.de/sr/
#5 lore raw

PATCH 1/1 firewire: addendum to address handler RCU conversion

Follow up on commit "firewire: remove global lock around address handlers, convert to RCU":

- address_handler_lock no longer serializes the address handler, only its function to serialize updates to the list of handlers remains. Rename the lock to address_handler_list_lock.

- Callers of fw_core_remove_address_handler() must be able to sleep. Comment on this in the API documentation.

- The counterpart fw_core_add_address_handler() is by nature something which is used in process context. Replace spin_lock_bh() by spin_lock() in fw_core_add_address_handler() and in fw_core_remove_address_handler(), and document that process context is now required for fw_core_add_address_handler().

- Extend the documentation of fw_address_callback_t.

---
 drivers/firewire/core-transaction.c |   13 ++++++++-----
 include/linux/firewire.h            |   12 ++++++++++--
 2 files changed, 18 insertions(+), 7 deletions(-)
drivers/firewire/core-transaction.c +8 -5
@@ -518,7 +518,7 @@ static struct fw_address_handler *lookup
 	return NULL;
 }
 
-static DEFINE_SPINLOCK(address_handler_lock);
+static DEFINE_SPINLOCK(address_handler_list_lock);
 static LIST_HEAD(address_handler_list);
 
 const struct fw_address_region fw_high_memory_region =
@@ -555,6 +555,7 @@ static bool is_in_fcp_region(u64 offset,
  * the specified callback is invoked.  The parameters passed to the callback
  * give the details of the particular request.
  *
+ * To be called in process context.
  * Return value:  0 on success, non-zero otherwise.
  *
  * The start offset of the handler's address region is determined by
@@ -575,7 +576,7 @@ int fw_core_add_address_handler(struct f
 	    handler->length == 0)
 		return -EINVAL;
 
-	spin_lock_bh(&address_handler_lock);
+	spin_lock(&address_handler_list_lock);
 
 	handler->offset = region->start;
 	while (handler->offset + handler->length <= region->end) {
@@ -594,7 +595,7 @@ int fw_core_add_address_handler(struct f
 		}
 	}
 
-	spin_unlock_bh(&address_handler_lock);
+	spin_unlock(&address_handler_list_lock);
 
 	return ret;
 }
@@ -603,14 +604,16 @@ EXPORT_SYMBOL(fw_core_add_address_handle
 /**
  * fw_core_remove_address_handler() - unregister an address handler
  *
+ * To be called in process context.
+ *
  * When fw_core_remove_address_handler() returns, @handler->callback() is
  * guaranteed to not run on any CPU anymore.
  */
 void fw_core_remove_address_handler(struct fw_address_handler *handler)
 {
-	spin_lock_bh(&address_handler_lock);
+	spin_lock(&address_handler_list_lock);
 	list_del_rcu(&handler->link);
-	spin_unlock_bh(&address_handler_lock);
+	spin_unlock(&address_handler_list_lock);
 	synchronize_rcu();
 }
 EXPORT_SYMBOL(fw_core_remove_address_handler);
include/linux/firewire.h +10 -2
@@ -265,8 +265,16 @@ typedef void (*fw_transaction_callback_t
 					  void *data, size_t length,
 					  void *callback_data);
 /*
- * Important note:  Except for the FCP registers, the callback must guarantee
- * that either fw_send_response() or kfree() is called on the @request.
+ * This callback handles an inbound request subaction.  It is called in
+ * RCU read-side context, therefore must not sleep.
+ *
+ * The callback should not initiate outbound request subactions directly.
+ * Otherwise there is a danger of recursion of inbound and outbound
+ * transactions from and to the local node.
+ *
+ * The callback is responsible that either fw_send_response() or kfree()
+ * is called on the @request, except for FCP registers for which the core
+ * takes care of that.
  */
 typedef void (*fw_address_callback_t)(struct fw_card *card,
 				      struct fw_request *request,
Signature · 3 lines
Stefan Richter
-=====-===-- =--= ==-==
http://arcgraph.de/sr/
#6 lore raw
In reply to Stefan Richter

drivers/firewire/core-transaction.c static struct fw_address_handler *lookup · line 495

Quoted 11 lines · On Sep 27 Stefan Richter wrote:
drivers/firewire/core-transaction.c +1 -1
@@ -489,7 +489,7 @@ static struct fw_address_handler *lookup
 {
 	struct fw_address_handler *handler;
 
-	list_for_each_entry(handler, list, link) {
+	list_for_each_entry_rcu(handler, list, link) {
 		if (handler->offset < offset + length &&
 		    offset < handler->offset + handler->length)
 			return handler;

[...]

I will add an #include <linux/rculist.h> to core-transaction.c.

Signature · 3 lines
Stefan Richter
-=====-===-- =--= ===--
http://arcgraph.de/sr/
#7 lore raw
Quoted 7 lines · Stefan Richter
Quoted 6 lines · On Sep 27 Stefan Richter wrote:
drivers/firewire/core-transaction.c +0 -0
@@ -489,7 +489,7 @@ static struct fw_address_handler *lookup
 {
 	struct fw_address_handler *handler;
 
Quoted 8 lines

drivers/firewire/core-transaction.c static struct fw_address_handler *lookup · line 495

Quoted 5 lines
drivers/firewire/core-transaction.c +1 -1
-	list_for_each_entry(handler, list, link) {
+	list_for_each_entry_rcu(handler, list, link) {
 		if (handler->offset < offset + length &&
 		    offset < handler->offset + handler->length)
 			return handler;

[...]

I will add an #include <linux/rculist.h> to core-transaction.c.

Thanks Stefan. Apologies for missing that.

Peter Hurley

#8 lore raw
In reply to Stefan Richter
Quoted 6 lines · Stefan Richter

Date: Sun, 19 Aug 2012 02:50:02 -0400
From: Peter Hurley <[email protected]>

Upper-layer handlers for inbound requests were called with a spinlock held by firewire-core. Calling into upper layers with a lower layer lock held is generally a bad idea.

[... snip ...]

Quoted 7 lines

(Changelog rewritten by Stefan R.)

---
Peter, are you OK with the new changelog?

Yes, that's ok with me.

Peter Hurley