Re: [RFC PATCH 1/2] s390/appldata: Emulate virtual timer with delayed work

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] sysctl: race in timer deactivation allows permanent stall
- [Medium] appldata: interval sysctl updates ignored due to schedule_delayed_work()
- [Medium] appldata: CPU offlining causes arithmetic underflow and premature execution
--

commit 8e2ea62d851c3c34199ee4aaf1c6b3ad590787f1
Author: Heiko Carstens <hca@linux.ibm.com>

s390/appldata: Emulate virtual timer with delayed work

This commit emulates the virtual timer in appldata using delayed work
to allow for the eventual removal of the vtimer infrastructure. The work is
scheduled for the minimum wall-clock time until the configured CPU-time
interval elapses, subsequently reading per-CPU statistics.

> diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
> index 9cba4633c3f3b..aca747c98f1b0 100644
> --- a/arch/s390/appldata/appldata_base.c
> +++ b/arch/s390/appldata/appldata_base.c

[ ... ]

> +static u64 appldata_total_cpu_time_ns(void)
> +{
> +	u64 total = 0;
> +	int cpu;
> +
> +	for_each_online_cpu(cpu) {
> +		total += kcpustat_cpu(cpu).cpustat[CPUTIME_USER];
> +		total += kcpustat_cpu(cpu).cpustat[CPUTIME_NICE];
> +		total += kcpustat_cpu(cpu).cpustat[CPUTIME_SYSTEM];
> +		total += kcpustat_cpu(cpu).cpustat[CPUTIME_IRQ];
> +		total += kcpustat_cpu(cpu).cpustat[CPUTIME_SOFTIRQ];
> +	}
> +	return total;
> +}

[Severity: Medium]
Is it safe to use for_each_online_cpu() here for a monotonic time counter?

If a CPU goes offline, won't the returned total time unexpectedly shrink?
Later, when calculating the elapsed time:

elapsed = now - appldata_cputime_start;

If now shrinks to be smaller than appldata_cputime_start, elapsed will
underflow to a massive unsigned value. This bypasses the elapsed < interval
check in appldata_work_fn() and causes the callbacks to execute immediately.

[ ... ]

> +static void appldata_schedule_work(u64 remaining)
>  {
> -	queue_work(appldata_wq, (struct work_struct *) data);
> +	unsigned int ncpus = num_online_cpus();
> +	unsigned long delay = HZ / 10;
> +
> +	/*
> +	 * At most ncpus CPUs consume CPU time simultaneously, so the
> +	 * minimum wall-clock time until the remaining CPU time elapses
> +	 * is remaining / ncpus.
> +	 * Make sure the work is not scheduled more than once per 100ms.
> +	 */
> +	delay = max(delay, nsecs_to_jiffies(remaining / ncpus));
> +	schedule_delayed_work(&appldata_work, delay);
>  }

[Severity: Medium]
Does this successfully update the interval if the work is already queued?

When a user updates the interval via /proc/sys/appldata/interval, the sysctl
handler calls appldata_work_setup(APPLDATA_MOD_TIMER), which eventually calls
this function.

Since schedule_delayed_work() is a no-op when a work item is already queued,
it seems the requested interval change is ignored for the current cycle. Would
mod_delayed_work() be required here instead to update the existing timer?

[ ... ]

> @@ -215,12 +240,12 @@ appldata_timer_handler(const struct ctl_table *ctl, int write,
>  	if (rc < 0 || !write)
>  		return rc;
>  
> -	spin_lock(&appldata_timer_lock);
> -	if (timer_active)
> -		__appldata_vtimer_setup(APPLDATA_ADD_TIMER);
> -	else
> -		__appldata_vtimer_setup(APPLDATA_DEL_TIMER);
> -	spin_unlock(&appldata_timer_lock);
> +	if (timer_active) {
> +		scoped_guard(mutex, &appldata_timer_lock)
> +			appldata_work_setup(APPLDATA_ADD_TIMER);
> +	} else {
> +		appldata_work_setup(APPLDATA_DEL_TIMER);
> +	}
>  	return 0;
>  }

[Severity: Medium]
Is there a race condition here since the APPLDATA_DEL_TIMER path does not
take the appldata_timer_lock?

If a sysctl write of 0 (DEL_TIMER) races with a concurrent sysctl write of 1
(ADD_TIMER), they could interleave. If DEL_TIMER executes locklessly and
sets appldata_timer_active = 0 just before ADD_TIMER takes the lock and queues
the work, DEL_TIMER might proceed to unconditionally cancel the newly queued
work.

This would leave the timer in an active = 1 state without any delayed work
queued, permanently breaking appldata gathering.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005145004.156348-1-hca@linux.ibm.com?part=1