Thread: [PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()
Brought to you by:
aeb,
bencollins
From: Takashi S. <o-t...@sa...> - 2024-09-09 14:00:29
|
Hi, It seems to be the last week for v6.12 development. I realize it unpreferable to propose intrusive changes, however I also realized that there is a room to refactor core functions in respect to handler of work item for isochronous context for the next merge window... This series of changes refactors the core function to call fw_iso_context_flush_completions() from the work item. It optimizes some event waiting and mediation of concurrent calls as well. Takashi Sakamoto (2): firewire: core: move workqueue handler from 1394 OHCI driver to core function firewire: core: use mutex to coordinate concurrent calls to flush completions drivers/firewire/core-iso.c | 31 ++++++++------- drivers/firewire/core.h | 5 --- drivers/firewire/ohci.c | 78 +++++++------------------------------ include/linux/firewire.h | 1 + 4 files changed, 31 insertions(+), 84 deletions(-) -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-09-09 14:00:31
|
In current implementation, the work item for isochronous context executes the same procedure of fw_iso_context_flush_completions() internally. There is a space to refactor the implementation. This commit calls fw_iso_context_flush_completions() in the work item. It obsoletes fw_iso_context_init_work(). It also obsoletes a pair of disable_work_sync() and enable_work() since the usage of test_and_set_bit_lock() mediates concurrent call already. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-iso.c | 26 +++++++++------------ drivers/firewire/core.h | 5 ----- drivers/firewire/ohci.c | 45 ++----------------------------------- 3 files changed, 12 insertions(+), 64 deletions(-) diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c index f2394f3ed194..9f41c78878ad 100644 --- a/drivers/firewire/core-iso.c +++ b/drivers/firewire/core-iso.c @@ -131,6 +131,13 @@ size_t fw_iso_buffer_lookup(struct fw_iso_buffer *buffer, dma_addr_t completed) return 0; } +static void flush_completions_work(struct work_struct *work) +{ + struct fw_iso_context *ctx = container_of(work, struct fw_iso_context, work); + + fw_iso_context_flush_completions(ctx); +} + struct fw_iso_context *fw_iso_context_create(struct fw_card *card, int type, int channel, int speed, size_t header_size, fw_iso_callback_t callback, void *callback_data) @@ -149,6 +156,7 @@ struct fw_iso_context *fw_iso_context_create(struct fw_card *card, ctx->header_size = header_size; ctx->callback.sc = callback; ctx->callback_data = callback_data; + INIT_WORK(&ctx->work, flush_completions_work); trace_isoc_outbound_allocate(ctx, channel, speed); trace_isoc_inbound_single_allocate(ctx, channel, header_size); @@ -218,29 +226,15 @@ EXPORT_SYMBOL(fw_iso_context_queue_flush); * to process the context asynchronously, fw_iso_context_schedule_flush_completions() is available * instead. * - * Context: Process context. May sleep due to disable_work_sync(). + * Context: Process context. */ int fw_iso_context_flush_completions(struct fw_iso_context *ctx) { - int err; - trace_isoc_outbound_flush_completions(ctx); trace_isoc_inbound_single_flush_completions(ctx); trace_isoc_inbound_multiple_flush_completions(ctx); - might_sleep(); - - // Avoid dead lock due to programming mistake. - if (WARN_ON_ONCE(current_work() == &ctx->work)) - return 0; - - disable_work_sync(&ctx->work); - - err = ctx->card->driver->flush_iso_completions(ctx); - - enable_work(&ctx->work); - - return err; + return ctx->card->driver->flush_iso_completions(ctx); } EXPORT_SYMBOL(fw_iso_context_flush_completions); diff --git a/drivers/firewire/core.h b/drivers/firewire/core.h index 0ae2c84ecafe..96ae366889e0 100644 --- a/drivers/firewire/core.h +++ b/drivers/firewire/core.h @@ -159,11 +159,6 @@ int fw_iso_buffer_alloc(struct fw_iso_buffer *buffer, int page_count); int fw_iso_buffer_map_dma(struct fw_iso_buffer *buffer, struct fw_card *card, enum dma_data_direction direction); -static inline void fw_iso_context_init_work(struct fw_iso_context *ctx, work_func_t func) -{ - INIT_WORK(&ctx->work, func); -} - /* -topology */ diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 3a911cfb5ff3..02ff0363d3ad 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -1182,47 +1182,6 @@ static void context_tasklet(unsigned long data) } } -static void ohci_isoc_context_work(struct work_struct *work) -{ - struct fw_iso_context *base = container_of(work, struct fw_iso_context, work); - struct iso_context *isoc_ctx = container_of(base, struct iso_context, base); - struct context *ctx = &isoc_ctx->context; - struct descriptor *d, *last; - u32 address; - int z; - struct descriptor_buffer *desc; - - desc = list_entry(ctx->buffer_list.next, struct descriptor_buffer, list); - last = ctx->last; - while (last->branch_address != 0) { - struct descriptor_buffer *old_desc = desc; - - address = le32_to_cpu(last->branch_address); - z = address & 0xf; - address &= ~0xf; - ctx->current_bus = address; - - // If the branch address points to a buffer outside of the current buffer, advance - // to the next buffer. - if (address < desc->buffer_bus || address >= desc->buffer_bus + desc->used) - desc = list_entry(desc->list.next, struct descriptor_buffer, list); - d = desc->buffer + (address - desc->buffer_bus) / sizeof(*d); - last = find_branch_descriptor(d, z); - - if (!ctx->callback(ctx, d, last)) - break; - - if (old_desc != desc) { - // If we've advanced to the next buffer, move the previous buffer to the - // free list. - old_desc->used = 0; - guard(spinlock_irqsave)(&ctx->ohci->lock); - list_move_tail(&old_desc->list, &ctx->buffer_list); - } - ctx->last = last; - } -} - /* * Allocate a new buffer and add it to the list of free buffers for this * context. Must be called with ohci->lock held. @@ -3169,7 +3128,6 @@ static struct fw_iso_context *ohci_allocate_iso_context(struct fw_card *card, ret = context_init(&ctx->context, ohci, regs, callback); if (ret < 0) goto out_with_header; - fw_iso_context_init_work(&ctx->base, ohci_isoc_context_work); if (type == FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL) { set_multichannel_mask(ohci, 0); @@ -3624,7 +3582,8 @@ static int ohci_flush_iso_completions(struct fw_iso_context *base) int ret = 0; if (!test_and_set_bit_lock(0, &ctx->flushing_completions)) { - ohci_isoc_context_work(&base->work); + // Note that tasklet softIRQ is not used to process isochronous context anymore. + context_tasklet((unsigned long)&ctx->context); switch (base->type) { case FW_ISO_CONTEXT_TRANSMIT: -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-09-09 14:00:37
|
In current implementation, test_and_set_bit_lock() is used to mediate concurrent calls of ohci_flush_iso_completions(). However, the ad-hoc usage of atomic operations is not preferable. This commit uses mutex_trylock() as the similar operations. The core function is responsible for the mediation, instead of 1394 OHCI driver. Signed-off-by: Takashi Sakamoto <o-t...@sa...> --- drivers/firewire/core-iso.c | 11 +++++++++-- drivers/firewire/ohci.c | 37 ++++++++++++++----------------------- include/linux/firewire.h | 1 + 3 files changed, 24 insertions(+), 25 deletions(-) diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c index 9f41c78878ad..a5c5ef3c725d 100644 --- a/drivers/firewire/core-iso.c +++ b/drivers/firewire/core-iso.c @@ -157,6 +157,7 @@ struct fw_iso_context *fw_iso_context_create(struct fw_card *card, ctx->callback.sc = callback; ctx->callback_data = callback_data; INIT_WORK(&ctx->work, flush_completions_work); + mutex_init(&ctx->flushing_completions_mutex); trace_isoc_outbound_allocate(ctx, channel, speed); trace_isoc_inbound_single_allocate(ctx, channel, header_size); @@ -173,6 +174,8 @@ void fw_iso_context_destroy(struct fw_iso_context *ctx) trace_isoc_inbound_multiple_destroy(ctx); ctx->card->driver->free_iso_context(ctx); + + mutex_destroy(&ctx->flushing_completions_mutex); } EXPORT_SYMBOL(fw_iso_context_destroy); @@ -226,7 +229,7 @@ EXPORT_SYMBOL(fw_iso_context_queue_flush); * to process the context asynchronously, fw_iso_context_schedule_flush_completions() is available * instead. * - * Context: Process context. + * Context: Process context due to mutex_trylock(). */ int fw_iso_context_flush_completions(struct fw_iso_context *ctx) { @@ -234,7 +237,11 @@ int fw_iso_context_flush_completions(struct fw_iso_context *ctx) trace_isoc_inbound_single_flush_completions(ctx); trace_isoc_inbound_multiple_flush_completions(ctx); - return ctx->card->driver->flush_iso_completions(ctx); + scoped_cond_guard(mutex_try, /* nothing to do */, &ctx->flushing_completions_mutex) { + return ctx->card->driver->flush_iso_completions(ctx); + } + + return 0; } EXPORT_SYMBOL(fw_iso_context_flush_completions); diff --git a/drivers/firewire/ohci.c b/drivers/firewire/ohci.c index 02ff0363d3ad..b182998a77f4 100644 --- a/drivers/firewire/ohci.c +++ b/drivers/firewire/ohci.c @@ -166,7 +166,6 @@ struct iso_context { struct context context; void *header; size_t header_length; - unsigned long flushing_completions; u32 mc_buffer_bus; u16 mc_completed; u16 last_timestamp; @@ -3579,31 +3578,23 @@ static void ohci_flush_queue_iso(struct fw_iso_context *base) static int ohci_flush_iso_completions(struct fw_iso_context *base) { struct iso_context *ctx = container_of(base, struct iso_context, base); - int ret = 0; - if (!test_and_set_bit_lock(0, &ctx->flushing_completions)) { - // Note that tasklet softIRQ is not used to process isochronous context anymore. - context_tasklet((unsigned long)&ctx->context); + // Note that tasklet softIRQ is not used to process isochronous context anymore. + context_tasklet((unsigned long)&ctx->context); - switch (base->type) { - case FW_ISO_CONTEXT_TRANSMIT: - case FW_ISO_CONTEXT_RECEIVE: - if (ctx->header_length != 0) - flush_iso_completions(ctx, FW_ISO_CONTEXT_COMPLETIONS_CAUSE_FLUSH); - break; - case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL: - if (ctx->mc_completed != 0) - flush_ir_buffer_fill(ctx); - break; - default: - ret = -ENOSYS; - } - - clear_bit_unlock(0, &ctx->flushing_completions); - smp_mb__after_atomic(); + switch (base->type) { + case FW_ISO_CONTEXT_TRANSMIT: + case FW_ISO_CONTEXT_RECEIVE: + if (ctx->header_length != 0) + flush_iso_completions(ctx, FW_ISO_CONTEXT_COMPLETIONS_CAUSE_FLUSH); + return 0; + case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL: + if (ctx->mc_completed != 0) + flush_ir_buffer_fill(ctx); + return 0; + default: + return -ENOSYS; } - - return ret; } static const struct fw_card_driver ohci_driver = { diff --git a/include/linux/firewire.h b/include/linux/firewire.h index f815d12deda0..19e8c5f9537c 100644 --- a/include/linux/firewire.h +++ b/include/linux/firewire.h @@ -512,6 +512,7 @@ union fw_iso_callback { struct fw_iso_context { struct fw_card *card; struct work_struct work; + struct mutex flushing_completions_mutex; int type; int channel; int speed; -- 2.43.0 |
From: Takashi S. <o-t...@sa...> - 2024-09-10 08:51:27
|
On Mon, Sep 09, 2024 at 11:00:16PM +0900, Takashi Sakamoto wrote: > Hi, > > It seems to be the last week for v6.12 development. I realize it > unpreferable to propose intrusive changes, however I also realized that > there is a room to refactor core functions in respect to handler of work > item for isochronous context for the next merge window... > > This series of changes refactors the core function to call > fw_iso_context_flush_completions() from the work item. It optimizes some > event waiting and mediation of concurrent calls as well. > > Takashi Sakamoto (2): > firewire: core: move workqueue handler from 1394 OHCI driver to core > function > firewire: core: use mutex to coordinate concurrent calls to flush > completions > > drivers/firewire/core-iso.c | 31 ++++++++------- > drivers/firewire/core.h | 5 --- > drivers/firewire/ohci.c | 78 +++++++------------------------------ > include/linux/firewire.h | 1 + > 4 files changed, 31 insertions(+), 84 deletions(-) Applied to for-next branch. Regards Takashi Sakamoto |