| Message ID | 20230713064111.558652-4-pizhenwei@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc fixes for throttle | expand |
On 13.07.23 08:41, zhenwei pi wrote: > Only one direction is necessary in several scenarios: > - a read-only disk > - operations on a device are considered as *write* only. For example, > encrypt/decrypt/sign/verify operations on a cryptodev use a single > *write* timer(read timer callback is defined, but never invoked). > > Allow a single direction in throttle, this reduces memory, and uplayer > does not need a dummy callback any more. > > Reviewed-by: Alberto Garcia<berto@igalia.com> > Signed-off-by: zhenwei pi<pizhenwei@bytedance.com> > --- > util/throttle.c | 32 ++++++++++++++++++++++---------- > 1 file changed, 22 insertions(+), 10 deletions(-) > > diff --git a/util/throttle.c b/util/throttle.c > index 5642e61763..c0bd0c26c3 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -199,12 +199,17 @@ static bool throttle_compute_timer(ThrottleState *ts, > void throttle_timers_attach_aio_context(ThrottleTimers *tt, > AioContext *new_context) > { > - tt->timers[THROTTLE_READ] = > - aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->timer_cb[THROTTLE_READ], tt->timer_opaque); > - tt->timers[THROTTLE_WRITE] = > - aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque); > + if (tt->timer_cb[THROTTLE_READ]) { > + tt->timers[THROTTLE_READ] = > + aio_timer_new(new_context, tt->clock_type, SCALE_NS, > + tt->timer_cb[THROTTLE_READ], tt->timer_opaque); > + } > + > + if (tt->timer_cb[THROTTLE_WRITE]) { > + tt->timers[THROTTLE_WRITE] = > + aio_timer_new(new_context, tt->clock_type, SCALE_NS, > + tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque); > + } I think a `for (int i = 0; i < THROTTLE_MAX; i++)` loop would make this nicer. (Again: Optional.) > } > > /* [...] > @@ -272,7 +280,7 @@ void throttle_timers_destroy(ThrottleTimers *tt) > /* is any throttling timer configured */ > bool throttle_timers_are_initialized(ThrottleTimers *tt) > { > - if (tt->timers[0]) { > + if (tt->timers[THROTTLE_READ] || tt->timers[THROTTLE_WRITE]) { Not wrong, but I’d prefer the more general ``` for (int i = 0; i < THROTTLE_MAX; i++) { if (tt->timers[i]) { return true; } } return false; ``` > return true; > } > > @@ -424,8 +432,12 @@ bool throttle_schedule_timer(ThrottleState *ts, > { > int64_t now = qemu_clock_get_ns(tt->clock_type); > int64_t next_timestamp; > + QEMUTimer *timer; > bool must_wait; > > + timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ]; Could be shorter as `timer = tt->timers[is_write ? THROTTLE_WRITE : THROTTLE_READ];`. Anyway, I only have style suggestion to offer, so either way: Reviewed-by: Hanna Czenczek <hreitz@redhat.com> > + assert(timer); > + > must_wait = throttle_compute_timer(ts, > is_write, > now,
diff --git a/util/throttle.c b/util/throttle.c index 5642e61763..c0bd0c26c3 100644 --- a/util/throttle.c +++ b/util/throttle.c @@ -199,12 +199,17 @@ static bool throttle_compute_timer(ThrottleState *ts, void throttle_timers_attach_aio_context(ThrottleTimers *tt, AioContext *new_context) { - tt->timers[THROTTLE_READ] = - aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->timer_cb[THROTTLE_READ], tt->timer_opaque); - tt->timers[THROTTLE_WRITE] = - aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque); + if (tt->timer_cb[THROTTLE_READ]) { + tt->timers[THROTTLE_READ] = + aio_timer_new(new_context, tt->clock_type, SCALE_NS, + tt->timer_cb[THROTTLE_READ], tt->timer_opaque); + } + + if (tt->timer_cb[THROTTLE_WRITE]) { + tt->timers[THROTTLE_WRITE] = + aio_timer_new(new_context, tt->clock_type, SCALE_NS, + tt->timer_cb[THROTTLE_WRITE], tt->timer_opaque); + } } /* @@ -235,6 +240,7 @@ void throttle_timers_init(ThrottleTimers *tt, QEMUTimerCB *write_timer_cb, void *timer_opaque) { + assert(read_timer_cb || write_timer_cb); memset(tt, 0, sizeof(ThrottleTimers)); tt->clock_type = clock_type; @@ -247,7 +253,9 @@ void throttle_timers_init(ThrottleTimers *tt, /* destroy a timer */ static void throttle_timer_destroy(QEMUTimer **timer) { - assert(*timer != NULL); + if (*timer == NULL) { + return; + } timer_free(*timer); *timer = NULL; @@ -272,7 +280,7 @@ void throttle_timers_destroy(ThrottleTimers *tt) /* is any throttling timer configured */ bool throttle_timers_are_initialized(ThrottleTimers *tt) { - if (tt->timers[0]) { + if (tt->timers[THROTTLE_READ] || tt->timers[THROTTLE_WRITE]) { return true; } @@ -424,8 +432,12 @@ bool throttle_schedule_timer(ThrottleState *ts, { int64_t now = qemu_clock_get_ns(tt->clock_type); int64_t next_timestamp; + QEMUTimer *timer; bool must_wait; + timer = is_write ? tt->timers[THROTTLE_WRITE] : tt->timers[THROTTLE_READ]; + assert(timer); + must_wait = throttle_compute_timer(ts, is_write, now, @@ -437,12 +449,12 @@ bool throttle_schedule_timer(ThrottleState *ts, } /* request throttled and timer pending -> do nothing */ - if (timer_pending(tt->timers[is_write])) { + if (timer_pending(timer)) { return true; } /* request throttled and timer not pending -> arm timer */ - timer_mod(tt->timers[is_write], next_timestamp); + timer_mod(timer, next_timestamp); return true; }