* [PATCH] firewire: remove global lock around address handlers, convert to RCU [not found] ` <20120820030450.7f5bfa37@stein> @ 2012-09-27 19:44 ` Stefan Richter 2012-09-27 19:46 ` [PATCH] firewire: addendum to address handler RCU conversion Stefan Richter ` (2 more replies) 0 siblings, 3 replies; 5+ messages in thread From: Stefan Richter @ 2012-09-27 19:44 UTC (permalink / raw) To: linux1394-devel; +Cc: Peter Hurley, linux-kernel 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] firewire: addendum to address handler RCU conversion 2012-09-27 19:44 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter @ 2012-09-27 19:46 ` Stefan Richter 2012-09-28 9:38 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter 2012-09-28 17:05 ` Peter Hurley 2 siblings, 0 replies; 5+ messages in thread From: Stefan Richter @ 2012-09-27 19:46 UTC (permalink / raw) To: linux1394-devel; +Cc: Peter Hurley, linux-kernel 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU 2012-09-27 19:44 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter 2012-09-27 19:46 ` [PATCH] firewire: addendum to address handler RCU conversion Stefan Richter @ 2012-09-28 9:38 ` Stefan Richter 2012-09-28 17:03 ` Peter Hurley 2012-09-28 17:05 ` Peter Hurley 2 siblings, 1 reply; 5+ messages in thread From: Stefan Richter @ 2012-09-28 9:38 UTC (permalink / raw) To: linux1394-devel; +Cc: Peter Hurley, linux-kernel 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/ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU 2012-09-28 9:38 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter @ 2012-09-28 17:03 ` Peter Hurley 0 siblings, 0 replies; 5+ messages in thread From: Peter Hurley @ 2012-09-28 17:03 UTC (permalink / raw) To: Stefan Richter Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] firewire: remove global lock around address handlers, convert to RCU 2012-09-27 19:44 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter 2012-09-27 19:46 ` [PATCH] firewire: addendum to address handler RCU conversion Stefan Richter 2012-09-28 9:38 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter @ 2012-09-28 17:05 ` Peter Hurley 2 siblings, 0 replies; 5+ messages in thread From: Peter Hurley @ 2012-09-28 17:05 UTC (permalink / raw) To: Stefan Richter Cc: linux1394-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-09-28 17:05 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1345359002.6089.3.camel@thor>
[not found] ` <20120819122759.30f9981f@stein>
[not found] ` <1345403610.7706.72.camel@thor>
[not found] ` <20120820030450.7f5bfa37@stein>
2012-09-27 19:44 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter
2012-09-27 19:46 ` [PATCH] firewire: addendum to address handler RCU conversion Stefan Richter
2012-09-28 9:38 ` [PATCH] firewire: remove global lock around address handlers, convert to RCU Stefan Richter
2012-09-28 17:03 ` Peter Hurley
2012-09-28 17:05 ` Peter Hurley
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.