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

Sun Jan 1 07:02:04 UTC 2017

On Sun, Jan 01, 2017 at 04:17:16AM +0200, Tanu Kaskinen wrote:
> 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.
>

Yeah, it makes the equation more explicit. Meanwhile it also gives
the illusion that we actually do something upon memblockq write
index change.

We actually do nothing, and don't update any state (!), upon _most_
of the memblockq writes (since most of them set accounting to
true).  Right now in cgit origin/master, it all boils down to this:

    static void write_index_changed(pa_memblockq *bq,
                                    int64_t old_write_index,
                                    bool account) {

        if (!account)
            bq->missing -= (bq->write_index - old_write_index);
    }

which makes me don't understand what's actually going on. Anyway,
I've done some git blaming, and found that this was not always the
case. Rather, memblockq state used to change a bit upon writes.

What introduced that behavior was a commit by Lennart in 2010,
during the final 0.9.22 release before moving to systemd. [*]
Here's the commit; interestingly it directly mentions our case of
writing to the stream more than what is originally requested:

    commit 699233fb47d133f6ea1e36e8354a386c23608d5a
    Author: Lennart Poettering <lennart at poettering.net>
    Date:   Fri Jan 8 20:07:34 2010 +0100

    native: fix request counter miscalculations

    Do not subtract bytes the client sends us beyond what we
    requested from our missing bytes counter.

    This was mostly a thinko that caused servers asking for too
    little data when the client initially sent more data than
    requested, because that data sent too much was accounted for
    twice.

    This commit fixes this miscalculation.

    http://bugzilla.redhat.com/show_bug.cgi?id=534130

I'll leave it at that for today since I'll have to travel tomorrow,
in a long 26 hours trip, to CES. I guess we can release pulse v10.0
now, and then can discuss this further. What needs to be done can
be summarized to:

   - Add a comment stating that memblockq.requested is only used
     for nicer debugging statements, and does not affect the
     memblockq actual state.

   - Moving the nice explanation in this patch series to
     memblockq.c itself

   - Adding more comments by Pierre; the "not understandable by
     mere mortals" part

   - Referencing some of the messages in Lennart's commit above;
     after understanding the current issue further, etc.

> 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.
>

Yeah, indeed. Looked only at the current master, forgetting that
the revert caused a considerable change in the code.

Thanks a lot for tackling this btw; the new test cases are also
really nice ;-)

regards,

[*] https://lists.freedesktop.org/archives/pulseaudio-discuss/2010-November/008111.html

-- 
Darwish
http://darwish.chasingpointers.com