Hi Richard,
On Mon, Apr 24, 2023 at 02:06:18PM +0100, Richard Henderson wrote:
> WARNING: This email originated from outside of Qualcomm. Please be wary of
> any links or attachments, and do not enable macros.
>
> On 4/24/23 12:29, Jamie Iles wrote:
> > +/*
> > + * Calculate the number of CPUs that we will process in a single iteration
> > of
> > + * the main CPU thread loop so that we can fairly distribute the
> > instruction
> > + * count across CPUs.
> > + *
> > + * The CPU count is cached based on the CPU list generation ID to avoid
> > + * iterating the list every time.
> > + */
> > +static int rr_cpu_count(void)
> > +{
> > + static unsigned int last_gen_id = ~0;
> > + static int cpu_count;
> > + CPUState *cpu;
> > +
> > + cpu_list_lock();
> > + if (cpu_list_generation_id_get() != last_gen_id) {
> > + cpu_count = 0;
> > + CPU_FOREACH(cpu) {
> > + ++cpu_count;
> > + }
> > + last_gen_id = cpu_list_generation_id_get();
> > + }
> > + cpu_list_unlock();
> > +
> > + return cpu_count;
> > +}
>
> The read of cpu_count should be in the lock.
>
> (Ideally we'd expose QEMU_LOCK_GUARD(&qemu_cpu_list_lock) which would make
> the read+return
> trivial.)
Sure, can do that, I can add that as a first patch and update other
users.
>
> > /*
> > * In the single-threaded case each vCPU is simulated in turn. If
> > * there is more than a single vCPU we create a simple timer to kick
> > @@ -185,6 +212,9 @@ static void *rr_cpu_thread_fn(void *arg)
> > cpu->exit_request = 1;
> >
> > while (1) {
> > + int cpu_count = rr_cpu_count();
> > + int64_t cpu_budget = INT64_MAX;
> > +
> > qemu_mutex_unlock_iothread();
> > replay_mutex_lock();
> > qemu_mutex_lock_iothread();
> > @@ -197,6 +227,8 @@ static void *rr_cpu_thread_fn(void *arg)
> > * waking up the I/O thread and waiting for completion.
> > */
> > icount_handle_deadline();
> > +
> > + cpu_budget = icount_percpu_budget(cpu_count);
>
> I think you can move the call to rr_cpu_count() here, so that it only happens
> if icount is
> in use.
>
> Why cpu_budget = INT64_MAX as opposed to 0 or 0xdeadbeef? It's not set or
> used except for
> icount_enabled.
That's left over from an earlier version, I can leave it uninitialized.
Thanks,
Jamie
