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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

07.10.2019 18:22, Max Reitz wrote:
> 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);
> }

A lot more nicer, thanks. Thanks for your suggestions, I'll take them!

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

And, then it would be named and customizable "--object allocator,..." to
be used for different things like jobs or block drivers.. But I'd postpone
it for future, my goal is async requests in backup and I'm moving towards it
since 2016 (we have downstream version of it since 2016) :(.. And now, with
separated block-copy and aio-task API, I feel really close.

>> +}
>> 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
>>
> 
> 


-- 
Best regards,
Vladimir