Re: [PATCH 4/6] util: introduce co-shared-amount

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9891 bytes --]

On 03.10.19 19:15, Vladimir Sementsov-Ogievskiy wrote:
> Introduce an API for some shared splitable resource, like memory.

*splittable, I suppose

> It's going to be used by backup. Backup uses both read/write io and
> copy_range. copy_range may consume memory implictly, so new API is

*the new API

> abstract: it don't allocate any real memory but only handling out

*It doesn’t allocate any real memory but only hands out

> tickets.

Note that allocation doesn’t necessarily mean you get a handle to the
allocated object.  It just means that some resource (or part of a
resource) is now under your exclusive control.  At least that’s how I
understand the term.

So this is indeed some form of allocation.

> The idea is that we have some total amount of something and callers
> should wait in coroutine queue if there is not enough of the resource
> at the moment.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  include/qemu/co-shared-amount.h | 66 ++++++++++++++++++++++++++++
>  util/qemu-co-shared-amount.c    | 77 +++++++++++++++++++++++++++++++++
>  util/Makefile.objs              |  1 +
>  3 files changed, 144 insertions(+)
>  create mode 100644 include/qemu/co-shared-amount.h
>  create mode 100644 util/qemu-co-shared-amount.c
> 
> diff --git a/include/qemu/co-shared-amount.h b/include/qemu/co-shared-amount.h
> new file mode 100644
> index 0000000000..e2dbc43dfd
> --- /dev/null
> +++ b/include/qemu/co-shared-amount.h
> @@ -0,0 +1,66 @@
> +/*
> + * Generic shared amount for coroutines

“amount” always begs the question “amount of what”.  So maybe something
more verbose like “Helper functionality for distributing a fixed total
amount of an abstract resource among multiple coroutines” would answer
that question.

(I can’t come up with a better short name than shared-amount, though.
It does get the point across once the concept’s clear.)

> + *
> + * Copyright (c) 2019 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#ifndef QEMU_CO_SHARED_AMOUNT_H
> +#define QEMU_CO_SHARED_AMOUNT_H
> +
> +
> +typedef struct QemuCoSharedAmount QemuCoSharedAmount;
> +
> +/*
> + * Create QemuCoSharedAmount structure
> + *
> + * @total: total amount to be shared between clients
> + *
> + * Note: this API is not thread-safe as it originally designed to be used in
> + * backup block-job and called from one aio context. If multiple threads support
> + * is needed it should be implemented (for ex., protect QemuCoSharedAmount by
> + * mutex).

I think the simple note “This API is not thread-safe” will suffice.  The
rest seems more confusing to me than that it really helps.

> + */
> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total);
> +
> +/*
> + * Release QemuCoSharedAmount structure

I’d add the note from qemu_co_put_amount():

“This function may only be called once everything allocated by all
clients has been deallocated/released.”

> + */
> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s);
> +
> +/*
> + * Try to get n peaces. If not enough free peaces returns false, otherwise true.

*pieces

But I’d prefer “Try to allocate an amount of @n.  Return true on
success, and false if there is too little left of the collective
resource to fulfill the request.”

> + */
> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +/*
> + * Get n peaces. If not enough yields. Return on success.

I’d prefer “Allocate an amount of $n, and, if necessary, yield until
that becomes possible.”

> + */
> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +/*
> + * Put n peaces. Client must not put more than it gets, still it may put in
> + * split: for example, get(5) and then put(3), put(2). All peaces must be put
> + * back before qemu_co_shared_amount_free call.

I’d prefer “Deallocate/Release an amount of @n.  The total amount
allocated by a caller does not need to be deallocated/released with a
single call, but may be split over several calls.  For example, get(4),
get(3), and then put(5), put(2).”

(And drop the qemu_co_shared_amount_free() reference, because it should
so say there.)

> + */
> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n);
> +
> +
> +#endif /* QEMU_CO_SHARED_AMOUNT_H */
> diff --git a/util/qemu-co-shared-amount.c b/util/qemu-co-shared-amount.c
> new file mode 100644
> index 0000000000..8855ce5705
> --- /dev/null
> +++ b/util/qemu-co-shared-amount.c
> @@ -0,0 +1,77 @@
> +/*
> + * Generic shared amount for coroutines
> + *
> + * Copyright (c) 2019 Virtuozzo International GmbH
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to deal
> + * in the Software without restriction, including without limitation the rights
> + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
> + * copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
> + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
> + * THE SOFTWARE.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qemu/coroutine.h"
> +#include "qemu/co-shared-amount.h"
> +
> +struct QemuCoSharedAmount {
> +    uint64_t total;
> +    uint64_t taken;

I’d reverse the “taken” to be “available”.  Then the only purpose of
“total” would be as part of assertions.

> +
> +    CoQueue queue;
> +};
> +
> +QemuCoSharedAmount *qemu_co_shared_amount_new(uint64_t total)
> +{
> +    QemuCoSharedAmount *s = g_new0(QemuCoSharedAmount, 1);
> +
> +    s->total = total;
> +    qemu_co_queue_init(&s->queue);
> +
> +    return s;
> +}
> +
> +void qemu_co_shared_amount_free(QemuCoSharedAmount *s)
> +{
> +    assert(s->taken == 0);
> +    g_free(s);
> +}
> +
> +bool qemu_co_try_get_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    if (n < s->total && s->total - n >= s->taken) {

(This’d become simply “s->available >= n”)

(And to be honest I have a hard time parsing that second condition.  To
me, the natural order would appear to be “s->total - s->taken >= n”.  I
mean, I can see that it matches by rearranging the inequation, but...
And in this order you could drop the “n < s->total” part because it’s
guaranteed that s->taken <= s->total.)

> +        s->taken += n;
> +        return true;
> +    }
> +
> +    return false;
> +}
> +
> +void coroutine_fn qemu_co_get_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    assert(n < s->total);
> +    while (s->total - n < s->taken) {
> +        qemu_co_queue_wait(&s->queue, NULL);
> +    }
> +
> +    assert(qemu_co_try_get_amount(s, n));

I’d refrain from putting things that should do something in assertions
because assert() is not guaranteed to be compiled.

It is in practice in qemu, but I still don’t like it too much.

Furthermore, it appears to me that the following would be simpler:

while (!qemu_co_try_get_amount(s, n)) {
    qemu_co_queue_wait(&s->queue, NULL);
}

> +}
> +
> +void coroutine_fn qemu_co_put_amount(QemuCoSharedAmount *s, uint64_t n)
> +{
> +    assert(n <= s->taken);
> +    s->taken -= n;
> +    qemu_co_queue_restart_all(&s->queue);

It itches me to ask for a better allocation strategy (like maybe
smallest-allocation-first), but I suppose I should just scratch myself.

Max

> +}
> diff --git a/util/Makefile.objs b/util/Makefile.objs
> index 41bf59d127..65ae18993a 100644
> --- a/util/Makefile.objs
> +++ b/util/Makefile.objs
> @@ -37,6 +37,7 @@ util-obj-y += rcu.o
>  util-obj-$(CONFIG_MEMBARRIER) += sys_membarrier.o
>  util-obj-y += qemu-coroutine.o qemu-coroutine-lock.o qemu-coroutine-io.o
>  util-obj-y += qemu-coroutine-sleep.o
> +util-obj-y += qemu-co-shared-amount.o
>  util-obj-y += coroutine-$(CONFIG_COROUTINE_BACKEND).o
>  util-obj-y += buffer.o
>  util-obj-y += timed-average.o
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]