[RFC PATCH 0/2] s390: Remove vtimer infrastructure

[RFC PATCH 0/2] s390: Remove vtimer infrastructure

[Heiko Carstens]

s390's virtual timer infrastructure has only a single user and the
semantics of the interface are not straightforward: a virtual timer
fires if the sum of elapsed CPU time over all CPUs exceeds a specified
value.

This is suboptimal since the number of active CPUs defines how
frequently the timer fires. With many CPUs and short intervals this can
be very frequent. Furthermore the required accounting in the vtime code
uses a single 64-bit value to accumulate elapsed CPU time. In busy
systems this can lead to cache line bouncing when the value is updated
concurrently on several CPUs; which is the reason why other accounting
mechanisms use per-CPU data structures for this purpose.

In practice this is not a problem with the appldata default interval
of 10 seconds, however the interval is configurable via sysctl with a
minimum value of 1ms.

Given that there is only a single user, emulate the virtual timer in
appldata using delayed work: the work is scheduled for the minimum
possible wall-clock time until the configured CPU-time interval elapses
(remaining CPU time / number of online CPUs), with a minimum delay of
100ms. The work then reads per-CPU statistics of all online CPUs to
check whether enough CPU time has elapsed, and if so runs the
registered callbacks.

The calculation is more expensive, but avoids potential cache line
bouncing problem. In addition it allows to remove the entire vtimer
infrastructure code with a subsequent patch.

Thanks,
Heiko

Heiko Carstens (2):
  s390/appldata: Emulate virtual timer with delayed work
  s390/vtime: Remove vtimer infrastructure

 arch/s390/appldata/appldata_base.c | 150 +++++++++++---------
 arch/s390/include/asm/vtime.h      |   2 +
 arch/s390/include/asm/vtimer.h     |  30 ----
 arch/s390/kernel/process.c         |   1 -
 arch/s390/kernel/smp.c             |   2 +-
 arch/s390/kernel/time.c            |   2 +-
 arch/s390/kernel/vtime.c           | 212 ++---------------------------
 7 files changed, 94 insertions(+), 305 deletions(-)
 delete mode 100644 arch/s390/include/asm/vtimer.h

-- 
2.53.0

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

[Heiko Carstens]

Emulate the virtual timer in appldata using delayed work: the work is
scheduled for the minimum possible wall-clock time until the
configured CPU-time interval elapses (remaining CPU time / number of
online CPUs), with a minimum delay of 100ms. The work then reads
per-CPU statistics of all online CPUs to check whether enough CPU time
has elapsed, and if so runs the registered callbacks.

This slightly more expensive, but allows to subsequently remove the
entire vtimer infrastructure.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/appldata/appldata_base.c | 150 ++++++++++++++++-------------
 1 file changed, 82 insertions(+), 68 deletions(-)

diff --git a/arch/s390/appldata/appldata_base.c b/arch/s390/appldata/appldata_base.c
index 9cba4633c3f3..aca747c98f1b 100644
--- a/arch/s390/appldata/appldata_base.c
+++ b/arch/s390/appldata/appldata_base.c
@@ -28,19 +28,14 @@
 #include <linux/workqueue.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/kernel_stat.h>
 #include <asm/appldata.h>
-#include <asm/vtimer.h>
 #include <asm/smp.h>
 
 #include "appldata.h"
 
-
-#define APPLDATA_CPU_INTERVAL	10000		/* default (CPU) time for
-						   sampling interval in
-						   milliseconds */
-
-#define TOD_MICRO	0x01000			/* nr. of TOD clock units
-						   for 1 microsecond */
+/* Default CPU time for sampling interval */
+#define APPLDATA_CPU_INTERVAL	(10 * NSEC_PER_SEC)
 
 /*
  * /proc entries (sysctl)
@@ -64,22 +59,14 @@ static const struct ctl_table appldata_table[] = {
 	},
 };
 
-/*
- * Timer
- */
-static struct vtimer_list appldata_timer;
+static void appldata_work_fn(struct work_struct *work);
+static DECLARE_DELAYED_WORK(appldata_work, appldata_work_fn);
 
-static DEFINE_SPINLOCK(appldata_timer_lock);
-static int appldata_interval = APPLDATA_CPU_INTERVAL;
+static DEFINE_MUTEX(appldata_timer_lock);
+static u64 appldata_interval = APPLDATA_CPU_INTERVAL;
 static int appldata_timer_active;
 
-/*
- * Work queue
- */
-static struct workqueue_struct *appldata_wq;
-static void appldata_work_fn(struct work_struct *work);
-static DECLARE_WORK(appldata_work, appldata_work_fn);
-
+static u64 appldata_cputime_start;
 
 /*
  * Ops list
@@ -89,34 +76,68 @@ static LIST_HEAD(appldata_ops_list);
 
 
 /*************************** timer, work, DIAG *******************************/
-/*
- * appldata_timer_function()
- *
- * schedule work and reschedule timer
- */
-static void appldata_timer_function(unsigned long data)
+static u64 appldata_total_cpu_time_ns(void)
 {
-	queue_work(appldata_wq, (struct work_struct *) data);
+	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;
+}
+
+static void appldata_schedule_work(u64 remaining)
+{
+	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);
 }
 
 /*
  * appldata_work_fn()
  *
- * call data gathering function for each (active) module
+ * Check whether the CPU-time interval has elapsed. If yes, run the
+ * data-gathering callbacks and reschedule for the next full interval.
+ * If no, only reschedule for the remaining time.
  */
 static void appldata_work_fn(struct work_struct *work)
 {
-	struct list_head *lh;
+	u64 now, elapsed, interval;
 	struct appldata_ops *ops;
+	struct list_head *lh;
 
-	mutex_lock(&appldata_ops_mutex);
+	now = appldata_total_cpu_time_ns();
+	scoped_guard(mutex, &appldata_timer_lock) {
+		if (!appldata_timer_active)
+			return;
+		interval = appldata_interval;
+		elapsed = now - appldata_cputime_start;
+		if (elapsed < interval) {
+			appldata_schedule_work(interval - elapsed);
+			return;
+		}
+		appldata_cputime_start = now;
+		appldata_schedule_work(interval);
+	}
+	guard(mutex)(&appldata_ops_mutex);
 	list_for_each(lh, &appldata_ops_list) {
 		ops = list_entry(lh, struct appldata_ops, list);
-		if (ops->active == 1) {
+		if (ops->active)
 			ops->callback(ops->data);
-		}
 	}
-	mutex_unlock(&appldata_ops_mutex);
 }
 
 static struct appldata_product_id appldata_id = {
@@ -162,33 +183,37 @@ int appldata_diag(char record_nr, u16 function, unsigned long buffer,
 #define APPLDATA_MOD_TIMER	2
 
 /*
- * __appldata_vtimer_setup()
+ * appldata_work_setup()
  *
- * Add, delete or modify virtual timers on all online cpus.
- * The caller needs to get the appldata_timer_lock spinlock.
+ * Add, delete or modify the appldata delayed work.
  */
-static void __appldata_vtimer_setup(int cmd)
+static void appldata_work_setup(int cmd)
 {
-	u64 timer_interval = (u64) appldata_interval * 1000 * TOD_MICRO;
+	u64 now, elapsed, remaining;
 
 	switch (cmd) {
 	case APPLDATA_ADD_TIMER:
+		lockdep_assert_held(&appldata_timer_lock);
 		if (appldata_timer_active)
 			break;
-		appldata_timer.expires = timer_interval;
-		add_virt_timer_periodic(&appldata_timer);
+		appldata_cputime_start = appldata_total_cpu_time_ns();
+		appldata_schedule_work(appldata_interval);
 		appldata_timer_active = 1;
 		break;
 	case APPLDATA_DEL_TIMER:
-		del_virt_timer(&appldata_timer);
-		if (!appldata_timer_active)
-			break;
+		lockdep_assert_not_held(&appldata_timer_lock);
 		appldata_timer_active = 0;
+		cancel_delayed_work_sync(&appldata_work);
 		break;
 	case APPLDATA_MOD_TIMER:
+		lockdep_assert_held(&appldata_timer_lock);
 		if (!appldata_timer_active)
 			break;
-		mod_virt_timer_periodic(&appldata_timer, timer_interval);
+		now = appldata_total_cpu_time_ns();
+		elapsed = now - appldata_cputime_start;
+		remaining = (elapsed < appldata_interval) ? (appldata_interval - elapsed) : 1;
+		appldata_schedule_work(remaining);
+		break;
 	}
 }
 
@@ -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;
 }
 
@@ -234,11 +259,11 @@ static int
 appldata_interval_handler(const struct ctl_table *ctl, int write,
 			   void *buffer, size_t *lenp, loff_t *ppos)
 {
-	int interval = appldata_interval;
+	int interval_ms = appldata_interval / NSEC_PER_MSEC;
 	int rc;
 	struct ctl_table ctl_entry = {
 		.procname	= ctl->procname,
-		.data		= &interval,
+		.data		= &interval_ms,
 		.maxlen		= sizeof(int),
 		.extra1		= SYSCTL_ONE,
 	};
@@ -247,10 +272,10 @@ appldata_interval_handler(const struct ctl_table *ctl, int write,
 	if (rc < 0 || !write)
 		return rc;
 
-	spin_lock(&appldata_timer_lock);
-	appldata_interval = interval;
-	__appldata_vtimer_setup(APPLDATA_MOD_TIMER);
-	spin_unlock(&appldata_timer_lock);
+	scoped_guard(mutex, &appldata_timer_lock) {
+		appldata_interval = interval_ms * NSEC_PER_MSEC;
+		appldata_work_setup(APPLDATA_MOD_TIMER);
+	}
 	return 0;
 }
 
@@ -392,19 +417,8 @@ void appldata_unregister_ops(struct appldata_ops *ops)
 
 /******************************* init / exit *********************************/
 
-/*
- * appldata_init()
- *
- * init timer, register /proc entries
- */
 static int __init appldata_init(void)
 {
-	init_virt_timer(&appldata_timer);
-	appldata_timer.function = appldata_timer_function;
-	appldata_timer.data = (unsigned long) &appldata_work;
-	appldata_wq = alloc_ordered_workqueue("appldata", 0);
-	if (!appldata_wq)
-		return -ENOMEM;
 	register_sysctl(appldata_proc_name, appldata_table);
 	return 0;
 }
-- 
2.53.0

[RFC PATCH 2/2] s390/vtime: Remove vtimer infrastructure

[Heiko Carstens]

The only user is gone - remove the vtimer infrastructure.

Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
---
 arch/s390/include/asm/vtime.h  |   2 +
 arch/s390/include/asm/vtimer.h |  30 -----
 arch/s390/kernel/process.c     |   1 -
 arch/s390/kernel/smp.c         |   2 +-
 arch/s390/kernel/time.c        |   2 +-
 arch/s390/kernel/vtime.c       | 212 ++-------------------------------
 6 files changed, 12 insertions(+), 237 deletions(-)
 delete mode 100644 arch/s390/include/asm/vtimer.h

diff --git a/arch/s390/include/asm/vtime.h b/arch/s390/include/asm/vtime.h
index da116a93d3b6..22e494b22192 100644
--- a/arch/s390/include/asm/vtime.h
+++ b/arch/s390/include/asm/vtime.h
@@ -8,6 +8,8 @@
 
 DECLARE_PER_CPU(u64, mt_cycles[8]);
 
+void vtime_init(void);
+
 static inline void update_timer_sys(void)
 {
 	struct lowcore *lc = get_lowcore();
diff --git a/arch/s390/include/asm/vtimer.h b/arch/s390/include/asm/vtimer.h
deleted file mode 100644
index e601adaa6320..000000000000
--- a/arch/s390/include/asm/vtimer.h
+++ /dev/null
@@ -1,30 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-/*
- *  Copyright IBM Corp. 2003, 2012
- *  Virtual CPU timer
- *
- *  Author(s): Jan Glauber <jan.glauber@de.ibm.com>
- */
-
-#ifndef _ASM_S390_TIMER_H
-#define _ASM_S390_TIMER_H
-
-#define VTIMER_MAX_SLICE (0x7fffffffffffffffULL)
-
-struct vtimer_list {
-	struct list_head entry;
-	u64 expires;
-	u64 interval;
-	void (*function)(unsigned long);
-	unsigned long data;
-};
-
-extern void init_virt_timer(struct vtimer_list *timer);
-extern void add_virt_timer(struct vtimer_list *timer);
-extern void add_virt_timer_periodic(struct vtimer_list *timer);
-extern int mod_virt_timer(struct vtimer_list *timer, u64 expires);
-extern int mod_virt_timer_periodic(struct vtimer_list *timer, u64 expires);
-extern int del_virt_timer(struct vtimer_list *timer);
-extern void vtime_init(void);
-
-#endif /* _ASM_S390_TIMER_H */
diff --git a/arch/s390/kernel/process.c b/arch/s390/kernel/process.c
index 416650ae4871..4c73d2528fcb 100644
--- a/arch/s390/kernel/process.c
+++ b/arch/s390/kernel/process.c
@@ -35,7 +35,6 @@
 #include <asm/cpu_mf.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
-#include <asm/vtimer.h>
 #include <asm/exec.h>
 #include <asm/fpu.h>
 #include <asm/irq.h>
diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
index 32499cad86f0..f7c262a17c03 100644
--- a/arch/s390/kernel/smp.c
+++ b/arch/s390/kernel/smp.c
@@ -48,7 +48,7 @@
 #include <asm/setup.h>
 #include <asm/irq.h>
 #include <asm/tlbflush.h>
-#include <asm/vtimer.h>
+#include <asm/vtime.h>
 #include <asm/abs_lowcore.h>
 #include <asm/sclp.h>
 #include <asm/debug.h>
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 2b989bebd220..dd8a88cc9c4b 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -48,9 +48,9 @@
 #include <asm/vdso.h>
 #include <asm/irq.h>
 #include <asm/irq_regs.h>
-#include <asm/vtimer.h>
 #include <asm/stp.h>
 #include <asm/cio.h>
+#include <asm/vtime.h>
 #include "entry.h"
 
 union tod_clock __bootdata_preserved(tod_clock_base);
diff --git a/arch/s390/kernel/vtime.c b/arch/s390/kernel/vtime.c
index efcbf406f03e..d9fd39c5732e 100644
--- a/arch/s390/kernel/vtime.c
+++ b/arch/s390/kernel/vtime.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- *    Virtual cpu timer based timer functions.
+ *    Virtual CPU time accounting
  *
  *    Copyright IBM Corp. 2004, 2012
  *    Author(s): Jan Glauber <jan.glauber@de.ibm.com>
@@ -14,7 +14,6 @@
 #include <linux/time.h>
 #include <asm/alternative.h>
 #include <asm/cputime.h>
-#include <asm/vtimer.h>
 #include <asm/vtime.h>
 #include <asm/cpu_mf.h>
 #include <asm/idle.h>
@@ -22,19 +21,14 @@
 
 #include "entry.h"
 
-static void virt_timer_expire(void);
-
-static LIST_HEAD(virt_timer_list);
-static DEFINE_SPINLOCK(virt_timer_lock);
-static atomic64_t virt_timer_current;
-static atomic64_t virt_timer_elapsed;
+#define CPU_TIMER_MAX	0x7fffffffffffffffUL
 
 DEFINE_PER_CPU(u64, mt_cycles[8]);
 static DEFINE_PER_CPU(u64, mt_scaling_mult) = { 1 };
 static DEFINE_PER_CPU(u64, mt_scaling_div) = { 1 };
 static DEFINE_PER_CPU(unsigned long, mt_scaling_jiffies);
 
-static inline void set_vtimer(u64 expires)
+static inline void cpu_timer_init(u64 value)
 {
 	struct lowcore *lc = get_lowcore();
 	u64 timer;
@@ -42,18 +36,9 @@ static inline void set_vtimer(u64 expires)
 	asm volatile(
 		"	stpt	%0\n"	/* Store current cpu timer value */
 		"	spt	%1"	/* Set new value imm. afterwards */
-		: "=Q" (timer) : "Q" (expires));
+		: "=Q" (timer) : "Q" (value));
 	lc->system_timer += lc->last_update_timer - timer;
-	lc->last_update_timer = expires;
-}
-
-static inline int virt_timer_forward(u64 elapsed)
-{
-	lockdep_assert_irqs_disabled();
-	if (list_empty(&virt_timer_list))
-		return 0;
-	elapsed = atomic64_add_return(elapsed, &virt_timer_elapsed);
-	return elapsed >= atomic64_read(&virt_timer_current);
+	lc->last_update_timer = value;
 }
 
 static void update_mt_scaling(void)
@@ -125,7 +110,7 @@ static inline void vtime_reset_last_update(struct lowcore *lc)
  * Update process times based on virtual cpu times stored by entry.S
  * to the lowcore fields user_timer, system_timer & steal_clock.
  */
-static int do_account_vtime(struct task_struct *tsk)
+static void do_account_vtime(struct task_struct *tsk)
 {
 	u64 timer, clock, user, guest, system, hardirq, softirq;
 	struct lowcore *lc = get_lowcore();
@@ -172,8 +157,6 @@ static int do_account_vtime(struct task_struct *tsk)
 		account_system_index_scaled(tsk, hardirq, CPUTIME_IRQ);
 	if (softirq)
 		account_system_index_scaled(tsk, softirq, CPUTIME_SOFTIRQ);
-
-	return virt_timer_forward(user + guest + system + hardirq + softirq);
 }
 
 void vtime_task_switch(struct task_struct *prev)
@@ -203,8 +186,7 @@ void vtime_flush(struct task_struct *tsk)
 	struct lowcore *lc = get_lowcore();
 	u64 steal, avg_steal;
 
-	if (do_account_vtime(tsk))
-		virt_timer_expire();
+	do_account_vtime(tsk);
 
 	steal = lc->steal_timer;
 	avg_steal = lc->avg_steal_timer;
@@ -247,187 +229,9 @@ void vtime_account_hardirq(struct task_struct *tsk)
 	get_lowcore()->hardirq_timer += vtime_delta();
 }
 
-/*
- * Sorted add to a list. List is linear searched until first bigger
- * element is found.
- */
-static void list_add_sorted(struct vtimer_list *timer, struct list_head *head)
-{
-	struct vtimer_list *tmp;
-
-	list_for_each_entry(tmp, head, entry) {
-		if (tmp->expires > timer->expires) {
-			list_add_tail(&timer->entry, &tmp->entry);
-			return;
-		}
-	}
-	list_add_tail(&timer->entry, head);
-}
-
-/*
- * Handler for expired virtual CPU timer.
- */
-static void virt_timer_expire(void)
-{
-	struct vtimer_list *timer, *tmp;
-	unsigned long elapsed;
-	LIST_HEAD(cb_list);
-
-	/* walk timer list, fire all expired timers */
-	spin_lock(&virt_timer_lock);
-	elapsed = atomic64_read(&virt_timer_elapsed);
-	list_for_each_entry_safe(timer, tmp, &virt_timer_list, entry) {
-		if (timer->expires < elapsed)
-			/* move expired timer to the callback queue */
-			list_move_tail(&timer->entry, &cb_list);
-		else
-			timer->expires -= elapsed;
-	}
-	if (!list_empty(&virt_timer_list)) {
-		timer = list_first_entry(&virt_timer_list,
-					 struct vtimer_list, entry);
-		atomic64_set(&virt_timer_current, timer->expires);
-	}
-	atomic64_sub(elapsed, &virt_timer_elapsed);
-	spin_unlock(&virt_timer_lock);
-
-	/* Do callbacks and recharge periodic timers */
-	list_for_each_entry_safe(timer, tmp, &cb_list, entry) {
-		list_del_init(&timer->entry);
-		timer->function(timer->data);
-		if (timer->interval) {
-			/* Recharge interval timer */
-			timer->expires = timer->interval +
-				atomic64_read(&virt_timer_elapsed);
-			spin_lock(&virt_timer_lock);
-			list_add_sorted(timer, &virt_timer_list);
-			spin_unlock(&virt_timer_lock);
-		}
-	}
-}
-
-void init_virt_timer(struct vtimer_list *timer)
-{
-	timer->function = NULL;
-	INIT_LIST_HEAD(&timer->entry);
-}
-EXPORT_SYMBOL(init_virt_timer);
-
-static inline int vtimer_pending(struct vtimer_list *timer)
-{
-	return !list_empty(&timer->entry);
-}
-
-static void internal_add_vtimer(struct vtimer_list *timer)
-{
-	if (list_empty(&virt_timer_list)) {
-		/* First timer, just program it. */
-		atomic64_set(&virt_timer_current, timer->expires);
-		atomic64_set(&virt_timer_elapsed, 0);
-		list_add(&timer->entry, &virt_timer_list);
-	} else {
-		/* Update timer against current base. */
-		timer->expires += atomic64_read(&virt_timer_elapsed);
-		if (likely((s64) timer->expires <
-			   (s64) atomic64_read(&virt_timer_current)))
-			/* The new timer expires before the current timer. */
-			atomic64_set(&virt_timer_current, timer->expires);
-		/* Insert new timer into the list. */
-		list_add_sorted(timer, &virt_timer_list);
-	}
-}
-
-static void __add_vtimer(struct vtimer_list *timer, int periodic)
-{
-	unsigned long flags;
-
-	timer->interval = periodic ? timer->expires : 0;
-	spin_lock_irqsave(&virt_timer_lock, flags);
-	internal_add_vtimer(timer);
-	spin_unlock_irqrestore(&virt_timer_lock, flags);
-}
-
-/*
- * add_virt_timer - add a oneshot virtual CPU timer
- */
-void add_virt_timer(struct vtimer_list *timer)
-{
-	__add_vtimer(timer, 0);
-}
-EXPORT_SYMBOL(add_virt_timer);
-
-/*
- * add_virt_timer_int - add an interval virtual CPU timer
- */
-void add_virt_timer_periodic(struct vtimer_list *timer)
-{
-	__add_vtimer(timer, 1);
-}
-EXPORT_SYMBOL(add_virt_timer_periodic);
-
-static int __mod_vtimer(struct vtimer_list *timer, u64 expires, int periodic)
-{
-	unsigned long flags;
-	int rc;
-
-	BUG_ON(!timer->function);
-
-	if (timer->expires == expires && vtimer_pending(timer))
-		return 1;
-	spin_lock_irqsave(&virt_timer_lock, flags);
-	rc = vtimer_pending(timer);
-	if (rc)
-		list_del_init(&timer->entry);
-	timer->interval = periodic ? expires : 0;
-	timer->expires = expires;
-	internal_add_vtimer(timer);
-	spin_unlock_irqrestore(&virt_timer_lock, flags);
-	return rc;
-}
-
-/*
- * returns whether it has modified a pending timer (1) or not (0)
- */
-int mod_virt_timer(struct vtimer_list *timer, u64 expires)
-{
-	return __mod_vtimer(timer, expires, 0);
-}
-EXPORT_SYMBOL(mod_virt_timer);
-
-/*
- * returns whether it has modified a pending timer (1) or not (0)
- */
-int mod_virt_timer_periodic(struct vtimer_list *timer, u64 expires)
-{
-	return __mod_vtimer(timer, expires, 1);
-}
-EXPORT_SYMBOL(mod_virt_timer_periodic);
-
-/*
- * Delete a virtual timer.
- *
- * returns whether the deleted timer was pending (1) or not (0)
- */
-int del_virt_timer(struct vtimer_list *timer)
-{
-	unsigned long flags;
-
-	if (!vtimer_pending(timer))
-		return 0;
-	spin_lock_irqsave(&virt_timer_lock, flags);
-	list_del_init(&timer->entry);
-	spin_unlock_irqrestore(&virt_timer_lock, flags);
-	return 1;
-}
-EXPORT_SYMBOL(del_virt_timer);
-
-/*
- * Start the virtual CPU timer on the current CPU.
- */
 void vtime_init(void)
 {
-	/* set initial cpu timer */
-	set_vtimer(VTIMER_MAX_SLICE);
+	cpu_timer_init(CPU_TIMER_MAX);
 	/* Setup initial MT scaling values */
 	if (smp_cpu_mtid) {
 		__this_cpu_write(mt_scaling_jiffies, jiffies);
-- 
2.53.0

Re: [RFC PATCH 2/2] s390/vtime: Remove vtimer infrastructure

[sashiko-bot]

> The only user is gone - remove the vtimer infrastructure.
> 
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[sashiko-bot]

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