[pulseaudio-discuss] [PATCH 2/2] memblockq-test: fix incorrect assumption of pa_memblockq_pop_missing() behaviour

[Tanu Kaskinen]

On Sat, 2016-12-31 at 18:26 +0200, Ahmed S. Darwish wrote:
> Hi!
> 
> On Fri, Dec 30, 2016 at 05:52:36PM +0200, Tanu Kaskinen wrote:
> > The intuitive meaning of "missing" would be the difference between
> > tlength and the current queue length, and that's how memblockq-test
> > assumed pa_memblockq_pop_missing() to define the term "missing", but
> > that was an incorrect assumption, causing the last
> > pa_memblockq_pop_missing() return value assertion to fail.
> > 
> > This patch fixes the failing assertion and adds some comments about how
> > the "missing" and "requested" variables in memblockq work.
> > ---
> >  src/tests/memblockq-test.c | 95 ++++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 75 insertions(+), 20 deletions(-)
> > 
> > diff --git a/src/tests/memblockq-test.c b/src/tests/memblockq-test.c
> > index fc83d99..2a9b88a 100644
> > --- a/src/tests/memblockq-test.c
> > +++ b/src/tests/memblockq-test.c
> > @@ -421,74 +421,129 @@ START_TEST (memblockq_test_pop_missing) {
> >      bq = pa_memblockq_new("test memblockq", idx, maxlength, tlength, &ss, prebuf, minreq, maxrewind, &silence);
> >      fail_unless(bq != NULL);
> >  
> > -    /* initially, the whole target length of bytes is missing */
> > +    /* The following equation regarding the internal variables of a memblockq
> > +     * is always true:
> > +     *
> > +     *   length + missing + requested = tlength
> > +     *
> > +     * "length" is the current memblockq length (write index minus read index)
> > +     * and "tlength" is the target length. The intuitive meaning of "missing"
> > +     * would be the difference between tlength and length, but actually
> > +     * "missing" and "requested" together constitute the amount that is missing
> > +     * from the queue. Writing to the queue decrements "requested" and reading
> > +     * from the queue increments "missing". pa_memblockq_pop_missing() resets
> > +     * "missing" to zero, returns the old "missing" value and adds the
> > +     * equivalent amount to "requested".
> > +     *
> > +     * This test has comments between each step documenting the assumed state
> > +     * of those internal variables. */
> > +
> 
> What really bothers me is that memblockq.requested is actually
> referenced in just _one_ place: a MEMBLOCKQ_DEBUG printf()! :-(
> 
> So memblockq.requested actually serves no value: it's just used
> for debugging statements. That makes my earlier notes of requested
> needed to be negative also wrong. And thus there's definitely
> another reason why the now-reverted 74251f0 commit triggered a
> problem when doing large 64K writes on low-latency streams..
> 
> I've verified the statement above. Removed memblockq.requested
> completely [1] and everything compiled (and worked) fine as
> expected; even in the big stream writes case.
> 
> So an ACK of course for this patch; its explanations make things
> much more comprehesible! Just a note that the case is still open,
> and we still don't know what's actually causing problems in the
> memblockq code..

Interesting. I'd say there's still some value for having the
"requested" variable, because it makes it a bit easier to explain how
the "missing" variable behaves. I'm not against removing the
"requested" variable, though.

Here's my explanation why the reverted commit caused problems: when
that commit was still in use, "requested" *did* get used. It was used
for calculating the "missing" variable. Therefore, when the "requested"
variable was calculated wrong, "missing" was calculated wrong too.

-- 
Tanu

https://www.patreon.com/tanuk