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