| Message ID | 20230713064111.558652-2-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: > Use enum ThrottleType instead of number index. > > Reviewed-by: Alberto Garcia<berto@igalia.com> > Signed-off-by: zhenwei pi<pizhenwei@bytedance.com> > --- > include/qemu/throttle.h | 11 ++++++++--- > util/throttle.c | 16 +++++++++------- > 2 files changed, 17 insertions(+), 10 deletions(-) [...] > diff --git a/util/throttle.c b/util/throttle.c > index 81f247a8d1..5642e61763 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts, > void throttle_timers_attach_aio_context(ThrottleTimers *tt, > AioContext *new_context) > { > - tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->read_timer_cb, tt->timer_opaque); > - tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS, > - tt->write_timer_cb, tt->timer_opaque); > + 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); This could benefit from the new structure: ``` for (int i = 0; i < THROTTLE_MAX; i++) { tt->timers[i] = aio_timer_new(..., tt->timer_cb[i], ...); } ``` Even more so after patch 3. Still, optional, of course, so either way: Reviewed-by: Hanna Czenczek <hreitz@redhat.com> Of note is that we still have instances in util/throttle.c and block/throttle-groups.c that don’t use these enums to access the tt->timers[] array, and that’s a bit unfortunate now (i.e. `tt->timers[is_write]` should rather be `tt->timers[is_write ? THROTTLE_WRITE : THROTTLE_READ]` instead). But functionally it’s OK and I believe patches 3 and 6 do address this.
diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h index 05f6346137..ba6293eeef 100644 --- a/include/qemu/throttle.h +++ b/include/qemu/throttle.h @@ -99,13 +99,18 @@ typedef struct ThrottleState { int64_t previous_leak; /* timestamp of the last leak done */ } ThrottleState; +typedef enum { + THROTTLE_READ = 0, + THROTTLE_WRITE, + THROTTLE_MAX +} ThrottleType; + typedef struct ThrottleTimers { - QEMUTimer *timers[2]; /* timers used to do the throttling */ + QEMUTimer *timers[THROTTLE_MAX]; /* timers used to do the throttling */ QEMUClockType clock_type; /* the clock used */ /* Callbacks */ - QEMUTimerCB *read_timer_cb; - QEMUTimerCB *write_timer_cb; + QEMUTimerCB *timer_cb[THROTTLE_MAX]; void *timer_opaque; } ThrottleTimers; diff --git a/util/throttle.c b/util/throttle.c index 81f247a8d1..5642e61763 100644 --- a/util/throttle.c +++ b/util/throttle.c @@ -199,10 +199,12 @@ static bool throttle_compute_timer(ThrottleState *ts, void throttle_timers_attach_aio_context(ThrottleTimers *tt, AioContext *new_context) { - tt->timers[0] = aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->read_timer_cb, tt->timer_opaque); - tt->timers[1] = aio_timer_new(new_context, tt->clock_type, SCALE_NS, - tt->write_timer_cb, tt->timer_opaque); + 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); } /* @@ -236,8 +238,8 @@ void throttle_timers_init(ThrottleTimers *tt, memset(tt, 0, sizeof(ThrottleTimers)); tt->clock_type = clock_type; - tt->read_timer_cb = read_timer_cb; - tt->write_timer_cb = write_timer_cb; + tt->timer_cb[THROTTLE_READ] = read_timer_cb; + tt->timer_cb[THROTTLE_WRITE] = write_timer_cb; tt->timer_opaque = timer_opaque; throttle_timers_attach_aio_context(tt, aio_context); } @@ -256,7 +258,7 @@ void throttle_timers_detach_aio_context(ThrottleTimers *tt) { int i; - for (i = 0; i < 2; i++) { + for (i = 0; i < THROTTLE_MAX; i++) { throttle_timer_destroy(&tt->timers[i]); } }