Re: [patch 1/3] drivers/cpuidle: add cpuidle-haltpoll driver

From: Peter Zijlstra <peterz@infradead.org>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: kvm-devel <kvm@vger.kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim KrÄ?mÃ¡Å?" <rkrcmar@redhat.com>,
	"Andrea Arcangeli" <aarcange@redhat.com>,
	"Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
	"Wanpeng Li" <kernellwp@gmail.com>,
	"Konrad Rzeszutek Wilk" <konrad.wilk@oracle.com>,
	"Raslan KarimAllah" <karahmed@amazon.de>,
	"Boris Ostrovsky" <boris.ostrovsky@oracle.com>,
	"Ankur Arora" <ankur.a.arora@oracle.com>,
	"Christian Borntraeger" <borntraeger@de.ibm.com>
Subject: Re: [patch 1/3] drivers/cpuidle: add cpuidle-haltpoll driver
Date: Thu, 6 Jun 2019 21:03:23 +0200	[thread overview]
Message-ID: <20190606190323.GM3419@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20190603225254.212931277@amt.cnet>

On Mon, Jun 03, 2019 at 07:52:43PM -0300, Marcelo Tosatti wrote:
> +static int haltpoll_enter_idle(struct cpuidle_device *dev,
> +			  struct cpuidle_driver *drv, int index)
> +{
> +	int do_halt = 0;
> +	unsigned int *cpu_halt_poll_ns;
> +	ktime_t start, now;
> +	int cpu = smp_processor_id();
> +
> +	cpu_halt_poll_ns = per_cpu_ptr(&halt_poll_ns, cpu);
> +
> +	/* No polling */
> +	if (guest_halt_poll_ns == 0) {
> +		if (current_clr_polling_and_test()) {
> +			local_irq_enable();
> +			return index;
> +		}
> +		default_idle();
> +		return index;
> +	}
> +
> +	local_irq_enable();
> +
> +	now = start = ktime_get();
> +	if (!current_set_polling_and_test()) {
> +		ktime_t end_spin;
> +
> +		end_spin = ktime_add_ns(now, *cpu_halt_poll_ns);
> +
> +		while (!need_resched()) {
> +			cpu_relax();
> +			now = ktime_get();
> +
> +			if (!ktime_before(now, end_spin)) {
> +				do_halt = 1;
> +				break;
> +			}
> +		}
> +	}
> +
> +	if (do_halt) {
> +		u64 block_ns;
> +
> +		/*
> +		 * No events while busy spin window passed,
> +		 * halt.
> +		 */
> +		local_irq_disable();
> +		if (current_clr_polling_and_test()) {
> +			local_irq_enable();
> +			return index;
> +		}
> +		default_idle();
> +		block_ns = ktime_to_ns(ktime_sub(ktime_get(), start));
> +		adjust_haltpoll_ns(block_ns, cpu_halt_poll_ns);
> +	} else {
> +		u64 block_ns = ktime_to_ns(ktime_sub(now, start));
> +
> +		trace_cpuidle_haltpoll_success(*cpu_halt_poll_ns, block_ns);
> +		current_clr_polling();
> +	}
> +
> +	return index;
> +}

You might want to look at using sched_clock() here instead of
ktime_get(). ktime_get() can get _very_ expensive when it drops back to
HPET or things like that, where sched_clock() will always keep using
TSC, even when it is not globally synchronized.

(and since this code runs with preemption disabled, we don't care about
the clock being globally sane)

So something like this:

	start = sched_clock();
	if (current_set_polling_and_test()) {
		local_irq_enable();
		goto out;
	}

	local_irq_enable();
	for (;;) {
		if (need_resched()) {
			current_clr_polling();
			trace_..();
			goto out;
		}

		now = sched_clock();
		if (now - start > cpu_halt_poll_ns)
			break;

		cpu_relax();
	}

	local_irq_disable();
	if (current_clr_polling_and_test()) {
		local_irq_enable();
		goto out;
	}

	default_idle();
	block_ns = sched_clock() - start;
	adjust_haltpoll_ns(block_ns, cpu_halt_poll_ns);

out:
	return index;