[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 4/6] util: introduce co-shared-amount
From: |
Max Reitz |
Subject: |
Re: [PATCH 4/6] util: introduce co-shared-amount |
Date: |
Mon, 7 Oct 2019 17:22:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.0 |
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 <address@hidden>
> ---
> 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
>
signature.asc
Description: OpenPGP digital signature
- [PATCH 0/6] block-copy: memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 6/6] block/block-copy: increase buffered copy request, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 4/6] util: introduce co-shared-amount, Vladimir Sementsov-Ogievskiy, 2019/10/03
- Re: [PATCH 4/6] util: introduce co-shared-amount,
Max Reitz <=
- [PATCH 2/6] block/block-copy: limit copy_range_size to 16 MiB, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 3/6] block/block-copy: refactor copying, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 1/6] block/block-copy: allocate buffer in block_copy_with_bounce_buffer, Vladimir Sementsov-Ogievskiy, 2019/10/03
- [PATCH 5/6] block/block-copy: add memory limit, Vladimir Sementsov-Ogievskiy, 2019/10/03