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

[Meng Xu <mengxu@xxxxxxxxxxxxx>]

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.

Thanks,

Meng

-- 
------------
Meng Xu
PhD Student in Computer and Information Science
University of Pennsylvania
http://www.cis.upenn.edu/~mengxu/

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