[PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()

[PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()

[Takashi Sakamoto]

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

[PATCH 1/2] firewire: core: move workqueue handler from 1394 OHCI driver to core function

[Takashi Sakamoto]

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-takashi@sakamocchi.jp>
---
 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

[PATCH 2/2] firewire: core: use mutex to coordinate concurrent calls to flush completions

[Takashi Sakamoto]

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-takashi@sakamocchi.jp>
---
 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

Re: [PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()

[Takashi Sakamoto]

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

Re: [PATCH 0/2] firewire: core: optimize for concurrent calls of fw_iso_context_flush_completions()

[Takashi Sakamoto]

Hi,

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

I realized that the above changes have unpreferable effects to the behaviour
for user space interface. The changes allow to call the handler of
isochronous context again to drain the rest of packet buffer after calling
the handler at first due to processing the interrupt flag of 1394 OHCI IT/IR
descriptor. As a result, it is possible to enqueue two iso_interrupt events
for user space applications in the bottom half of hardIRQ. However, this is
against the description in UAPI header:

```
$ cat include/uapi/linux/firewire-cdev.h
...
 * struct fw_cdev_event_iso_interrupt - Sent when an iso packet was completed
...
 * This event is sent when the controller has completed an &fw_cdev_iso_packet
 * with the %FW_CDEV_ISO_INTERRUPT bit set, when explicitly requested with
 * %FW_CDEV_IOC_FLUSH_ISO, or when there have been so many completed packets
 * without the interrupt bit set that the kernel's internal buffer for @header
 * is about to overflow.  (In the last case, ABI versions < 5 drop header data
 * up to the next interrupt packet.)
```

As a bottom half of hardIRQ, the work item should enqueue a single event
associated to the interrupt event. The rest of packet buffer should be
handled in the bottom half of next hardIRQ unless in the path of
FW_CDEV_ISO_INTERRUPT.

Let me revert these changes later.


Regards

Takashi Sakamoto