Re: [Xen-devel] [PATCH] xen: sched: rtds: refactor code

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

On Thu, 2016-05-12 at 23:34 -0400, Meng Xu wrote:
> On Wed, May 11, 2016 at 11:20 AM, Tianyang Chen <tiche@xxxxxxxxxxxxxx
> > wrote:
> > 
> > No functional change:
> >  -Various coding style fix
> >  -Added comments for UPDATE_LIMIT_SHIFT.
> > 
Hey, Tianyang, thanks for this.

> > Use non-atomic bit-ops:
> >  -Vcpu flags are checked and cleared atomically. Performance can be
> >   improved with corresponding non-atomic versions since schedule.c
> >   already has spin_locks in place.
> > 
And this too. However, I think the patch should be split at least in
two, one doing the style/comments cleanups, the other doing the
atomic/non-atomic switch.

> > Suggested-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
> It's better to add the link to the thread that has the suggestion.
> 
Yes, suggested-by tags are not especially useful, IMHO. I'm fine with
you using it, but as Meng says, a link would be more helpful. If you
send a series, which will have a cover letter, put the link(s) in the
cover letter rather than in the changelog, as that's an important
information when reviewing, much less when looking at git-log in 5
years time. :-)

> > @@ -930,7 +936,7 @@ burn_budget(const struct scheduler *ops, struct
> > rt_vcpu *svc, s_time_t now)
> >      if ( svc->cur_budget <= 0 )
> >      {
> >          svc->cur_budget = 0;
> > -        set_bit(__RTDS_depleted, &svc->flags);
> > +        __set_bit(__RTDS_depleted, &svc->flags);
> >      }
> > 
> >      /* TRACE */
> > @@ -955,7 +961,7 @@ burn_budget(const struct scheduler *ops, struct
> > rt_vcpu *svc, s_time_t now)
> >   * lock is grabbed before calling this function
> The comment says "lock is grabbed before calling this function".
> IIRC,  we use __ to represent that we grab the lock before call this
> function.
> Then this change violates the convention.
> 
Yeah, but it was a bad convention, IMO, at least for sched_*.c files. 

In fact, it is debatable whether one or two underscore is what should
really be used.

Also, that's not the only convention that lead to the introduction of
this king of prefix (for example, I've seen many times '__' used for
indicating some sort of 'raw' part of the operation... there are
examples of this in Xen too)... Which means one never knows which one
is the valid convention at some point, about '__'! :-O

But even if we leave this aside, and consider at the '__'==>locked
rule, well, pretty much all functions in sched_rt.c are called with the
proper locks held already. Basically, since in many occasione, the lock
has been taken in schedule.c before calling the hook, all the functions
eve called by an hook --end maybe even the hook itself-- should have
the '__' prefix, which defeats the purpose of the convention itself!

So, on this, I agree with Tianyang, and I think removing the '__' is a
good thing for this source file.

If there are places where we want to particularly stress the fact that
locks should have be taken already, then either add a comment or a
'_locked' suffix to the function name. But that should be the exception
rather than the rule and, out of the top of my head, I don't recall any
cases in sched_rt.c where that would be tremendously helpful...

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: signature.asc
Description: This is a digitally signed message part

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