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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

On 22/06/16 17:16, Meng Xu wrote:
> On Wed, Jun 22, 2016 at 11:51 AM, George Dunlap
> <george.dunlap@xxxxxxxxxx> wrote:
>> On Mon, May 16, 2016 at 12:54 AM, Tianyang Chen <tiche@xxxxxxxxxxxxxx> wrote:
>>> No functional change:
>>>  -Various coding style fix
>>>  -Added comments for UPDATE_LIMIT_SHIFT.
>>>
>>> Signed-off-by: Tianyang Chen <tiche@xxxxxxxxxxxxxx>
>>
>> Hey Tianyang,
>>
>> The changes here for the most part look good (with a few comments --
>> see below), but the title and changelog could use some work.
>>
>> For one, you're not actually doing any refactoring -- I'd call this
>> patch a "clean-up" patch.
>>
>> Secondly, you should go through and enumerate the different clean-ups
>> you do.  For instance, you mention why you remove the __ at the head
>> of functions in your cover letter, but you don't mention it here.
>>
>>> ---
>>>  xen/common/sched_rt.c |  106 
>>> ++++++++++++++++++++++++++-----------------------
>>>  1 file changed, 56 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/xen/common/sched_rt.c b/xen/common/sched_rt.c
>>> index 7f8f411..1584d53 100644
>>> --- a/xen/common/sched_rt.c
>>> +++ b/xen/common/sched_rt.c
>>> @@ -80,7 +80,7 @@
>>>   * in schedule.c
>>>   *
>>>   * The functions involes RunQ and needs to grab locks are:
>>> - *    vcpu_insert, vcpu_remove, context_saved, __runq_insert
>>> + *    vcpu_insert, vcpu_remove, context_saved, runq_insert
>>>   */
>>>
>>>
>>> @@ -107,6 +107,12 @@
>>>   */
>>>  #define RTDS_MIN_BUDGET     (MICROSECS(10))
>>>
>>> +/*
>>> + * UPDATE_LIMIT_SHIT: a constant used in rt_update_deadline(). When finding
>>
>> Missing an 'F'. :-)
> 
> Ah, my bad.. I should have caught these typos. :-(
> 
>>
>>> + * the next deadline, performing addition could be faster if the difference
>>> + * between cur_deadline and now is small. If the difference is bigger than
>>> + * 1024 * period, use multiplication.
>>> + */
>>>  #define UPDATE_LIMIT_SHIFT      10
>>>
>>>  /*
>>> @@ -158,25 +164,25 @@
>>>  static void repl_timer_handler(void *data);
>>>
>>>  /*
>>> - * Systme-wide private data, include global RunQueue/DepletedQ
>>> + * System-wide private data, include global RunQueue/DepletedQ
>>>   * Global lock is referenced by schedule_data.schedule_lock from all
>>>   * physical cpus. It can be grabbed via vcpu_schedule_lock_irq()
>>>   */
>>>  struct rt_private {
>>> -    spinlock_t lock;            /* the global coarse grand lock */
>>> -    struct list_head sdom;      /* list of availalbe domains, used for 
>>> dump */
>>> -    struct list_head runq;      /* ordered list of runnable vcpus */
>>> -    struct list_head depletedq; /* unordered list of depleted vcpus */
>>> -    struct list_head replq;     /* ordered list of vcpus that need 
>>> replenishment */
>>> -    cpumask_t tickled;          /* cpus been tickled */
>>> -    struct timer *repl_timer;   /* replenishment timer */
>>> +    spinlock_t lock;             /* the global coarse grand lock */
>>
>> * course-grained
>>
>> Also, I'm not sure what the point of indenting all these comments out
>> an extra space is.  I don't object, of course, if Meng doesn't object,
>> but at very least it could use a one-line explanation in the
>> changelog.
> 
> I think he is trying to align those comments to make them start from
> the same column. I was confused at the reason at the very beginning.
> Then I pulled his repo and checked this change.

Right -- well neither you as a reviewer nor anyone in the future looking
back at this changeset should have to try to guess what the purpose was;
if he did want to align them, that's perfectly fine, it just needs a
brief mention in the changelog. :-)

 -George



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