Re: [PATCH v2] accel/tcg/tcg-accel-ops-rr: ensure fairness with icount

[Jamie Iles]

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