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

[Sebastian Tanase]

----- Mail original -----
> De: "Paolo Bonzini" <address@hidden>
> À: "Sebastian Tanase" <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, address@hidden
> Envoyé: Mardi 27 Mai 2014 17:54:33
> Objet: Re: [RFC PATCH 3/4] cpu_exec: Add sleeping algorithm
> 
> Il 27/05/2014 17:49, Sebastian Tanase ha scritto:
> >> 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):
> 
> You're right.  Of course this becomes moot if you move the updating
> code
> to a separate function; otherwise, please add a comment.
> 
> You didn't answer the rest of the question---is it right to ignore
> original_extra, or was it a bug?
> 
> Paolo
> 
>From my understanding, cpu->icount_extra is only updated if we end up in the
TB_EXIT_ICOUNT_EXPIRED case and if the interrupt flag is 0 
(cpu->icount_decr.u32 is 
positive when treated as a signed integer). In this situation, we also update 
original_extra.
                    case TB_EXIT_ICOUNT_EXPIRED:
                    {
                        /* Instruction counter expired.  */
                        int insns_left;
                        tb = (TranslationBlock *)(next_tb & ~TB_EXIT_MASK);
                        insns_left = cpu->icount_decr.u32;
                        if (cpu->icount_extra && insns_left >= 0) {
                            /* Refill decrementer and continue execution.  */
                            cpu->icount_extra += insns_left;
                            if (cpu->icount_extra > 0xffff) {
                                insns_left = 0xffff;
                            } else {
                                insns_left = cpu->icount_extra;
                            }
                            cpu->icount_extra -= insns_left;
                            cpu->icount_decr.u16.low = insns_left;
                         if (icount_align_option) {
                                /* Instruction counters have been
                                   updated so we update ours */
                                original_extra = cpu->icount_extra;
                                original_low = cpu->icount_decr.u16.low;
                            }
                        ...
When cpu->icount_extra becomes 0, original_extra also becomes 0, so next time
we end up in the TB_EXIT_ICOUNT_EXPIRED case, we will go into the else branch
and given that cpu->icount_extra is 0, original_extra has to be 0 also.
                        } else {
                            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();
                                    }
                                }
                            }

That being said, I think that updating original_extra in other cases is useless.
                        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;
                            diff_clk += instr_exec_time;
                            original_extra = cpu->icount_extra;
                            original_low = cpu->icount_decr.u16.low;
                        }

Overall, I think that using one counter to track both icount_extra and
icount_decr.u16.low, as you suggested, is better and makes the code easier
to understand.

Regards,

Sebastian Tanase

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