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

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

[Stefan Richter]

Date: Sun, 19 Aug 2012 02:50:02 -0400
From: Peter Hurley <peter@hurleysoftware.com>

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.)

Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
Peter, are you OK with the new changelog?

 drivers/firewire/core-transaction.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -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);
 }


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

[PATCH] firewire: addendum to address handler RCU conversion

[Stefan Richter]

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.

Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
---
 drivers/firewire/core-transaction.c |   13 ++++++++-----
 include/linux/firewire.h            |   12 ++++++++++--
 2 files changed, 18 insertions(+), 7 deletions(-)

--- a/drivers/firewire/core-transaction.c
+++ b/drivers/firewire/core-transaction.c
@@ -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);
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -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,


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

Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU

[Stefan Richter]

On Sep 27 Stefan Richter wrote:
> --- a/drivers/firewire/core-transaction.c
> +++ b/drivers/firewire/core-transaction.c
> @@ -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.
-- 
Stefan Richter
-=====-===-- =--= ===--
http://arcgraph.de/sr/

Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU

[Peter Hurley]

On Fri, 2012-09-28 at 05:38 -0400, Stefan Richter wrote:
> On Sep 27 Stefan Richter wrote:
> > --- a/drivers/firewire/core-transaction.c
> > +++ b/drivers/firewire/core-transaction.c
> > @@ -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.

Thanks Stefan. Apologies for missing that.

Peter Hurley

Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU

[Peter Hurley]

On Thu, 2012-09-27 at 15:44 -0400, Stefan Richter wrote:
> Date: Sun, 19 Aug 2012 02:50:02 -0400
> From: Peter Hurley <peter@hurleysoftware.com>
> 
> 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 ...]
> 
> (Changelog rewritten by Stefan R.)
> 
> Signed-off-by: Peter Hurley <peter@hurleysoftware.com>
> Signed-off-by: Stefan Richter <stefanr@s5r6.in-berlin.de>
> ---
> Peter, are you OK with the new changelog?

Yes, that's ok with me.

Peter Hurley