hrtimer: Cleanup usage trainwrecks treewide

perf_event_mux_interval_ms_store() tries to apply an updated hrtimer_interval to all possible cpus in a completely unsafe way. The changelog of the offending commit says:

"In the 5th version, we handle the reprogramming of the hrtimer using
hrtimer_forward_now(). That way, we sync up to new timer value quickly (suggested by Jiri Olsa)."

The hrtimer reprogramming is completely unrelated to hrtimer_forward_now(). hrtimer_forward_now() merily forwards the expiry time of the hrtimer past now and that's where things get really bad in this code:

The forward function is called on enqueued timers. That means the update of the expiry time corrupts the ordering of the hrtimer rbtree.

The proper way to update hrtimers on remote cpus is to use a smp function call on all online cpus and perform the update locally by canceling the timer, forwarding and restarting it.

Fixes 62b856397927 "perf: Add sysfs entry to adjust multiplexing interval per PMU"

Signed-off-by Thomas Gleixner <[email protected]>

Cc Stephane Eranian <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc [email protected]

Cc Arnaldo Carvalho de Melo <[email protected]>

---
 kernel/events/core.c |   36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

Index: linux/kernel/events/core.c
===================================================================

kernel/events/core.c +20 -16

@@ -6827,13 +6827,27 @@ perf_event_mux_interval_ms_show(struct d
        return snprintf(page, PAGE_SIZE-1, "%d\n", pmu->hrtimer_interval_ms);
 }
 
+static void perf_event_mux_update_interval(void *info)
+{
+       struct pmu *pmu = info;
+       struct perf_cpu_context *cpuctx = this_cpu_ptr(pmu->pmu_cpu_context);
+       int was_armed = hrtimer_cancel(&cpuctx->hrtimer);
+
+       /* Update the interval and restart the timer with the new interval */
+       cpuctx->hrtimer_interval = ms_to_ktime(pmu->hrtimer_interval_ms);
+       if (was_armed) {
+               hrtimer_forward_now(&cpuctx->hrtimer, cpuctx->hrtimer_interval);
+               hrtimer_start_expires(&cpuctx->hrtimer, HRTIMER_MODE_ABS);
+       }
+}
+
 static ssize_t
 perf_event_mux_interval_ms_store(struct device *dev,
                                 struct device_attribute *attr,
                                 const char *buf, size_t count)
 {
        struct pmu *pmu = dev_get_drvdata(dev);
-       int timer, cpu, ret;
+       int timer, ret;
 
        ret = kstrtoint(buf, 0, &timer);
        if (ret)

@@ -6842,22 +6856,12 @@ perf_event_mux_interval_ms_store(struct
        if (timer < 1)
                return -EINVAL;
 
-       /* same value, noting to do */
-       if (timer == pmu->hrtimer_interval_ms)
-               return count;
-
-       pmu->hrtimer_interval_ms = timer;
-
-       /* update all cpuctx for this PMU */
-       for_each_possible_cpu(cpu) {
-               struct perf_cpu_context *cpuctx;
-               cpuctx = per_cpu_ptr(pmu->pmu_cpu_context, cpu);
-               cpuctx->hrtimer_interval = ns_to_ktime(NSEC_PER_MSEC * timer);
-
-               if (hrtimer_active(&cpuctx->hrtimer))
-                       hrtimer_forward_now(&cpuctx->hrtimer, cpuctx->hrtimer_interval);
+       if (timer != pmu->hrtimer_interval_ms) {
+               get_online_cpus();
+               pmu->hrtimer_interval_ms = timer;
+               on_each_cpu(perf_event_mux_update_interval, pmu, 1);
+               put_online_cpus();
        }
-
        return count;
 }
 static DEVICE_ATTR_RW(perf_event_mux_interval_ms);

The code sets the expiry value of the timer to a relative value and starts it with hrtimer_start_expires. That's fine, but that only works once. The timer is started in relative mode, so the expiry value gets overwritten with the absolut expiry time (now + expiry).

So once the timer expired, a new call to hrtimer_start_expires results in an immidiately expired timer, because the expiry value is already in the past.

Use the proper mechanisms to (re)start the timer in the intended way.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc dingtianhong <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc [email protected]

Quoted 11 lines · On Monday 13 April 2015 21:02:23 Thomas Gleixner wrote:

The code sets the expiry value of the timer to a relative value and starts it with hrtimer_start_expires. That's fine, but that only works once. The timer is started in relative mode, so the expiry value gets overwritten with the absolut expiry time (now + expiry).

So once the timer expired, a new call to hrtimer_start_expires results in an immidiately expired timer, because the expiry value is already in the past.

Use the proper mechanisms to (re)start the timer in the intended way.

Quoted 7 lines

Signed-off-by Thomas Gleixner <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc dingtianhong <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc [email protected]

Thanks a lot for the fix. The mistake was clearly mine, as I had sent a patch to introduce the tx coalesce timer without access to hardware or a way to test that what I did was correct.

There are other known problems in the version of the driver that got merged, and I believe that someone is now looking at them.

What I think we really want here is a way for user space to configure both the minimum and maximum coalesce timer separately rather than assuming half the time is what we want.

Arnd

Quoted 4 lines

drivers/net/ethernet/hisilicon/hip04_eth.c +0 -0

@@ -413,6 +413,15 @@ out:
 	return count;
 }
 

drivers/net/ethernet/hisilicon/hip04_eth.c out: · line 423

Quoted 8 lines

drivers/net/ethernet/hisilicon/hip04_eth.c +8 -0

+static void hip04_start_tx_timer(struct hip04_priv *priv)
+{
+	ktime_t t;
+
+	/* allow timer to fire after half the time at the earliest */
+	t = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
+	hrtimer_start(&priv->tx_coalesce_timer, t, HRTIMER_MODE_REL);
+}

Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code?

Quoted 6 lines

Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code?

Yes, I missed that. Simple fix for this is:

  unsigned long t_ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;

hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(t_ns),

                         t_ns, HRTIMER_MODE_REL);

Quoted 7 lines · On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:

Quoted 6 lines

Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code?

Quoted 6 lines

Yes, I missed that. Simple fix for this is:

  unsigned long t_ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;

hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(t_ns),

                         t_ns, HRTIMER_MODE_REL);

Ah, good. I have to admit that I'd probably make the same mistake again if I was to do this for another driver and you hadn't sent the fix. The hrtimer_set_expires_range() function just looked like it had been designed for the use case I was interested in ;-).

Any idea how to prevent the next person from making the same mistake?

Quoted 15 lines · Arnd Bergmann

Quoted 13 lines · On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:

Quoted 6 lines

Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code?

Yes, I missed that. Simple fix for this is:

  unsigned long t_ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;

hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(t_ns),

                         t_ns, HRTIMER_MODE_REL);

Quoted 6 lines

Ah, good. I have to admit that I'd probably make the same mistake again if I was to do this for another driver and you hadn't sent the fix. The hrtimer_set_expires_range() function just looked like it had been designed for the use case I was interested in ;-).

Any idea how to prevent the next person from making the same mistake?

Yes. Documentation :)

Thanks,

tglx

.

Quoted 15 lines · On Monday 13 April 2015 23:42:03 Thomas Gleixner wrote:

Quoted 13 lines

Quoted 6 lines

Question: this looks to me like it sets both the minimum and maximum time to priv->tx_coalesce_usecs/2, when the intention was to set the minimum to priv->tx_coalesce_usecs/2 and the maximum to priv->tx_coalesce_usecs. Am I missing something subtle here, or did you just misread my original intention from the botched code?

Yes, I missed that. Simple fix for this is:

  unsigned long t_ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;

hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(t_ns),

                         t_ns, HRTIMER_MODE_REL);

Quoted 6 lines

Ah, good. I have to admit that I'd probably make the same mistake again if I was to do this for another driver and you hadn't sent the fix. The hrtimer_set_expires_range() function just looked like it had been designed for the use case I was interested in ;-).

Any idea how to prevent the next person from making the same mistake?

Yes. Documentation :)

The code sets the expiry value of the timer to a relative value and starts it with hrtimer_start_expires. That's fine, but that only works once. The timer is started in relative mode, so the expiry value gets overwritten with the absolut expiry time (now + expiry).

So once the timer expired, a new call to hrtimer_start_expires results in an immidiately expired timer, because the expiry value is already in the past.

Use the proper mechanisms to (re)start the timer in the intended way.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc dingtianhong <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc [email protected]

---
 drivers/net/ethernet/hisilicon/hip04_eth.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Index: linux/drivers/net/ethernet/hisilicon/hip04_eth.c
===================================================================

drivers/net/ethernet/hisilicon/hip04_eth.c +11 -7

@@ -413,6 +413,15 @@ out:
 	return count;
 }
 
+static void hip04_start_tx_timer(struct hip04_priv *priv)
+{
+	unsigned long ns = priv->tx_coalesce_usecs * NSEC_PER_USEC / 2;
+
+	/* allow timer to fire after half the time at the earliest */
+	hrtimer_start_range_ns(&priv->tx_coalesce_timer, ns_to_ktime(ns),
+			       ns, HRTIMER_MODE_REL);
+}
+
 static int hip04_mac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 {
 	struct hip04_priv *priv = netdev_priv(ndev);

@@ -466,8 +475,7 @@ static int hip04_mac_start_xmit(struct s
 		}
 	} else if (!hrtimer_is_queued(&priv->tx_coalesce_timer)) {
 		/* cleanup not pending yet, start a new timer */
-		hrtimer_start_expires(&priv->tx_coalesce_timer,
-				      HRTIMER_MODE_REL);
+		hip04_start_tx_timer(priv);
 	}
 
 	return NETDEV_TX_OK;

@@ -549,7 +557,7 @@ done:
 	/* clean up tx descriptors and start a new timer if necessary */
 	tx_remaining = hip04_tx_reclaim(ndev, false);
 	if (rx < budget && tx_remaining)
-		hrtimer_start_expires(&priv->tx_coalesce_timer, HRTIMER_MODE_REL);
+		hip04_start_tx_timer(priv);
 
 	return rx;
 }

@@ -809,7 +817,6 @@ static int hip04_mac_probe(struct platfo
 	struct hip04_priv *priv;
 	struct resource *res;
 	unsigned int irq;
-	ktime_t txtime;
 	int ret;
 
 	ndev = alloc_etherdev(sizeof(struct hip04_priv));

@@ -846,9 +853,6 @@ static int hip04_mac_probe(struct platfo
 	 */
 	priv->tx_coalesce_frames = TX_DESC_NUM * 3 / 4;
 	priv->tx_coalesce_usecs = 200;
-	/* allow timer to fire after half the time at the earliest */
-	txtime = ktime_set(0, priv->tx_coalesce_usecs * NSEC_PER_USEC / 2);
-	hrtimer_set_expires_range(&priv->tx_coalesce_timer, txtime, txtime);
 	priv->tx_coalesce_timer.function = tx_done;
 
 	priv->map = syscon_node_to_regmap(arg.np);

.

The code sets the expiry value of the timer to a relative value and starts it with hrtimer_start_expires. That's fine, but that only works once. The timer is started in relative mode, so the expiry value gets overwritten with the absolut expiry time (now + expiry).

So once the timer expired, a new call to hrtimer_start_expires results in an immidiately expired timer, because the expiry value is already in the past.

Use the proper mechanisms to (re)start the timer in the intended way.

Signed-off-by Thomas Gleixner <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc dingtianhong <[email protected]>

Cc Arnd Bergmann <[email protected]>

Cc Zhangfei Gao <[email protected]>

Cc Dan Carpenter <[email protected]>

Cc [email protected]

The code sets the expiry value of the timer to a relative value and starts it with hrtimer_start_expires. That's fine, but that only works once. The timer is started in relative mode, so the expiry value gets overwritten with the absolut expiry time (now + expiry).

So once the timer expired, a new call to hrtimer_start_expires results in an immidiately expired timer, because the expiry value is already in the past.

Use the proper mechanisms to (re)start the timer in the intended way.

Signed-off-by Thomas Gleixner <[email protected]>

oz_timer_add() modifies the expiry value of an active timer, which results in data corruption.

Use hrtimer_start() and remove the silly conditional.

While at it use the proper helper function to convert milliseconds to ktime.

Signed-off-by Thomas Gleixner <[email protected]>

Cc Shigekatsu Tateno <[email protected]>

Cc Greg Kroah-Hartman <[email protected]>

Cc [email protected]

---
 drivers/staging/ozwpan/ozproto.c |   17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)