Re: [Qemu-block] [PATCH RFC v3 1/8] block: move ThrottleGroup membership to ThrottleGroupMember

[Manos Pitsidianakis]

On Tue, Jun 27, 2017 at 02:08:54PM +0200, Alberto Garcia wrote:
> On Fri 23 Jun 2017 02:46:53 PM CEST, Manos Pitsidianakis wrote:
> > This commit gathers ThrottleGroup membership details from
> > BlockBackendPublic into ThrottleGroupMember and refactors existing code
> > to use the structure.
> >
> > Signed-off-by: Manos Pitsidianakis <address@hidden>
>
> Hey Manos, thanks for the patch. It looks good to me in general, I only
> have a couple of comments:
>
> >      /* If no IO are queued for scheduling on the next round robin token
> > -     * then decide the token is the current bs because chances are
> > -     * the current bs get the current request queued.
> > +     * then decide the token is the current tgm because chances are
> > +     * the current tgm get the current request queued.
>
> I wonder if it's not better to use 'member' instead of 'tgm' in general.
> My impression is that the former is clearer and not too long, but I
> don't have a very strong opinion so you can keep it like this if you
> want.

I will change it, no problem,

> > -/* Check if an I/O request needs to be throttled, wait and set a timer
> > - * if necessary, and schedule the next request using a round robin
> > - * algorithm.
> > +/* Check if an I/O request needs to be throttled, wait and set a timer if
> > + * necessary, and schedule the next request using a round robin algorithm.
> >   *
>
> There's a few cases like this when you reformat a paragraph but don't
> actually change the text. I think it just adds unnecessary noise to the
> diff...

Wiki says "It's OK to fix coding style issues in the immediate area (few 
lines) of the lines you're changing" so I left the reformats. Since they 
are noticed they must be too noisey.. I will either remove the changes 
or do a restyling patch later.

> > --- a/include/qemu/throttle.h
> > +++ b/include/qemu/throttle.h
> > @@ -27,6 +27,7 @@
> >
> >  #include "qemu-common.h"
> >  #include "qemu/timer.h"
> > +#include "qemu/coroutine.h"
> >
> >  #define THROTTLE_VALUE_MAX 1000000000000000LL
> >
> > @@ -153,4 +154,29 @@ bool throttle_schedule_timer(ThrottleState *ts,
> >
> >  void throttle_account(ThrottleState *ts, bool is_write, uint64_t size);
> >
> > +
> > +/* The ThrottleGroupMember structure indicates membership in a ThrottleGroup
> > + * and holds related data.
> > + */
> > +
> > +typedef struct ThrottleGroupMember {
> > +    /* throttled_reqs_lock protects the CoQueues for throttled requests.  */
> > +    CoMutex      throttled_reqs_lock;
> > +    CoQueue      throttled_reqs[2];
> > +
> > +    /* Nonzero if the I/O limits are currently being ignored; generally
> > +     * it is zero.  Accessed with atomic operations.
> > +     */
> > +    unsigned int io_limits_disabled;
> > +
> > +    /* The following fields are protected by the ThrottleGroup lock.
> > +     * See the ThrottleGroup documentation for details.
> > +     * throttle_state tells us if I/O limits are configured. */
> > +    ThrottleState *throttle_state;
> > +    ThrottleTimers throttle_timers;
> > +    unsigned       pending_reqs[2];
> > +    QLIST_ENTRY(ThrottleGroupMember) round_robin;
> > +
> > +} ThrottleGroupMember;
> > +
>
> Any reason why you add this to throttle.h (which is generic throttling
> code independent from the block layer) and not to throttle-groups.h?

I put it there because it's not directly ThrottleGroup-related, but 
you're right, if throttle.h is block layer free it should remain that 
way.
> Otherwise it all looks good to me, thanks!
>
> Berto

signature.asc
Description: PGP signature