firewire: remove global lock around address handlers, convert to RCU
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
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 fif (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_handlevoid 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_requif (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(streturn ; } -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/
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 *lookupreturn 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/
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/
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> tocore-transaction.c.
Thanks Stefan. Apologies for missing that.
Peter Hurley
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.)
Signed-off-by Peter Hurley <[email protected]>Signed-off-by Stefan Richter <[email protected]>---
Peter, are you OK with the new changelog?
Yes, that's ok with me.
Peter Hurley