| Message ID | 20170405132503.32125-12-alex.bennee@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | icount and misc MTTCG fixes for 2.9-rc4 | expand |
On 05/04/2017 15:25, Alex Bennée wrote: > This ensures each time the vCPU thread reads the icount we update the > master timer_state.qemu_icount field. This way as long as updates are > in BQL protected sections (which they should be) the main-loop can > never come to update the log and find time has gone backwards. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > --- > cpus.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/cpus.c b/cpus.c > index ff75af449a..63de033cc8 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu) > > int64_t cpu_get_icount_raw(void) > { > - int64_t icount; > CPUState *cpu = current_cpu; > > - icount = atomic_read(&timers_state.qemu_icount); > if (cpu && cpu->running) { > if (!cpu->can_do_io) { > fprintf(stderr, "Bad icount read\n"); > exit(1); > } > /* Take into account what has run */ > - icount += cpu_get_icount_executed(cpu); > + cpu_update_icount(cpu); > } > - return icount; > + return atomic_read(&timers_state.qemu_icount); > } > > /* Return the virtual CPU time, based on the instruction counter. */ > Maybe the update should be done in gen_io_start instead. There shouldn't be any interference between vCPU and I/O threads except between gen_io_start and gen_io_end. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 05/04/2017 15:25, Alex Bennée wrote: >> This ensures each time the vCPU thread reads the icount we update the >> master timer_state.qemu_icount field. This way as long as updates are >> in BQL protected sections (which they should be) the main-loop can >> never come to update the log and find time has gone backwards. >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> --- >> cpus.c | 6 ++---- >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> diff --git a/cpus.c b/cpus.c >> index ff75af449a..63de033cc8 100644 >> --- a/cpus.c >> +++ b/cpus.c >> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu) >> >> int64_t cpu_get_icount_raw(void) >> { >> - int64_t icount; >> CPUState *cpu = current_cpu; >> >> - icount = atomic_read(&timers_state.qemu_icount); >> if (cpu && cpu->running) { >> if (!cpu->can_do_io) { >> fprintf(stderr, "Bad icount read\n"); >> exit(1); >> } >> /* Take into account what has run */ >> - icount += cpu_get_icount_executed(cpu); >> + cpu_update_icount(cpu); >> } >> - return icount; >> + return atomic_read(&timers_state.qemu_icount); >> } >> >> /* Return the virtual CPU time, based on the instruction counter. */ >> > > Maybe the update should be done in gen_io_start instead. There > shouldn't be any interference between vCPU and I/O threads except > between gen_io_start and gen_io_end. I'm not sure I follow. gen_io_start is a translation time thing. At least here we ensure we update whenever the value is read. -- Alex Bennée
----- Original Message ----- > From: "Alex Bennée" <alex.bennee@linaro.org> > To: "Paolo Bonzini" <pbonzini@redhat.com> > Cc: dovgaluk@ispras.ru, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>, qemu-devel@nongnu.org, > mttcg@greensocs.com, "fred konrad" <fred.konrad@greensocs.com>, "a rigo" <a.rigo@virtualopensystems.com>, > cota@braap.org, "bobby prani" <bobby.prani@gmail.com>, nikunj@linux.vnet.ibm.com, "Peter Crosthwaite" > <crosthwaite.peter@gmail.com> > Sent: Friday, April 7, 2017 7:35:29 PM > Subject: Re: [PATCH v2 11/12] cpus: call cpu_update_icount on read > > > Paolo Bonzini <pbonzini@redhat.com> writes: > > > On 05/04/2017 15:25, Alex Bennée wrote: > >> This ensures each time the vCPU thread reads the icount we update the > >> master timer_state.qemu_icount field. This way as long as updates are > >> in BQL protected sections (which they should be) the main-loop can > >> never come to update the log and find time has gone backwards. > >> > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > >> --- > >> cpus.c | 6 ++---- > >> 1 file changed, 2 insertions(+), 4 deletions(-) > >> > >> diff --git a/cpus.c b/cpus.c > >> index ff75af449a..63de033cc8 100644 > >> --- a/cpus.c > >> +++ b/cpus.c > >> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu) > >> > >> int64_t cpu_get_icount_raw(void) > >> { > >> - int64_t icount; > >> CPUState *cpu = current_cpu; > >> > >> - icount = atomic_read(&timers_state.qemu_icount); > >> if (cpu && cpu->running) { > >> if (!cpu->can_do_io) { > >> fprintf(stderr, "Bad icount read\n"); > >> exit(1); > >> } > >> /* Take into account what has run */ > >> - icount += cpu_get_icount_executed(cpu); > >> + cpu_update_icount(cpu); > >> } > >> - return icount; > >> + return atomic_read(&timers_state.qemu_icount); > >> } > >> > >> /* Return the virtual CPU time, based on the instruction counter. */ > >> > > > > Maybe the update should be done in gen_io_start instead. There > > shouldn't be any interference between vCPU and I/O threads except > > between gen_io_start and gen_io_end. > > I'm not sure I follow. gen_io_start is a translation time thing. At > least here we ensure we update whenever the value is read. Sorry, I meant we should generate TCG opcodes for the translation in gen_io_start. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > ----- Original Message ----- >> From: "Alex Bennée" <alex.bennee@linaro.org> >> To: "Paolo Bonzini" <pbonzini@redhat.com> >> Cc: dovgaluk@ispras.ru, rth@twiddle.net, "peter maydell" <peter.maydell@linaro.org>, qemu-devel@nongnu.org, >> mttcg@greensocs.com, "fred konrad" <fred.konrad@greensocs.com>, "a rigo" <a.rigo@virtualopensystems.com>, >> cota@braap.org, "bobby prani" <bobby.prani@gmail.com>, nikunj@linux.vnet.ibm.com, "Peter Crosthwaite" >> <crosthwaite.peter@gmail.com> >> Sent: Friday, April 7, 2017 7:35:29 PM >> Subject: Re: [PATCH v2 11/12] cpus: call cpu_update_icount on read >> >> >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >> > On 05/04/2017 15:25, Alex Bennée wrote: >> >> This ensures each time the vCPU thread reads the icount we update the >> >> master timer_state.qemu_icount field. This way as long as updates are >> >> in BQL protected sections (which they should be) the main-loop can >> >> never come to update the log and find time has gone backwards. >> >> >> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> >> --- >> >> cpus.c | 6 ++---- >> >> 1 file changed, 2 insertions(+), 4 deletions(-) >> >> >> >> diff --git a/cpus.c b/cpus.c >> >> index ff75af449a..63de033cc8 100644 >> >> --- a/cpus.c >> >> +++ b/cpus.c >> >> @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu) >> >> >> >> int64_t cpu_get_icount_raw(void) >> >> { >> >> - int64_t icount; >> >> CPUState *cpu = current_cpu; >> >> >> >> - icount = atomic_read(&timers_state.qemu_icount); >> >> if (cpu && cpu->running) { >> >> if (!cpu->can_do_io) { >> >> fprintf(stderr, "Bad icount read\n"); >> >> exit(1); >> >> } >> >> /* Take into account what has run */ >> >> - icount += cpu_get_icount_executed(cpu); >> >> + cpu_update_icount(cpu); >> >> } >> >> - return icount; >> >> + return atomic_read(&timers_state.qemu_icount); >> >> } >> >> >> >> /* Return the virtual CPU time, based on the instruction counter. */ >> >> >> > >> > Maybe the update should be done in gen_io_start instead. There >> > shouldn't be any interference between vCPU and I/O threads except >> > between gen_io_start and gen_io_end. >> >> I'm not sure I follow. gen_io_start is a translation time thing. At >> least here we ensure we update whenever the value is read. > > Sorry, I meant we should generate TCG opcodes for the translation in > gen_io_start. Ahh OK. I think this is 2.10 stuff though right? > > Paolo -- Alex Bennée
On 04/07/2017 06:14 AM, Alex Bennée wrote: >> Sorry, I meant we should generate TCG opcodes for the translation in >> gen_io_start. > Ahh OK. I think this is 2.10 stuff though right? > Definitely. r~
diff --git a/cpus.c b/cpus.c index ff75af449a..63de033cc8 100644 --- a/cpus.c +++ b/cpus.c @@ -246,19 +246,17 @@ void cpu_update_icount(CPUState *cpu) int64_t cpu_get_icount_raw(void) { - int64_t icount; CPUState *cpu = current_cpu; - icount = atomic_read(&timers_state.qemu_icount); if (cpu && cpu->running) { if (!cpu->can_do_io) { fprintf(stderr, "Bad icount read\n"); exit(1); } /* Take into account what has run */ - icount += cpu_get_icount_executed(cpu); + cpu_update_icount(cpu); } - return icount; + return atomic_read(&timers_state.qemu_icount); } /* Return the virtual CPU time, based on the instruction counter. */
This ensures each time the vCPU thread reads the icount we update the master timer_state.qemu_icount field. This way as long as updates are in BQL protected sections (which they should be) the main-loop can never come to update the log and find time has gone backwards. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> --- cpus.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) -- 2.11.0