xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
From: Meng Xu <mengxu@cis.upenn.edu>
To: Dario Faggioli <dario.faggioli@citrix.com>
Cc: "xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>,
	"Chen, Tianyang" <tiche@seas.upenn.edu>,
	Meng Xu <xumengpanda@gmail.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Dagaen Golomb <dgolomb@seas.upenn.edu>
Subject: Re: [PATCH v9]xen: sched: convert RTDS from time to event driven model
Date: Wed, 16 Mar 2016 10:20:48 -0400	[thread overview]
Message-ID: <CAENZ-+kVMfTbii8rh4E4-bZRK+hSTG5dbpJ9GNNHYzON82hb8A@mail.gmail.com> (raw)
In-Reply-To: <1458123811.3102.828.camel@citrix.com>

On Wed, Mar 16, 2016 at 6:23 AM, Dario Faggioli
<dario.faggioli@citrix.com> wrote:
> On Tue, 2016-03-15 at 23:40 -0400, Meng Xu wrote:
>> > > > @@ -115,6 +118,18 @@
>> > > >   #define RTDS_delayed_runq_add (1<<__RTDS_delayed_runq_add)
>> > > >
>> > > >   /*
>> > > > + * The replenishment timer handler needs to check this bit
>> > > > + * to know where a replenished vcpu was, when deciding which
>> > > > + * vcpu should tickle.
>> > > > + * A replenished vcpu should tickle if it was moved from the
>> > > > + * depleted queue to the run queue.
>> > > > + * + Set in burn_budget() if a vcpu has zero budget left.
>> > > > + * + Cleared and checked in the repenishment handler.
>> > >
>> > > It seems you have an extra + here...
>> > > Need to be removed.
>> > >
>> > > My bad, I didn't spot it out in last patch... :-(
>> > >
>> > You mean before "Cleared"? For __RTDS_scheduled there are '+'
>> > before
>> > 'Cleared', 'Checked', 'Set'.
>> Yes, those two +, are unnecessary. Isn't it?
>>
> I *think* the idea here was to sort of put down a bullet-ed list, but
> maybe we should ask the author. According to `git blame', is a certain
> Meng Xu, guy (commit 8726c055), anyone has his email address? :-D :-D

Ah-ha, let me try to find the Meng Xu. ;-)
Here we go. I cc.ed him... ;-)

>
> However, I don't particularly like either the style or the final result
> (in terms of wording), so, let's avoid doing more of that in new code
> (see my other email).

I checked the sched_credit.c and sched_credit2.c, it seems that either
+ or - is used as the list. When there are two levels of lists, both
are used.
However, it's inconsistent which one should be used at the top level.
Probably to keep the consistence in this file, we keep using + and
later when we want to clean up this style issue, if we will ,we can
replace them.

As to the comment, I will suggest:

/*
 * RTDS_was_depleted: Is a vcpus budget depleted?

 * + Set in burn_budget() when a vcpus budget turns to zero

 * + Checked and cleared in repl_handler() to replenish the budget

 */

What do you think?

BTW, how about other parts of the patch? Is there something that you don't like?
I think the invalid budget returned in rt_schedule() in this patch is
a serious logical bug.
If all of my comments are solved, I think it is in a good state and
I'm considering to send the reviewed-by tag in the near future.
However, I won't send the reviewed-by until I get your confirmation,
since this will be the first reviewed-by I will be giving as
maintainer. I'd like to take a safe step. :-D

Thanks and Best Regards,

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@lists.xen.org
http://lists.xen.org/xen-devel

  reply	other threads:[~2016-03-16 14:20 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-15  0:04 Tianyang Chen
2016-03-16  3:11 ` Meng Xu
2016-03-16  3:32   ` Chen, Tianyang
2016-03-16  3:40     ` Meng Xu
2016-03-16 10:23       ` Dario Faggioli
2016-03-16 14:20         ` Meng Xu [this message]
2016-03-16 14:44           ` Dario Faggioli
2016-03-16 20:51             ` Meng Xu
2016-03-16 14:25 ` Dario Faggioli
2016-03-16 15:43   ` Chen, Tianyang
2016-03-16 16:11     ` Dario Faggioli
2016-03-16 20:54     ` Meng Xu
2016-03-16 20:45   ` Meng Xu
2016-03-17  2:24   ` Chen, Tianyang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAENZ-+kVMfTbii8rh4E4-bZRK+hSTG5dbpJ9GNNHYzON82hb8A@mail.gmail.com \
    --to=mengxu@cis.upenn.edu \
    --cc=dario.faggioli@citrix.com \
    --cc=dgolomb@seas.upenn.edu \
    --cc=george.dunlap@citrix.com \
    --cc=tiche@seas.upenn.edu \
    --cc=xen-devel@lists.xenproject.org \
    --cc=xumengpanda@gmail.com \
    --subject='Re: [PATCH v9]xen: sched: convert RTDS from time to event driven model' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).