| Message ID | 1462625196-3640-1-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry for the delay. Takashi Sakamoto wrote: > These three commits were merged to improve PCM pointer granularity. > commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") > commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") > commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing") > > The point of them is to handle queued packets not only in software IRQ > context of IR/IT contexts, but also in process context. This idea > introduced a cyclic call of 'struct snd_pcm_ops.pointer()'. There is no recursion because of the tasklet. The only problem is that the tasklet could be scheduled repeatedly because new packets continue to arrive. But even when this happens, it's harmless. > The last commit adds 'pointer_flush' member to 'struct amdtp_stream' > to avoid the situation. On the other hand, This solution is weak at > race condition between the process context and the software IRQ > context of IR/IT contexts. > > Practically, this race is not so critical because it influences process > context to skip flushing queued packets and to get worse granularity of > PCM pointer. When a race causes pointer_flush to be read as true although it should be false, there is simply a superfluous call to flush_completions. When pointer_flush is read as false although it should be true, then the buffer_pointer value _might_ not be the most current one (due to the missing flush), but this is unlikely to happen because the buffer_pointer was just updated by update_pcm_pointers(). In any case, the just- scheduled tasklet will inform the application about the new pointer. > The similar solution can be achieved by 'in_interrupt()' macro. This > commit applies the macro to solve the race condition against > 'pointer_flush'. > unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) > { > - /* this optimization is allowed to be racy */ > - if (s->pointer_flush && amdtp_stream_running(s)) > ... > + if (!in_interrupt() && amdtp_stream_running(s)) > fw_iso_context_flush_completions(s->context); > - else > - s->pointer_flush = true; > > return ACCESS_ONCE(s->pcm_buffer_pointer); > } Looks good. Acked-by: Clemens Ladisch <clemens@ladisch.de> > And I think there's another race condition against processing each packets > by calling out/in_stream_callback(), but I cannot observe the race. As you already found out, fw_iso_context_flush_completions() is thread safe. Regards, Clemens
Hi Clemens, On May 11 2016 16:31, Clemens Ladisch wrote: > Sorry for the delay. > > Takashi Sakamoto wrote: >> These three commits were merged to improve PCM pointer granularity. >> commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") >> commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") >> commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing") >> >> The point of them is to handle queued packets not only in software IRQ >> context of IR/IT contexts, but also in process context. This idea >> introduced a cyclic call of 'struct snd_pcm_ops.pointer()'. > > There is no recursion because of the tasklet. The only problem is that > the tasklet could be scheduled repeatedly because new packets continue > to arrive. But even when this happens, it's harmless. > >> The last commit adds 'pointer_flush' member to 'struct amdtp_stream' >> to avoid the situation. On the other hand, This solution is weak at >> race condition between the process context and the software IRQ >> context of IR/IT contexts. >> >> Practically, this race is not so critical because it influences process >> context to skip flushing queued packets and to get worse granularity of >> PCM pointer. > > When a race causes pointer_flush to be read as true although it should > be false, there is simply a superfluous call to flush_completions. > > When pointer_flush is read as false although it should be true, then the > buffer_pointer value _might_ not be the most current one (due to the > missing flush), but this is unlikely to happen because the buffer_pointer > was just updated by update_pcm_pointers(). In any case, the just- > scheduled tasklet will inform the application about the new pointer. > >> The similar solution can be achieved by 'in_interrupt()' macro. This >> commit applies the macro to solve the race condition against >> 'pointer_flush'. > >> unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) >> { >> - /* this optimization is allowed to be racy */ >> - if (s->pointer_flush && amdtp_stream_running(s)) >> ... >> + if (!in_interrupt() && amdtp_stream_running(s)) >> fw_iso_context_flush_completions(s->context); >> - else >> - s->pointer_flush = true; >> >> return ACCESS_ONCE(s->pcm_buffer_pointer); >> } > > Looks good. > > Acked-by: Clemens Ladisch <clemens@ladisch.de> OK. Thanks for reviewing. I've posted a patch with updated comments and the tag. >> And I think there's another race condition against processing each packets >> by calling out/in_stream_callback(), but I cannot observe the race. > > As you already found out, fw_iso_context_flush_completions() is thread > safe. Regards Takashi Sakamoto
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c index 4484242..132c7f4 100644 --- a/sound/firewire/amdtp-stream.c +++ b/sound/firewire/amdtp-stream.c @@ -248,7 +248,6 @@ void amdtp_stream_pcm_prepare(struct amdtp_stream *s) tasklet_kill(&s->period_tasklet); s->pcm_buffer_pointer = 0; s->pcm_period_pointer = 0; - s->pointer_flush = true; } EXPORT_SYMBOL(amdtp_stream_pcm_prepare); @@ -353,7 +352,6 @@ static void update_pcm_pointers(struct amdtp_stream *s, s->pcm_period_pointer += frames; if (s->pcm_period_pointer >= pcm->runtime->period_size) { s->pcm_period_pointer -= pcm->runtime->period_size; - s->pointer_flush = false; tasklet_hi_schedule(&s->period_tasklet); } } @@ -798,11 +796,24 @@ EXPORT_SYMBOL(amdtp_stream_start); */ unsigned long amdtp_stream_pcm_pointer(struct amdtp_stream *s) { - /* this optimization is allowed to be racy */ - if (s->pointer_flush && amdtp_stream_running(s)) + /* + * This function is called in software IRQ context of period_tasklet or + * process context. + * + * When the software IRQ context was scheduled by software IRQ context + * of IR/IT contexts, queued packets were already handled. Therefore, + * no need to flush the queue in buffer anymore. + * + * When the process context reach here, some packets wll be queued in + * the buffer. These packets should be handled immediately to keep + * better granularity of PCM pointer. + * + * Later, the process context will sometimes schedules software IRQ + * context of the period_tasklet. Then, no need to flush the queue by + * the same reason as described for IR/IT contexts. + */ + if (!in_interrupt() && amdtp_stream_running(s)) fw_iso_context_flush_completions(s->context); - else - s->pointer_flush = true; return ACCESS_ONCE(s->pcm_buffer_pointer); } diff --git a/sound/firewire/amdtp-stream.h b/sound/firewire/amdtp-stream.h index 8775704..ab697cc 100644 --- a/sound/firewire/amdtp-stream.h +++ b/sound/firewire/amdtp-stream.h @@ -132,7 +132,6 @@ struct amdtp_stream { struct tasklet_struct period_tasklet; unsigned int pcm_buffer_pointer; unsigned int pcm_period_pointer; - bool pointer_flush; /* To wait for first packet. */ bool callbacked;
Hi Clemens, Could I request your comment to this patch? It is to solve a race condition, but the condition is quite rare. Practically, it might have a less meanings, except for better program. And I think there's another race condition against processing each packets by calling out/in_stream_callback(), but I cannot observe the race. Software IRQ contexts of IR/IT contexts and process contexts are under the race condition, however I can see no problems related to it in my several trials in multi-core machine. I have no idea about the reason that packet sequence is processed correctly between the software IRQ contexts and the process contexts without any lock primitives. Do you have some ideas about it? Regards Takashi Sakamoto ----- 8< ----- Subject: ALSA: firewire-lib: permit process context only to flush queued packets for better PCM period granularity These three commits were merged to improve PCM pointer granularity. commit 76fb87894828 ("ALSA: firewire-lib: taskletize the snd_pcm_period_elapsed() call") commit e9148dddc3c7 ("ALSA: firewire-lib: flush completed packets when reading PCM position") commit 92b862c7d685 ("ALSA: firewire-lib: optimize packet flushing") The point of them is to handle queued packets not only in software IRQ context of IR/IT contexts, but also in process context. This idea introduced a cyclic call of 'struct snd_pcm_ops.pointer()'. The last commit adds 'pointer_flush' member to 'struct amdtp_stream' to avoid the situation. On the other hand, This solution is weak at race condition between the process context and the software IRQ context of IR/IT contexts. Practically, this race is not so critical because it influences process context to skip flushing queued packets and to get worse granularity of PCM pointer. This is quite rare because 'struct snd_pcm_ops.pointer()' is frequently called by process contexts while IT/IR contexts run less frequently. However, it's better to solve the race. The similar solution can be achieved by 'in_interrupt()' macro. This commit applies the macro to solve the race condition against 'pointer_flush'. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/firewire/amdtp-stream.c | 23 +++++++++++++++++------ sound/firewire/amdtp-stream.h | 1 - 2 files changed, 17 insertions(+), 7 deletions(-)