| Message ID | 20230625085631.372238-4-pizhenwei@bytedance.com |
|---|---|
| State | New |
| Headers | show |
| Series | Misc fixes for throttle | expand |
On Sun 25 Jun 2023 04:56:29 PM +08, zhenwei pi wrote: > void throttle_timers_attach_aio_context(ThrottleTimers *tt, > AioContext *new_context) > { > - tt->timers[THROTTLE_TIMER_READ] = > - aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); > - tt->timers[THROTTLE_TIMER_WRITE] = > - aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque); > + if (tt->timer_cb[THROTTLE_TIMER_READ]) { > + tt->timers[THROTTLE_TIMER_READ] = > + aio_timer_new(new_context, tt->clock_type, SCALE_NS, > + tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); > + } > + > + if (tt->timer_cb[THROTTLE_TIMER_WRITE]) { > + tt->timers[THROTTLE_TIMER_WRITE] = > + aio_timer_new(new_context, tt->clock_type, SCALE_NS, > + tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque); > + } > } If now any of the timers can be NULL, don't you want to add additional checks / assertions to (at least) throttle_schedule_timer() ? Berto
On 6/27/23 05:38, Alberto Garcia wrote: > On Sun 25 Jun 2023 04:56:29 PM +08, zhenwei pi wrote: >> void throttle_timers_attach_aio_context(ThrottleTimers *tt, >> AioContext *new_context) >> { >> - tt->timers[THROTTLE_TIMER_READ] = >> - aio_timer_new(new_context, tt->clock_type, SCALE_NS, >> - tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); >> - tt->timers[THROTTLE_TIMER_WRITE] = >> - aio_timer_new(new_context, tt->clock_type, SCALE_NS, >> - tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque); >> + if (tt->timer_cb[THROTTLE_TIMER_READ]) { >> + tt->timers[THROTTLE_TIMER_READ] = >> + aio_timer_new(new_context, tt->clock_type, SCALE_NS, >> + tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); >> + } >> + >> + if (tt->timer_cb[THROTTLE_TIMER_WRITE]) { >> + tt->timers[THROTTLE_TIMER_WRITE] = >> + aio_timer_new(new_context, tt->clock_type, SCALE_NS, >> + tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque); >> + } >> } > > If now any of the timers can be NULL, don't you want to add additional > checks / assertions to (at least) throttle_schedule_timer() ? > > Berto OK.
diff --git a/util/throttle.c b/util/throttle.c index c1cc24831c..01faee783c 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_TIMER_READ] = - aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); - tt->timers[THROTTLE_TIMER_WRITE] = - aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->timer_cb[THROTTLE_TIMER_WRITE], tt->timer_opaque); + if (tt->timer_cb[THROTTLE_TIMER_READ]) { + tt->timers[THROTTLE_TIMER_READ] = + aio_timer_new(new_context, tt->clock_type, SCALE_NS, + tt->timer_cb[THROTTLE_TIMER_READ], tt->timer_opaque); + } + + if (tt->timer_cb[THROTTLE_TIMER_WRITE]) { + tt->timers[THROTTLE_TIMER_WRITE] = + aio_timer_new(new_context, tt->clock_type, SCALE_NS, + tt->timer_cb[THROTTLE_TIMER_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_TIMER_READ] || tt->timers[THROTTLE_TIMER_WRITE]) { return true; }
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. Signed-off-by: zhenwei pi <pizhenwei@bytedance.com> --- util/throttle.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)