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

[George Dunlap <george.dunlap@xxxxxxxxxx>]

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'. :-)

> + * 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.

Otherwise, looks good, thanks.

 -George

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