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..
[1]
Remove memblockq.requested; it's only used for debugging statements
and makes an already confusing code harder to deduce.
(I'll send an official submission for this soon)
---
diff --git a/src/pulsecore/memblockq.c b/src/pulsecore/memblockq.c
index f660ffaed..145b7a2e9 100644
--- a/src/pulsecore/memblockq.c
+++ b/src/pulsecore/memblockq.c
@@ -53,7 +53,7 @@ struct pa_memblockq {
bool in_prebuf;
pa_memchunk silence;
pa_mcalign *mcalign;
- int64_t missing, requested;
+ int64_t missing;
char *name;
pa_sample_spec sample_spec;
};
@@ -251,13 +251,11 @@ static void write_index_changed(pa_memblockq *bq, int64_t old_write_index, bool
delta = bq->write_index - old_write_index;
- if (account)
- bq->requested -= delta;
- else
+ if (!account)
bq->missing -= delta;
#ifdef MEMBLOCKQ_DEBUG
- pa_log_debug("[%s] pushed/seeked %lli: requested counter at %lli, account=%i", bq->name, (long long) delta, (long long) bq->requested, account);
+ pa_log_debug("[%s] pushed/seeked %lli", bq->name, (long long) delta);
#endif
}
@@ -846,7 +844,6 @@ size_t pa_memblockq_pop_missing(pa_memblockq *bq) {
l = (size_t) bq->missing;
- bq->requested += bq->missing;
bq->missing = 0;
#ifdef MEMBLOCKQ_DEBUG
--
Darwish
http://darwish.chasingpointers.com
