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
