[v3] xen: sched: removal of redundant check in Credit

Commit Message

Praveen Kumar Dec. 16, 2016, 7:23 p.m. UTC

The patch gets rid of a redundant check in csched_vcpu_acct. In fact,
the function is only called from csched_tick, which already checks
that current is not the idle vcpu. The patch also adds an ASSERT to
the same effect, in order to make assumption ( i.e., no calling this
on idle vcpus) even more clear and as a guard for future mis-use.

Signed-off-by: Praveen Kumar <[e-mail]>
Acked-by: Dario Faggioli <[e-mail]>
---
 xen/common/sched_credit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--
1.8.5.6

Comments

Dario Faggioli Dec. 17, 2016, 1:46 a.m. UTC | #1

On Sat, 2016-12-17 at 00:53 +0530, Praveen Kumar wrote:
> The patch gets rid of a redundant check in csched_vcpu_acct. In fact,
> the function is only called from csched_tick, which already checks
> that current is not the idle vcpu. The patch also adds an ASSERT to
> the same effect, in order to make assumption ( i.e., no calling this
> on idle vcpus) even more clear and as a guard for future mis-use.
> 
> Signed-off-by: Praveen Kumar <[e-mail]>
> Acked-by: Dario Faggioli <[e-mail]>
>
Better than before. But still, if I:
 - save this mail as mbox
 - try to import it in git
it fails.

OTOH, if I:
 - save this mail as mbox
 - run dos2unix on the mbox file
 - try to import it in git
it works!

Which, since you're now using git-send-email, makes me think it may be
your editor/OS which is at fault and inserts spurious stuff at line
breaks (the classic CR vs. CR+LF thing).

What editor on what OS are you using?

Another possibility is that there is something wrong here at my hand.
George, do you have (similar) issues when trying to apply this patch?

Regards,
Dario

Praveen Kumar Dec. 17, 2016, 8:14 a.m. UTC | #2

Hi,

On Sat, Dec 17, 2016 at 7:16 AM, Dario Faggioli <[e-mail]>
wrote:

> On Sat, 2016-12-17 at 00:53 +0530, Praveen Kumar wrote:
> > The patch gets rid of a redundant check in csched_vcpu_acct. In fact,
> > the function is only called from csched_tick, which already checks
> > that current is not the idle vcpu. The patch also adds an ASSERT to
> > the same effect, in order to make assumption ( i.e., no calling this
> > on idle vcpus) even more clear and as a guard for future mis-use.
> >
> > Signed-off-by: Praveen Kumar <[e-mail]>
> > Acked-by: Dario Faggioli <[e-mail]>
> >
> Better than before. But still, if I:
>  - save this mail as mbox
>  - try to import it in git
> it fails.
>
> OTOH, if I:
>  - save this mail as mbox
>  - run dos2unix on the mbox file
>  - try to import it in git
> it works!
>
> Which, since you're now using git-send-email, makes me think it may be
> your editor/OS which is at fault and inserts spurious stuff at line
> breaks (the classic CR vs. CR+LF thing).
>
> What editor on what OS are you using?
>

I am using SLED machine and vim editor. I tried checking with vimdiff over
the patch generated and mbox file ( create by evolution ), I found spurious
stuff (^M) as mentioned added. After dos2unix, the mbox file was same to
that of the patch sent.

>
> Another possibility is that there is something wrong here at my hand.
> George, do you have (similar) issues when trying to apply this patch?
>
> 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)

Praveen Kumar Dec. 20, 2016, 9:04 a.m. UTC | #3

Hi Dario,

I tried with 'git am' to apply the patch after downloading the mbox file,
that worked fine. Do let me know if that is ok.

Regards,

~Praveen.

On Sat, Dec 17, 2016 at 1:44 PM, Praveen Kumar <[e-mail]>
wrote:

> Hi,
>
> On Sat, Dec 17, 2016 at 7:16 AM, Dario Faggioli <[e-mail]
> > wrote:
>
>> On Sat, 2016-12-17 at 00:53 +0530, Praveen Kumar wrote:
>> > The patch gets rid of a redundant check in csched_vcpu_acct. In fact,
>> > the function is only called from csched_tick, which already checks
>> > that current is not the idle vcpu. The patch also adds an ASSERT to
>> > the same effect, in order to make assumption ( i.e., no calling this
>> > on idle vcpus) even more clear and as a guard for future mis-use.
>> >
>> > Signed-off-by: Praveen Kumar <[e-mail]>
>> > Acked-by: Dario Faggioli <[e-mail]>
>> >
>> Better than before. But still, if I:
>>  - save this mail as mbox
>>  - try to import it in git
>> it fails.
>>
>> OTOH, if I:
>>  - save this mail as mbox
>>  - run dos2unix on the mbox file
>>  - try to import it in git
>> it works!
>>
>> Which, since you're now using git-send-email, makes me think it may be
>> your editor/OS which is at fault and inserts spurious stuff at line
>> breaks (the classic CR vs. CR+LF thing).
>>
>> What editor on what OS are you using?
>>
>
> I am using SLED machine and vim editor. I tried checking with vimdiff over
> the patch generated and mbox file ( create by evolution ), I found spurious
> stuff (^M) as mentioned added. After dos2unix, the mbox file was same to
> that of the patch sent.
>
>>
>> Another possibility is that there is something wrong here at my hand.
>> George, do you have (similar) issues when trying to apply this patch?
>>
>> 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)
>
>
>

Dario Faggioli Dec. 20, 2016, 1:18 p.m. UTC | #4

On Tue, 2016-12-20 at 14:34 +0530, Praveen Kumar wrote:
> Hi Dario,
> 
> I tried with 'git am' to apply the patch after downloading the mbox
> file, that worked fine. Do let me know if that is ok.
> 
Well, if you tried and it works, I'm sure it is. I don't really use
`git am` as part of my workload, that's why I did not try.

I've now done it as a test, and I confirm it works. So it's probably an
StGit issue, or something else. In fact, I said in my email that,
although I was reporting an issue I was having, I wasn't sure how
relevant it was, and could not rule out it was an issue with my own
setup.

Thanks for trying `git am` and reporting it works. This patch has my
Ack already, which means I'm happy with it. It's now up to George, or
another committer, to either comment or put it in.

Let's a little bit of time for this to happen, especially considering
the time of the year. :-D
If it will take too long, you can ping the thread with an email.

Regards,
Dario

George Dunlap Dec. 28, 2016, 9:02 a.m. UTC | #5

On 17/12/16 01:46, Dario Faggioli wrote:
> On Sat, 2016-12-17 at 00:53 +0530, Praveen Kumar wrote:
>> The patch gets rid of a redundant check in csched_vcpu_acct. In fact,
>> the function is only called from csched_tick, which already checks
>> that current is not the idle vcpu. The patch also adds an ASSERT to
>> the same effect, in order to make assumption ( i.e., no calling this
>> on idle vcpus) even more clear and as a guard for future mis-use.
>>
>> Signed-off-by: Praveen Kumar <[e-mail]>
>> Acked-by: Dario Faggioli <[e-mail]>
>>
> Better than before. But still, if I:
>  - save this mail as mbox
>  - try to import it in git
> it fails.
> 
> OTOH, if I:
>  - save this mail as mbox
>  - run dos2unix on the mbox file
>  - try to import it in git
> it works!

FWIW the script I use runs dos2unix on everything I get via e-mail
(either saved as an mbox or as an attachment) as a matter of course.
Are you actually sometimes able to git am mbox'd files *without* running
dos2unix?

 -George

Dario Faggioli Dec. 28, 2016, 2:23 p.m. UTC | #6

On Wed, 2016-12-28 at 09:02 +0000, George Dunlap wrote:
> On 17/12/16 01:46, Dario Faggioli wrote:
> > On Sat, 2016-12-17 at 00:53 +0530, Praveen Kumar wrote:
> > > Signed-off-by: Praveen Kumar <[e-mail]>
> > > Acked-by: Dario Faggioli <[e-mail]>
> > > 
> > Better than before. But still, if I:
> >  - save this mail as mbox
> >  - try to import it in git
> > it fails.
> > 
> > OTOH, if I:
> >  - save this mail as mbox
> >  - run dos2unix on the mbox file
> >  - try to import it in git
> > it works!
> 
> FWIW the script I use runs dos2unix on everything I get via e-mail
> (either saved as an mbox or as an attachment) as a matter of course.
> Are you actually sometimes able to git am mbox'd files *without*
> running
> dos2unix?
> 
Not really, but main reason is I don't `git am' at all! I use `stg
import', but not very often, and I also end up pre-filtering with
dos2unix.

I mentioned it to Praveen as a potential issue, out of _my_ ignorance
of your workflow as committers. It turns out it's normal.. So, thanks
for letting me know, and sorry for the noise. :-)

Regards,
Dario

Patch

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index fc3a321..dfe8545 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -941,6 +941,7 @@  csched_vcpu_acct(struct csched_private *prv, unsigned int cpu)

     ASSERT( current->processor == cpu );
     ASSERT( svc->sdom != NULL );
+    ASSERT( !is_idle_vcpu(svc->vcpu) );

     /*
      * If this VCPU's priority was boosted when it last awoke, reset it.
@@ -957,8 +958,7 @@  csched_vcpu_acct(struct csched_private *prv, unsigned int cpu)
     /*
      * Update credits
      */
-    if ( !is_idle_vcpu(svc->vcpu) )
-        burn_credits(svc, NOW());
+    burn_credits(svc, NOW());

     /*
      * Put this VCPU and domain back on the active list if it was