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

[Tanu Kaskinen]

On Sun, 2017-01-01 at 09:02 +0200, Ahmed S. Darwish wrote:
> 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).

We update the write index, isn't that a rather important state update?

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

I feel I have a pretty good grasp of what's going on, but I'm not sure
what's unclear to you. Is it just that you thought that writes really
don't change any memblockq state except the unimportant "requested"
variable?

-- 
Tanu

https://www.patreon.com/tanuk