Re: cyclictest overflow instance tracking -- irc continued

Re: cyclictest overflow instance tracking -- irc continued

[John Kacur]

On Mon, 15 Oct 2012, Frank Rowand wrote:

Thanks for your further review Frank. Bhavesh - I think the right thing to 
do here, is to ask you to respin your patch. Can you make sure you are 
applying and testing against the latest. (0.84). Also, please put any 
white space clean-ups in a separate patch to make it easier to read the 
important patch. Oh, and send these in a new thread. If people could 
preface their mail with [PATCH RT-TESTS] that would also help.

Thank You.

John Kacur

 > 
> 16:13	jkacur	frowand_bot: would you like a Reviewed-by: credit for the cyclictest histogram overflow instance tracking patch?
> 16:13	jkacur	can I just add that or is the reviewer supposed to request it?
> 17:02	frowand	jkacur: sure, you can add my reviewed-by (at least for the first version, i didn't really look at the second version in detail)
> 
> For the first version, it was a drive-by review, just obvious stuff.
> 
> Now that I look at the second version a little closer (but still not in detail), it
> looks like he did some white space clean up, which should probably be a separate patch.
> 
> It also looks like he created his patch against an old version of cyclictest,
> because he has:
> 
> @@ -154,6 +155,7 @@ struct thread_stat {
>  	long redmax;
>  	long cycleofmax;
>  	long hist_overflow;
> +        long num_outliers;
>  };
> 
> 
> but struct thread_stat currently has another field:
> 
>         long redmax;
>         long cycleofmax;
>         long hist_overflow;
>         unsigned long now_after_next;
> };
> 
> and also the line numbers in the patch are off.
> 
> > from #linux-rt:
> > 
> > 17:07	jkacur	frowand: hmm, the patch doesn't appear to be working for me
> > 17:07	jkacur	I'll look at it more tomorrow
> > 17:08	jkacur	Thread 0: 18446744073709551615 00000 00001 00002 00003 00004 00005 00006 00007 00008 # 02841 others
> > 17:08	jkacur	is an example of broken output
> 
> 18446744073709551615 is 0xffffffffffffffff (or -1LL)
> 
> It looks like in timerthread(), the line:
> 
> +					stat->outliers[stat->num_outliers++] = stat->cycles - 1;
> 
> is picking up stat->cycles == zero.  Looking at the current version of cyclictest, for the
> first event cycles should already be one at this point.  But given that the line numbers in
> the patch are not consistent with the current cyclictest, I don't know where that chunk
> actually got inserted.
> 
> 
> -Frank
> 
> 

Re: cyclictest overflow instance tracking -- irc continued

[Bhavesh Davda]

Sorry about that John, Frank! Don't know how that happened…

Please look for two separate patches against the right ToT: one for whitespace cleanup, and the other with this feature, in the next few hours…

Thanks

- Bhavesh

On Oct 16, 2012, at 3:56 AM, John Kacur wrote:

> 
> 
> On Mon, 15 Oct 2012, Frank Rowand wrote:
> 
> Thanks for your further review Frank. Bhavesh - I think the right thing to 
> do here, is to ask you to respin your patch. Can you make sure you are 
> applying and testing against the latest. (0.84). Also, please put any 
> white space clean-ups in a separate patch to make it easier to read the 
> important patch. Oh, and send these in a new thread. If people could 
> preface their mail with [PATCH RT-TESTS] that would also help.
> 
> Thank You.
> 
> John Kacur
> 
>> 
>> 16:13	jkacur	frowand_bot: would you like a Reviewed-by: credit for the cyclictest histogram overflow instance tracking patch?
>> 16:13	jkacur	can I just add that or is the reviewer supposed to request it?
>> 17:02	frowand	jkacur: sure, you can add my reviewed-by (at least for the first version, i didn't really look at the second version in detail)
>> 
>> For the first version, it was a drive-by review, just obvious stuff.
>> 
>> Now that I look at the second version a little closer (but still not in detail), it
>> looks like he did some white space clean up, which should probably be a separate patch.
>> 
>> It also looks like he created his patch against an old version of cyclictest,
>> because he has:
>> 
>> @@ -154,6 +155,7 @@ struct thread_stat {
>> 	long redmax;
>> 	long cycleofmax;
>> 	long hist_overflow;
>> +        long num_outliers;
>> };
>> 
>> 
>> but struct thread_stat currently has another field:
>> 
>>        long redmax;
>>        long cycleofmax;
>>        long hist_overflow;
>>        unsigned long now_after_next;
>> };
>> 
>> and also the line numbers in the patch are off.
>> 
>>> from #linux-rt:
>>> 
>>> 17:07	jkacur	frowand: hmm, the patch doesn't appear to be working for me
>>> 17:07	jkacur	I'll look at it more tomorrow
>>> 17:08	jkacur	Thread 0: 18446744073709551615 00000 00001 00002 00003 00004 00005 00006 00007 00008 # 02841 others
>>> 17:08	jkacur	is an example of broken output
>> 
>> 18446744073709551615 is 0xffffffffffffffff (or -1LL)
>> 
>> It looks like in timerthread(), the line:
>> 
>> +					stat->outliers[stat->num_outliers++] = stat->cycles - 1;
>> 
>> is picking up stat->cycles == zero.  Looking at the current version of cyclictest, for the
>> first event cycles should already be one at this point.  But given that the line numbers in
>> the patch are not consistent with the current cyclictest, I don't know where that chunk
>> actually got inserted.
>> 
>> 
>> -Frank
>> 
>> 

--
Bhavesh Davda
bhavesh@vmware.com






--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cyclictest overflow instance tracking -- irc continued

[Bhavesh Davda]

Also, just to confirm: where is the GIT tree located?

I was sync'ed to https://github.com/clrkwllms/rt-tests.git

But I also see a tree at http://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rt-tests.git

Thanks

On Oct 16, 2012, at 9:11 AM, Bhavesh Davda wrote:

> Sorry about that John, Frank! Don't know how that happened…
> 
> Please look for two separate patches against the right ToT: one for whitespace cleanup, and the other with this feature, in the next few hours…
> 
> Thanks
> 
> - Bhavesh
> 
> On Oct 16, 2012, at 3:56 AM, John Kacur wrote:
> 
>> 
>> 
>> On Mon, 15 Oct 2012, Frank Rowand wrote:
>> 
>> Thanks for your further review Frank. Bhavesh - I think the right thing to 
>> do here, is to ask you to respin your patch. Can you make sure you are 
>> applying and testing against the latest. (0.84). Also, please put any 
>> white space clean-ups in a separate patch to make it easier to read the 
>> important patch. Oh, and send these in a new thread. If people could 
>> preface their mail with [PATCH RT-TESTS] that would also help.
>> 
>> Thank You.
>> 
>> John Kacur
>> 
>>> 
>>> 16:13	jkacur	frowand_bot: would you like a Reviewed-by: credit for the cyclictest histogram overflow instance tracking patch?
>>> 16:13	jkacur	can I just add that or is the reviewer supposed to request it?
>>> 17:02	frowand	jkacur: sure, you can add my reviewed-by (at least for the first version, i didn't really look at the second version in detail)
>>> 
>>> For the first version, it was a drive-by review, just obvious stuff.
>>> 
>>> Now that I look at the second version a little closer (but still not in detail), it
>>> looks like he did some white space clean up, which should probably be a separate patch.
>>> 
>>> It also looks like he created his patch against an old version of cyclictest,
>>> because he has:
>>> 
>>> @@ -154,6 +155,7 @@ struct thread_stat {
>>> 	long redmax;
>>> 	long cycleofmax;
>>> 	long hist_overflow;
>>> +        long num_outliers;
>>> };
>>> 
>>> 
>>> but struct thread_stat currently has another field:
>>> 
>>>       long redmax;
>>>       long cycleofmax;
>>>       long hist_overflow;
>>>       unsigned long now_after_next;
>>> };
>>> 
>>> and also the line numbers in the patch are off.
>>> 
>>>> from #linux-rt:
>>>> 
>>>> 17:07	jkacur	frowand: hmm, the patch doesn't appear to be working for me
>>>> 17:07	jkacur	I'll look at it more tomorrow
>>>> 17:08	jkacur	Thread 0: 18446744073709551615 00000 00001 00002 00003 00004 00005 00006 00007 00008 # 02841 others
>>>> 17:08	jkacur	is an example of broken output
>>> 
>>> 18446744073709551615 is 0xffffffffffffffff (or -1LL)
>>> 
>>> It looks like in timerthread(), the line:
>>> 
>>> +					stat->outliers[stat->num_outliers++] = stat->cycles - 1;
>>> 
>>> is picking up stat->cycles == zero.  Looking at the current version of cyclictest, for the
>>> first event cycles should already be one at this point.  But given that the line numbers in
>>> the patch are not consistent with the current cyclictest, I don't know where that chunk
>>> actually got inserted.
>>> 
>>> 
>>> -Frank
>>> 
>>> 
> 
> --
> Bhavesh Davda
> bhavesh@vmware.com
> 
> 
> 
> 
> 
> 

--
Bhavesh Davda
bhavesh@vmware.com






--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: cyclictest overflow instance tracking -- irc continued

[John Kacur]

On Tue, 16 Oct 2012, Bhavesh Davda wrote:

> Also, just to confirm: where is the GIT tree located?
> 
> I was sync'ed to https://github.com/clrkwllms/rt-tests.git

The above one was a temporary place while the problems with kernel.org 
were on going.

> 
> But I also see a tree at http://git.kernel.org/pub/scm/linux/kernel/git/clrkwllms/rt-tests.git

The above one is the correct one.

Thanks
John