On 31-05-18, 20:25, Daniel Lezcano wrote:
> On 29/05/2018 11:31, Viresh Kumar wrote:
> > On 25-05-18, 11:49, Daniel Lezcano wrote:
> >> + /*
> >> + * The last CPU waking up is in charge of setting the timer. If
> >> + * the CPU is hotplugged, the timer will move to another CPU
> >> + * (which may not belong to the same cluster) but that is not a
> >> + * problem as the timer will be set again by another CPU
> >> + * belonging to the cluster. This mechanism is self adaptive.
> >> + */
> >
> > I am afraid that the above comment may not be completely true all the
> > time. For a quad-core platform, it is possible for 3 CPUs (0,1,2) to
> > run this function as soon as the kthread is woken up, but one of the
> > CPUs (3) may be stuck servicing an IRQ, Deadline or RT activity.
> > Because you do atomic_inc() also in this function (above) itself,
> > below decrement may return a true value for the CPU2 and that will
> > restart the hrtimer, while one of the CPUs never got a chance to
> > increment count in the first place.
> >
> > The fix is simple though, do the increment in idle_injection_wakeup()
> > and things should be fine then.
>
> Ok.
>
> >> + if (!atomic_dec_and_test(&ii_dev->count))
> >> + return;
> >> +
> >> + run_duration_ms = atomic_read(&ii_dev->run_duration_ms);
> >> + if (run_duration_ms) {
> >> + hrtimer_start(&ii_dev->timer, ms_to_ktime(run_duration_ms),
> >> + HRTIMER_MODE_REL_PINNED);
> >> + return;
> >> + }
> >> +
> >> + complete(&ii_dev->stop_complete);
> >
> > So you call complete() after hrtimer is potentially restarted. This
> > can happen if idle_injection_stop() is called right after the above
> > atomic_read() has finished :)
> >
> > IOW, this doesn't look safe now as well.
>
> It is safe, we just missed a cycle and the stop will block until the
> next cycle. I did it on purpose and for me it is correct.
Okay, what about this then:
Path A Path B
idle_injection_fn() idle_injection_unregister()
hrtimer_start() idle_injection_stop()
complete()
wait_for_completion()
kfree(ii_dev);
Hrtimer is still used here after
getting freed.
Is this not possible ?
> >> +struct idle_injection_device *idle_injection_register(struct cpumask
> >> *cpumask)
> >> +{
> >> + struct idle_injection_device *ii_dev;
> >> + int cpu, cpu2;
> >> +
> >> + ii_dev = ii_dev_alloc();
> >> + if (!ii_dev)
> >> + return NULL;
> >> +
> >> + cpumask_copy(ii_dev->cpumask, cpumask);
> >> + hrtimer_init(&ii_dev->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> >> + ii_dev->timer.function = idle_injection_wakeup_fn;
> >> +
> >> + for_each_cpu(cpu, ii_dev->cpumask) {
> >> +
> >> + if (per_cpu(idle_injection_device, cpu)) {
> >
> > Maybe add unlikely here ?
>
> For this kind of init function and tight loop, there is no benefit of
> adding the unlikely / likely. I can add it if you want, but it is useless.
Okay.
> >> + pr_err("cpu%d is already registered\n", cpu);
> >> + goto out_rollback_per_cpu;
> >> + }
> >> +
> >> + per_cpu(idle_injection_device, cpu) = ii_dev;
> >> + }
> >> +
> >> + return ii_dev;
> >> +
> >> +out_rollback_per_cpu:
> >> + for_each_cpu(cpu2, ii_dev->cpumask) {
> >> + if (cpu == cpu2)
> >
> > And is this really required? Perhaps this conditional is making it
> > worse and just setting the per-cpu variable forcefully to NULL would
> > be much faster :)
>
> We want undo what was done, setting all variables to NULL resets the
> values from a previous register and we don't want that.
Why will that happen ? We are only iterating over a particular cpumask and not
all possible CPUs. Yes I understand the "undo only what we did" part, but the
conditional is more expensive than that I feel.
--
viresh
