Re: [Xen-devel] [PATCH v2 09/10] libxl: allow to set the ratelimit value online for Credit2

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Fri, 2016-09-30 at 11:33 +0100, George Dunlap wrote:
> On 30/09/16 11:30, Ian Jackson wrote:
> > 
> > Dario Faggioli writes ("[PATCH v2 09/10] libxl: allow to set the
> > ratelimit value online for Credit2"):
> > > 
> > > This is the remaining part of the plumbing (the libxl
> > > one) necessary to be able to change the value of the
> > > ratelimit_us parameter online, for Credit2 (like it is
> > > already for Credit1).
> > 
> > Thanks.  I have some coding style nits, all but one of which are
> > utterly trivial.  If it hadn't been for the `rc=0' one I would have
> > acked this patch as-is.
> > 
Ok, thanks for looking this quickly. :-)

> > > +int libxl_sched_credit2_params_set(libxl_ctx *ctx, uint32_t
> > > poolid,
> > > +                                   libxl_sched_credit2_params
> > > *scinfo)
> > > +{
> > > +    struct xen_sysctl_credit2_schedule sparam;
> > > +    int r, rc;
> > > +    GC_INIT(ctx);
> > > +
> > > +    rc = sched_ratelimit_check(gc, scinfo->ratelimit_us);
> > > +    if (rc) {
> > > +        goto out;
> > > +    }
> > 
> > When you respin this, I think IWBNI you could change this to
> >       if (rc)
> >           goto out;
> > or
> >       if (rc) goto out;
> > both of which are encouraged by the coding style, and are more
> > common
> > inside libxl.
> > 
Right!

> > An earlier hunk in this patch does not adjust from a similar
> > construct
> > where the { } are becoming newly unnecessary.  You may want to
> > adjust
> > that too, but I won't object if you don't feel like it.
> > 
Sure.

> > > +    sparam.ratelimit_us = scinfo->ratelimit_us;
> > > +
> > > +    r = xc_sched_credit2_params_set(ctx->xch, poolid, &sparam);
> > > +    if ( r < 0 ) {
> >            ^     ^
> > Coding style: libxl does not use the spaces just inside the ( ).
> > 
Bah, how did this got here! :-/

> > > 
> > > +        LOGE(ERROR, "Setting Credit2 scheduler parameters");
> > > +        rc = ERROR_FAIL;
> > > +        goto out;
> > > +    }
> > > +
> > > +    scinfo->ratelimit_us = sparam.ratelimit_us;
> > > +
> > > + out:
> > > +    GC_FREE;
> > > +    return rc;
> > 
> > This should have an explicit assignment rc=0 before the out
> > label.  As
> > it is, it will happen to be 0 anyway, so that's a stylistic fix.
> 
> If I'm happy with it as-is, and I fix these up before I check them
> in,
> can I put your Ack on it?  
>
So, just FTR, it doesn't just "happen to be 0 anyway", that was on
purpose. :-)

I.e., I think the function is small and simple enough for me to take
advantage of the knowledge that, when we get to the out: label, we've
necessarily called sched_ratelimit_check(), and saved the return code
in 'rc', and that such return code is either 0, or the proper error
value we want to return.

That being said, it's no big deal, and I'm fine whatever the code ends
up looking.

> Or would you rather have Dario re-send it to
> make sure the changes get implemented correctly?
> 
Again, I'm fine either way. For your convenience, I'm attaching here
two variants of this patch. Both have the coding style fixes suggested.
One (the one with '-rc' in the name), includes the 'rc = 0', the other
doesn't.

Feel free to pickup whichever you find best/more convenient/etc, or
feel free to ignore them, if they're not useful. I'm fine in any case.
:-)

Thanks and Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: libxl-allow-disabling-ratelimit-online.patch
Description: Text Data

Attachment: libxl-allow-disabling-ratelimit-online_rc.patch
Description: Text Data

Attachment: signature.asc
Description: This is a digitally signed message part

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel