Re: [PATCH 6/7] psi: pressure stall information for CPU, memory, and IO

Peter Zijlstra
2018-05-09T10:04:55Z

On Mon, May 07, 2018 at 05:01:34PM -0400, Johannes Weiner wrote:
> +static void psi_clock(struct work_struct *work)
> +{
> + u64 some[NR_PSI_RESOURCES] = { 0, };
> + u64 full[NR_PSI_RESOURCES] = { 0, };
> + unsigned long nonidle_total = 0;
> + unsigned long missed_periods;
> + struct delayed_work *dwork;
> + struct psi_group *group;
> + unsigned long expires;
> + int cpu;
> + int r;
> +
> + dwork = to_delayed_work(work);
> + group = container_of(dwork, struct psi_group, clock_work);
> +
> + /*
> + * Calculate the sampling period. The clock might have been
> + * stopped for a while.
> + */
> + expires = group->period_expires;
> + missed_periods = (jiffies - expires) / MY_LOAD_FREQ;
> + group->period_expires = expires + ((1 + missed_periods) * MY_LOAD_FREQ);
> +
> + /*
> + * Aggregate the per-cpu state into a global state. Each CPU
> + * is weighted by its non-idle time in the sampling period.
> + */
> + for_each_online_cpu(cpu) {

Typically when using online CPU state, you also need hotplug notifiers
to deal with changes in the online set.

You also typically need something like cpus_read_lock() around an
iteration of online CPUs, to avoid the set changing while you're poking
at them.

The lack for neither is evident or explained.

> + struct psi_group_cpu *groupc = per_cpu_ptr(group->cpus, cpu);
> + unsigned long nonidle;
> +
> + nonidle = nsecs_to_jiffies(groupc->nonidle_time);
> + groupc->nonidle_time = 0;
> + nonidle_total += nonidle;
> +
> + for (r = 0; r < NR_PSI_RESOURCES; r++) {
> + struct psi_resource *res = &groupc->res[r];
> +
> + some[r] += (res->times[0] + res->times[1]) * nonidle;
> + full[r] += res->times[1] * nonidle;
> +
> + /* It's racy, but we can tolerate some error */
> + res->times[0] = 0;
> + res->times[1] = 0;
> + }
> + }
> +
> + for (r = 0; r < NR_PSI_RESOURCES; r++) {
> + /* Finish the weighted aggregation */
> + some[r] /= max(nonidle_total, 1UL);
> + full[r] /= max(nonidle_total, 1UL);
> +
> + /* Accumulate stall time */
> + group->some[r] += some[r];
> + group->full[r] += full[r];
> +
> + /* Calculate recent pressure averages */
> + calc_avgs(group->avg_some[r], some[r], missed_periods);
> + calc_avgs(group->avg_full[r], full[r], missed_periods);
> + }
> +
> + /* Keep the clock ticking only when there is action */
> + if (nonidle_total)
> + schedule_delayed_work(dwork, MY_LOAD_FREQ);
> +}