[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 2/3] Introduce attributes to qemu timer subsyste
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-block] [PATCH 2/3] Introduce attributes to qemu timer subsystem |
Date: |
Mon, 15 Oct 2018 10:39:12 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 |
On 14/10/2018 16:55, Artem Pisarenko wrote:
> Attributes are simple flags, associated with individual timers for their
> whole lifetime.
> They intended to be used to mark individual timers for special handling by
> various qemu features operating at qemu core level.
> Existing timer, aio and coroutine interface extended with attribute-enabled
> variants of functions, which create/initialize timers.
>
> Signed-off-by: Artem Pisarenko <address@hidden>
> ---
>
> Notes:
> Conversion and association between QEMUTimerAttrBit and accessor macro
> are dumb.
> Maybe better alternatives exist (like QFlags in Qt framework) or existing
> qemu code may be reused, if any.
> Attributes also may be better named as flags, but they looks like
> something volatile, whereas 'attribute' expresses constant nature better.
>
> include/block/aio.h | 50 +++++++++++++++++++-
> include/qemu/coroutine.h | 5 +-
> include/qemu/timer.h | 110
> ++++++++++++++++++++++++++++++++++++++------
> tests/ptimer-test-stubs.c | 7 +--
> util/qemu-coroutine-sleep.c | 6 ++-
> util/qemu-timer.c | 12 +++--
> 6 files changed, 165 insertions(+), 25 deletions(-)
>
> diff --git a/include/block/aio.h b/include/block/aio.h
> index f08630c..a6be3fb 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -407,10 +407,35 @@ static inline QEMUTimer *aio_timer_new(AioContext *ctx,
> QEMUClockType type,
> int scale,
> QEMUTimerCB *cb, void *opaque)
> {
> - return timer_new_tl(ctx->tlg.tl[type], scale, cb, opaque);
> + return timer_new_a_tl(ctx->tlg.tl[type], scale, 0, cb, opaque);
> }
The new function _a_tl is a bit ugly.
I would prefer to:
- only have a new timer_new_full that takes all of scale, attribute and
timerlist
- accept a NULL timerlist and default to main_loop_tlg.tl[type]
- add aio_timer_{new,init}_with_attrs
> diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
> index 9801e7f..cffc2b2 100644
> --- a/include/qemu/coroutine.h
> +++ b/include/qemu/coroutine.h
> @@ -276,7 +276,10 @@ void qemu_co_rwlock_unlock(CoRwlock *lock);
> /**
> * Yield the coroutine for a given duration
> */
> -void coroutine_fn qemu_co_sleep_ns(QEMUClockType type, int64_t ns);
> +#define qemu_co_sleep_ns(type, ns) \
> + qemu_co_sleep_a_ns(type, 0, ns)
> +void coroutine_fn qemu_co_sleep_a_ns(QEMUClockType type, int attributes,
> + int64_t ns);
I wouldn't bother adding co_sleep_a_ns unless it's needed.
> /**
> * Yield until a file descriptor becomes readable
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 39ea907..031e3a1 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -52,6 +52,28 @@ typedef enum {
> QEMU_CLOCK_MAX
> } QEMUClockType;
>
> +/**
> + * QEMU Timer attributes:
> + *
> + * An individual timer may be assigned with one or multiple attributes when
> + * initialized.
> + * Attribute is a static flag, meaning that timer has corresponding property.
> + * Attributes are defined in QEMUTimerAttrBit enum and encoded to bit set,
> + * which used to initialize timer, stored to 'attributes' member and can be
> + * retrieved externally with timer_get_attributes() call.
> + * Values of QEMUTimerAttrBit aren't used directly,
> + * instead each attribute in bit set accessed with QEMU_TIMER_ATTR(id) macro,
> + * where 'id' is a unique part of attribute identifier.
I think QEMU_TIMER_ATTR is an unnecessary complication. I would just
add a TIMER_ATTR_EXTERNAL constant.
Paolo