Re: [CFR] Replacing while loops with proper division and multiplication

Hi Hans,

On Fri, Jun 5, 2015 at 12:09 AM, Hans Petter Selasky <hps_at_selasky.org> wrote:
> Hi,
>
> I was going through some timer code and found some unnecessary while loops
> in kern/kern_clocksource.c .
>
> I added some prints and found that during boot, "runs" can exceed 2000,
> while during regular usage runs is typically 1. Do you think it is worth to
> convert these loops into division and multiplications? It might make the CPU
> pipeline a tiny bit faster, having to skip some conditionals? And also
> possibly improve readability?
>
> What do you think?
>
> --HPS
>
>> Index: kern/kern_clocksource.c
>> ===================================================================
>> --- kern/kern_clocksource.c     (revision 283606)
>> +++ kern/kern_clocksource.c     (working copy)
>> _at__at_ -155,10 +155,11 _at__at_
>>  handleevents(sbintime_t now, int fake)
>>  {
>>         sbintime_t t, *hct;
>> +       sbintime_t runs;
>>         struct trapframe *frame;
>>         struct pcpu_state *state;
>>         int usermode;
>> -       int done, runs;
>> +       int done;
>>
>>         CTR3(KTR_SPARE2, "handle at %d:  now  %d.%08x",
>>             curcpu, (int)(now >> 32), (u_int)(now & 0xffffffff));
>> _at__at_ -173,12 +174,10 _at__at_
>>
>>         state = DPCPU_PTR(timerstate);
>>
>> -       runs = 0;
>> -       while (now >= state->nexthard) {
>> -               state->nexthard += tick_sbt;
>> -               runs++;
>> -       }
>> -       if (runs) {
>> +       runs = (now - state->nexthard) / tick_sbt;
>> +       if (runs > 0) {
>> +               printf("R%d ", (int)runs);
>> +               state->nexthard += tick_sbt * runs;
>>                 hct = DPCPU_PTR(hardclocktime);
>>                 *hct = state->nexthard - tick_sbt;
>>                 if (fake < 2) {

There is a difference in behavior in the two implementations when 'now
== state->nexthard'. In the loop-based implementation this would end
up with 'runs = 1' whereas in the division-based implementation it
would end up with 'runs = 0'.

I am not sure if this is intentional or just an oversight.

best
Neel

>> _at__at_ -186,25 +185,25 _at__at_
>>                         done = 1;
>>                 }
>>         }
>> -       runs = 0;
>> -       while (now >= state->nextstat) {
>> -               state->nextstat += statperiod;
>> -               runs++;
>> +       runs = (now - state->nextstat) / statperiod;
>> +       if (runs > 0) {
>> +               printf("S%d ", (int)runs);
>> +               state->nextstat += statperiod * runs;
>> +               if (fake < 2) {
>> +                       statclock_cnt(runs, usermode);
>> +                       done = 1;
>> +               }
>>         }
>> -       if (runs && fake < 2) {
>> -               statclock_cnt(runs, usermode);
>> -               done = 1;
>> -       }
>>         if (profiling) {
>> -               runs = 0;
>> -               while (now >= state->nextprof) {
>> -                       state->nextprof += profperiod;
>> -                       runs++;
>> +               runs = (now - state->nextprof) / profperiod;
>> +               if (runs > 0) {
>> +                       printf("T%d ", (int)runs);
>> +                       state->nextprof += profperiod * runs;
>> +                       if (!fake) {
>> +                               profclock_cnt(runs, usermode,
>> TRAPF_PC(frame));
>> +                               done = 1;
>> +                       }
>>                 }
>> -               if (runs && !fake) {
>> -                       profclock_cnt(runs, usermode, TRAPF_PC(frame));
>> -                       done = 1;
>> -               }
>>         } else
>>                 state->nextprof = state->nextstat;
>>         if (now >= state->nextcallopt) {
>
> _______________________________________________
> freebsd-current_at_freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-current
> To unsubscribe, send any mail to "freebsd-current-unsubscribe_at_freebsd.org"