[alsa-devel] Bug in alsa-lib causes high CPU with rate plugin

[Takashi Iwai]

At Fri, 9 Nov 2012 17:19:32 -0800,
Trent Piepho wrote:
> 
> On Fri, Nov 9, 2012 at 8:57 AM, Takashi Iwai <tiwai at suse.de> wrote:
> > At Thu, 8 Nov 2012 13:47:52 -0800,
> > Trent Piepho wrote:
> >> snd_pcm_wait() doesn't sleep, because it's based on the status of the
> >> hw buffer and that buffer does have free space.  snd_pcm_write_areas()
> >> would write to the hw buffer and not try to sleep.  But it's not
> >> looking at the hw buffer when it decides to sleep or write, it's
> >> looking at the rate buffer.  And the rate buffer's fill level says to
> >> sleep, not write.  And if snd_pcm_wait() was looking at the rate
> >> buffer, it would sleep.
> >>
> >
> > Yeah, this is a known longtime PITA in the rate plugin.
> >
> > Tweaking avail_min is also an option, but I'm afraid it's too
> > hackish.
> >
> > The primary problem is the discrepancy of the state "I have to wait
> > until period size becomes available" between the rate plugin and the
> > underlying PCM.  Since the poll is performed only on the slave PCM,
> > checking the rate plugin's avail is bogus in such a state.  In other
> > words, if the rate plugin puts the decision for the sleep simply
> > depending on the slave state, there will be no inconsistency between
> > them.
> >
> > Below is a quick fix.  Does it work for you?
> 
> It adds an extra call to the kernel to get the HW pointer position yet
> again.  snd_pcm_avail_update() is called initially by
> snd_pcm_write_areas.

Oops, I forgot to replace it with snd_pcm_mmap_avail(), which has no
extra update call.

>  The avail_update method for the rate plugin
> calls snd_pcm_avail_update on the slave.  It's worth noting at this
> point that we know that avail(rate) + unresampled_data = avail(slave).
>   Then check_for_wait() is called right after, which calls the rate
> plugin's check_for_wait method, which calls snd_pcm_avail_update() on
> the slave a second time.
> 
> It seems like you have a race if a period elapses between the two
> checks of the slave's avail.  Suppose upon entering
> snd_pcm_write_areas both buffers are 100% full.  avail =
> snd_pcm_update_avail(pcm /*rate*/) returns zero, which is up to date
> and correct.  We get to the code for check_for_wait() and it calls
> snd_pcm_avail_update(slave) and a period has elapsed since "avail" was
> found. So check_for_wait() returns not to wait, which is correct based
> on the current state of the slave.  Now the rest of the
> snd_pcm_write_areas() code runs, but avail=0 still and it's not going
> to work, it just blows up if avail = 0.
> 
> If you think about it, tweaking avail_min does the same thing as using
> the slave's avail and avail_min.
> 
> We had a wait check of "rate->avail < rate->avail_min".  What we want
> is "slave->avail < slave->avail_min".  Ignoring the resampling factor,
> we know that:
> rate->avail + rate->unresampled_data = slave->avail
> rate->avail_min = slave->avail_min
> 
> So if we do a little algebra:
> slave->avail < slave->avail_min
> rate->avail + rate->unresampled_data < slave->avail_min
> rate->avail + rate->unresampled_data < rate->avail_min
> rate->avail < rate->avail_min - rate->unresampled_data
> 
> In other words, subtracting the unresampled data from the avail_min
> for the rate pcm produces the exact same comparison as looking at the
> slave's avail and avail_min, which is what we actually wanted.

Well, thinking more on this, I don't think fiddling avail_min always
works.

First off, avail_min is a value set by the user and can be referred by
the outside during operation (until user again changes via sw_params
call).  Changing this internally may cause an unexpected side effect
in other codes -- remember that there can be an external plugin too.
(We do some ugly avail_min manipulations in the rate drain callback,
 but it's closed only in the function, thus the impact must be
 minimum.)

Secondly, the avail_min modification doesn't work if another plugin is
deployed over the rate plugin.  For example, using PCM foo below with
S8 format in your test program will again hog CPU.

pcm.foo {
    type linear
    slave.format S8
    slave.pcm {
	    type plug;
	    slave {
	        pcm "hw:0,0";
	        rate 48000;
	    }
    }
}

It's because the inconsistent state of the rate plugin isn't
considered in the upper layer. Obviously, for fixing this, we need to
propagate the workaround for the avail_min check.

My previous patch doesn't work for this case, too, but it's easy to
make it working.  A revised patch is attached below.  It still doesn't
cover all plugins (e.g. multi plugin), but for testing it should
suffice.


thanks,

Takashi

---
diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
index 5880057..393c357 100644
--- a/src/pcm/pcm.c
+++ b/src/pcm/pcm.c
@@ -2352,7 +2352,7 @@ int snd_pcm_open_named_slave(snd_pcm_t **pcmp, const char *name,
  */
 int snd_pcm_wait(snd_pcm_t *pcm, int timeout)
 {
-	if (snd_pcm_mmap_avail(pcm) >= pcm->avail_min) {
+	if (snd_pcm_may_wait_for_avail_min(pcm, snd_pcm_mmap_avail(pcm))) {
 		/* check more precisely */
 		switch (snd_pcm_state(pcm)) {
 		case SND_PCM_STATE_XRUN:
@@ -6771,14 +6771,14 @@ snd_pcm_sframes_t snd_pcm_write_areas(snd_pcm_t *pcm, const snd_pcm_channel_area
 			goto _end;
 		}
 		if ((state == SND_PCM_STATE_RUNNING &&
-		     (snd_pcm_uframes_t)avail < pcm->avail_min &&
-		     size > (snd_pcm_uframes_t)avail)) {
+		     size > (snd_pcm_uframes_t)avail &&
+		     snd_pcm_may_wait_for_avail_min(pcm, avail))) {
 			if (pcm->mode & SND_PCM_NONBLOCK) {
 				err = -EAGAIN;
 				goto _end;
 			}
 
-			err = snd_pcm_wait(pcm, -1);
+			err = snd_pcm_wait_nocheck(pcm, -1);
 			if (err < 0)
 				break;
 			goto _again;			
diff --git a/src/pcm/pcm_generic.c b/src/pcm/pcm_generic.c
index 0436439..6b26822 100644
--- a/src/pcm/pcm_generic.c
+++ b/src/pcm/pcm_generic.c
@@ -341,4 +341,10 @@ int snd_pcm_generic_set_chmap(snd_pcm_t *pcm, const snd_pcm_chmap_t *map)
 	return snd_pcm_set_chmap(generic->slave, map);
 }
 
+int snd_pcm_generic_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail)
+{
+	snd_pcm_generic_t *generic = pcm->private_data;
+	return snd_pcm_may_wait_for_avail_min(generic->slave, snd_pcm_mmap_avail(generic->slave));
+}
+
 #endif /* DOC_HIDDEN */
diff --git a/src/pcm/pcm_generic.h b/src/pcm/pcm_generic.h
index 916c6ec..f06f35f 100644
--- a/src/pcm/pcm_generic.h
+++ b/src/pcm/pcm_generic.h
@@ -109,6 +109,8 @@ typedef struct {
 	snd1_pcm_generic_get_chmap
 #define snd_pcm_generic_set_chmap \
 	snd1_pcm_generic_set_chmap
+#define snd_pcm_generic_may_wait_for_avail_min \
+	snd1_pcm_generic_may_wait_for_avail_min
 
 int snd_pcm_generic_close(snd_pcm_t *pcm);
 int snd_pcm_generic_nonblock(snd_pcm_t *pcm, int nonblock);
@@ -158,5 +160,5 @@ int snd_pcm_generic_munmap(snd_pcm_t *pcm);
 snd_pcm_chmap_query_t **snd_pcm_generic_query_chmaps(snd_pcm_t *pcm);
 snd_pcm_chmap_t *snd_pcm_generic_get_chmap(snd_pcm_t *pcm);
 int snd_pcm_generic_set_chmap(snd_pcm_t *pcm, const snd_pcm_chmap_t *map);
-
+int snd_pcm_generic_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail);
 
diff --git a/src/pcm/pcm_hooks.c b/src/pcm/pcm_hooks.c
index 0feb4a3..8ea4d2a 100644
--- a/src/pcm/pcm_hooks.c
+++ b/src/pcm/pcm_hooks.c
@@ -199,6 +199,7 @@ static const snd_pcm_fast_ops_t snd_pcm_hooks_fast_ops = {
 	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
 	.poll_descriptors = snd_pcm_generic_poll_descriptors,
 	.poll_revents = snd_pcm_generic_poll_revents,
+	.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,
 };
 
 /**
diff --git a/src/pcm/pcm_local.h b/src/pcm/pcm_local.h
index e7798fd..830035e 100644
--- a/src/pcm/pcm_local.h
+++ b/src/pcm/pcm_local.h
@@ -177,6 +177,7 @@ typedef struct {
 	int (*poll_descriptors_count)(snd_pcm_t *pcm);
 	int (*poll_descriptors)(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int space);
 	int (*poll_revents)(snd_pcm_t *pcm, struct pollfd *pfds, unsigned int nfds, unsigned short *revents);
+	int (*may_wait_for_avail_min)(snd_pcm_t *pcm, snd_pcm_uframes_t avail);
 } snd_pcm_fast_ops_t;
 
 struct _snd_pcm {
@@ -984,3 +985,12 @@ _snd_pcm_parse_config_chmaps(snd_config_t *conf);
 snd_pcm_chmap_t *
 _snd_pcm_choose_fixed_chmap(snd_pcm_t *pcm, snd_pcm_chmap_query_t * const *maps);
 
+/* return true if the PCM stream may wait to get avail_min space */
+static inline int snd_pcm_may_wait_for_avail_min(snd_pcm_t *pcm, snd_pcm_uframes_t avail)
+{
+	if (pcm->fast_ops->may_wait_for_avail_min)
+		return pcm->fast_ops->may_wait_for_avail_min(pcm, avail);
+	else
+		return avail < pcm->avail_min;
+}
+
diff --git a/src/pcm/pcm_plugin.c b/src/pcm/pcm_plugin.c
index d88e117..f228f3b 100644
--- a/src/pcm/pcm_plugin.c
+++ b/src/pcm/pcm_plugin.c
@@ -570,6 +570,7 @@ const snd_pcm_fast_ops_t snd_pcm_plugin_fast_ops = {
 	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
 	.poll_descriptors = snd_pcm_generic_poll_descriptors,
 	.poll_revents = snd_pcm_generic_poll_revents,
+	.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,
 };
 
 #endif
diff --git a/src/pcm/pcm_rate.c b/src/pcm/pcm_rate.c
index 4ba8521..f0bf218 100644
--- a/src/pcm/pcm_rate.c
+++ b/src/pcm/pcm_rate.c
@@ -1234,6 +1234,7 @@ static const snd_pcm_fast_ops_t snd_pcm_rate_fast_ops = {
 	.poll_descriptors_count = snd_pcm_generic_poll_descriptors_count,
 	.poll_descriptors = snd_pcm_generic_poll_descriptors,
 	.poll_revents = snd_pcm_rate_poll_revents,
+	.may_wait_for_avail_min = snd_pcm_generic_may_wait_for_avail_min,
 };
 
 static const snd_pcm_ops_t snd_pcm_rate_ops = {