Re: [Qemu-devel] [RFC PATCH 3/4] cpu_exec: Add sleeping algorithm

[Sebastian Tanase]

Hello,

First of all thank you for your feedback.

----- Mail original -----
> De: "Paolo Bonzini" <address@hidden>
> À: "Sebastian Tanase" <address@hidden>, address@hidden
> Cc: address@hidden, address@hidden, address@hidden, "peter maydell" 
> <address@hidden>,
> address@hidden, address@hidden, address@hidden, address@hidden, 
> address@hidden, address@hidden,
> address@hidden
> Envoyé: Mardi 27 Mai 2014 17:15:17
> Objet: Re: [RFC PATCH 3/4] cpu_exec: Add sleeping algorithm
> 
> Il 27/05/2014 16:54, Sebastian Tanase ha scritto:
> >                              if (insns_left > 0) {
> >                                  /* Execute remaining instructions.
> >                                   */
> >                                  cpu_exec_nocache(env, insns_left,
> >                                  tb);
> > +                                if (icount_align_option) {
> > +                                    instr_exec_time = original_low
> > -
> > +
> >                                                      
> > cpu->icount_decr.u16.low;
> > +                                    instr_exec_time =
> > instr_exec_time <<
> > +
> >                                                      icount_time_shift;
> > +                                    diff_clk += instr_exec_time;
> > +                                    if (diff_clk >
> > VM_CLOCK_ADVANCE) {
> > +                                        delay_host();
> > +                                    }
> > +                                }
> 
> Why doesn't it have to update original_low and original_extra, and
> why doesn't it have to take into account original_extra (the new
> cpu->icount_extra is zero, but what about the old one)?
> 

The reason I don't update original_low and original_extra is because
in this case the function will exit (from what I understood):

                         if (insns_left > 0) {
                                /* Execute remaining instructions.  */
                                cpu_exec_nocache(env, insns_left, tb);
                                if (icount_align_option) {
                                    instr_exec_time = original_low -
                                                      cpu->icount_decr.u16.low;
                                    instr_exec_time = instr_exec_time <<
                                                      icount_time_shift;
                                    diff_clk += instr_exec_time;
                                    if (diff_clk > VM_CLOCK_ADVANCE) {
                                        delay_host();
                                    }
                                }
                            }
                            cpu->exception_index = EXCP_INTERRUPT;
                            next_tb = 0;
                            cpu_loop_exit(cpu);

cpu_loop_exit will bring us at the first for loop, then into the else part of
the sigsetjmp then again in the for loop; the cpu->exception_index is 
EXCP_INTERRUPT 
and the for loop breaks and the function finishes.
The 2 instruction counters (original_low and original_extra) will be updated 
when
we reenter cpu_exec:

                if (icount_align_option) {
                    /* Calculate difference between guest clock and host clock.
                       This delay includes the delay of the last cycle, so
                       what we have to do is sleep until it is 0. As for the
                       advance/delay we gain here, we try to fix it next time. 
*/
                    realtime_clock = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
                    diff_clk = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) -
                               realtime_clock;
                    /* Print (every 2s max) if the guest is behind the host */
                    if (-diff_clk > 0 &&
                       (realtime_clock - last_realtime_clock) / 1000000000LL
                        >= MAX_DELAY_PRINT_RATE) {
                        print_delay();
                        last_realtime_clock = realtime_clock;
                    }
                    original_extra = cpu->icount_extra;
                    original_low = cpu->icount_decr.u16.low;
                }



> > +                    case 1:
> > +                    case 0:
> > +                        if (icount_align_option) {
> > +                            instr_exec_time = original_extra -
> > +                                              cpu->icount_extra +
> > +                                              original_low -
> > +
> >                                              cpu->icount_decr.u16.low;
> > +                            instr_exec_time = instr_exec_time <<
> > +                                              icount_time_shift;
> 
> I wonder if tracking cpu->icount_extra + cpu->icount_decr.u16.low
> (instead of the two separately) will lead to nicer code overall.
> 
>        int64_t current_icount = cpu->icount_extra +
>        cpu->icount_decr.u16.low;
>        diff_clk += (current_icount - old_icount) <<
>        icount_time_shift;
>        old_icount = current_icount;
> 
> Paolo
> 
> > +                            diff_clk += instr_exec_time;
> > +                            original_extra = cpu->icount_extra;
> > +                            original_low =
> > cpu->icount_decr.u16.low;
> > +                        }
> > +                        break;
> 
> This can go in the "default" label.
> 
> >                      default:
> 
> 
I will update the code as you suggested.

Regards,

---
Sebastian Tanase

Open Wide Ingenierie
23, rue Daviel
75013 Paris - France
www.openwide.fr