Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1724178 > unrolled thread

[PATCH 00/25] hrtimer: Provide softirq context hrtimers

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 07/25] hrtimer: Reduce conditional code (hres_active) Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 05/25] hrtimer: Switch for loop to _ffs() evaluation Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
      Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-08-31 15:40 +0200
      Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer Bjørn Mork <bjorn@mork.no> - 2017-08-31 16:00 +0200
        [PATCH 24/25 v2] net/cdc_ncm: Replace tasklet with softirq hrtimer Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-05 17:50 +0200
    [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
      Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Iwai <tiwai@suse.de> - 2017-08-31 16:30 +0200
      Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-08-31 16:30 +0200
      Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Iwai <tiwai@suse.de> - 2017-08-31 17:40 +0200
        Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-09-01 12:30 +0200
          Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Iwai <tiwai@suse.de> - 2017-09-01 14:00 +0200
            Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-09-02 03:30 +0200
              Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-09-04 14:50 +0200
              [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-05 18:00 +0200
                Re: [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Iwai <tiwai@suse.de> - 2017-09-05 18:10 +0200
                Re: [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-09-05 18:10 +0200
                  [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with softirq hrtimer Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2017-09-05 18:20 +0200
                    Re: [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with softirq hrtimer Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2017-09-06 06:40 +0200
                      Re: [alsa-devel] [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with     softirq hrtimer Takashi Iwai <tiwai@suse.de> - 2017-09-08 10:30 +0200
    [PATCH 22/25] softirq: Remove tasklet_hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 21/25] xfrm: Replace hrtimer tasklet with softirq hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 06/25] hrtimer: Store running timer in hrtimer_clock_base Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based  hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
      Re: [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq  based hrtimer Thomas Gleixner <tglx@linutronix.de> - 2017-09-01 18:00 +0200
        Re: [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based  hrtimer Oliver Hartkopp <socketcan@hartkopp.net> - 2017-09-01 19:10 +0200
      Re: [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based  hrtimer Oliver Hartkopp <socketcan@hartkopp.net> - 2017-09-01 18:00 +0200
      Re: [PATCH 19/25] can/bcm: Replace hrtimer_tasklet with softirq based  hrtimer Oliver Hartkopp <socketcan@hartkopp.net> - 2017-09-02 20:10 +0200
    [PATCH 10/25] hrtimer: Make handling of hrtimer reprogramming and  enqueuing not conditional Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 14/25] hrtimer: Split out code from __hrtimer_get_next_event()  for reuse Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 25/25] usb/gadget/NCM: Replace tasklet with softirq hrtimer Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 03/25] hrtimer: Fix kerneldoc for struct hrtimer_cpu_base Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    [PATCH 09/25] hrtimer: Reduce conditional code (hrtimer_reprogram()) Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:30 +0200
    Re: [PATCH 00/25] hrtimer: Provide softirq context hrtimers Anna-Maria Gleixner <anna-maria@linutronix.de> - 2017-08-31 14:40 +0200

Page 1 of 2  [1] 2  Next page →

#1724178 — [PATCH 00/25] hrtimer: Provide softirq context hrtimers

There are quite some places in the kernel which use a combination of
hrtimers and tasklets to make use of the precise expiry of hrtimers, which
schedule a tasklet to bring the actual function into softirq context.

This was introduced when the previous hrtimer softirq code was
removed. That code was implemented by expiring the timer in hard irq
context and then deferring the execution of the callback into softirq
context. That caused a lot of pointless shuffling between the rbtree and a
linked list.

In recent discussions it turned out that more potential users of hrtimers
in softirq context might come up. Aside of that the RT patches need this
functionality as well to defer hrtimers into softirq context if their
callbacks are not interrupt safe on RT.

This series implements a new approach by adding SOFT_* clock ids and
instead of doing the list shuffle, timers started with these clock ids are
put into separate soft expiry hrtimer queues. These queues are evaluated
only when the hardirq context detects that the first expiring timer in the
softirq queues has expired. That makes the overhead in the hardirq context
minimal.

The series reworks the code to reuse as much as possible from the existing
facilities for the new softirq hrtimers and integrates them with all
flavours of hrtimers (HIGH_RES=y/n - NOHZ=y/n).

To achieve this quite some of the conditionals in the existing code are
removed for the price of adding some pointless data and state tracking to
the HIGH_RES=n case. That's minimal, but well worth it as it increases the
readability and maintainability of the code.

The first part of the series implements the new functionality and the
second part converts the hrtimer/tasklet users to make use of it and
removes struct hrtimer_tasklet and the surrounding helper functions.

This series is available from git as well:

    git://git.kernel.org/pub/scm/linux/kernel/git/bigeasy/linux-hrtimer.git soft_hrtimer
    https://git.kernel.org/bigeasy/linux-hrtimer/h/soft_hrtimer


Thanks,

	Anna-Maria

---
 drivers/net/usb/cdc_ncm.c             |   37 +-
 drivers/net/wireless/mac80211_hwsim.c |   44 +-
 drivers/usb/gadget/function/f_ncm.c   |   28 -
 include/linux/hrtimer.h               |   76 ++---
 include/linux/interrupt.h             |   25 -
 include/linux/usb/cdc_ncm.h           |    2 
 include/net/xfrm.h                    |    2 
 kernel/softirq.c                      |   51 ---
 kernel/time/hrtimer.c                 |  513 +++++++++++++++++++++-------------
 net/can/bcm.c                         |  150 +++------
 net/xfrm/xfrm_state.c                 |   29 +
 sound/drivers/dummy.c                 |   16 -
 12 files changed, 484 insertions(+), 489 deletions(-)

[toc] | [next] | [standalone]

#1724179 — [PATCH 07/25] hrtimer: Reduce conditional code (hres_active)

The hrtimer_cpu_base struct has the CONFIG_HIGH_RES_TIMERS conditional
struct member hres_active. All related functions to this member are
conditional as well.

There is no functional change, when the hres_active member is unconditional
with all related functions and is set to zero during initialization. This
makes the code easier to read.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
---
 include/linux/hrtimer.h |   17 ++++++-----------
 kernel/time/hrtimer.c   |   30 ++++++++++++++----------------
 2 files changed, 20 insertions(+), 27 deletions(-)

--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -180,9 +180,9 @@ struct hrtimer_cpu_base {
 	unsigned int			clock_was_set_seq;
 	bool				migration_enabled;
 	bool				nohz_active;
+	unsigned int			hres_active	: 1;
 #ifdef CONFIG_HIGH_RES_TIMERS
 	unsigned int			in_hrtirq	: 1,
-					hres_active	: 1,
 					hang_detected	: 1;
 	ktime_t				expires_next;
 	struct hrtimer			*next_timer;
@@ -264,16 +264,16 @@ static inline ktime_t hrtimer_cb_get_tim
 	return timer->base->get_time();
 }
 
-#ifdef CONFIG_HIGH_RES_TIMERS
-struct clock_event_device;
-
-extern void hrtimer_interrupt(struct clock_event_device *dev);
-
 static inline int hrtimer_is_hres_active(struct hrtimer *timer)
 {
 	return timer->base->cpu_base->hres_active;
 }
 
+#ifdef CONFIG_HIGH_RES_TIMERS
+struct clock_event_device;
+
+extern void hrtimer_interrupt(struct clock_event_device *dev);
+
 /*
  * The resolution of the clocks. The resolution value is returned in
  * the clock_getres() system call to give application programmers an
@@ -296,11 +296,6 @@ extern unsigned int hrtimer_resolution;
 
 #define hrtimer_resolution	(unsigned int)LOW_RES_NSEC
 
-static inline int hrtimer_is_hres_active(struct hrtimer *timer)
-{
-	return 0;
-}
-
 static inline void clock_was_set_delayed(void) { }
 
 #endif
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -505,6 +505,19 @@ static inline ktime_t hrtimer_update_bas
 					    offs_real, offs_boot, offs_tai);
 }
 
+/*
+ * Is the high resolution mode active ?
+ */
+static inline int __hrtimer_hres_active(struct hrtimer_cpu_base *cpu_base)
+{
+	return cpu_base->hres_active;
+}
+
+static inline int hrtimer_hres_active(void)
+{
+	return __hrtimer_hres_active(this_cpu_ptr(&hrtimer_bases));
+}
+
 /* High resolution timer related functions */
 #ifdef CONFIG_HIGH_RES_TIMERS
 
@@ -534,19 +547,6 @@ static inline int hrtimer_is_hres_enable
 }
 
 /*
- * Is the high resolution mode active ?
- */
-static inline int __hrtimer_hres_active(struct hrtimer_cpu_base *cpu_base)
-{
-	return cpu_base->hres_active;
-}
-
-static inline int hrtimer_hres_active(void)
-{
-	return __hrtimer_hres_active(this_cpu_ptr(&hrtimer_bases));
-}
-
-/*
  * Reprogram the event source with checking both queues for the
  * next event
  * Called with interrupts disabled and base->lock held
@@ -654,7 +654,6 @@ static void hrtimer_reprogram(struct hrt
 static inline void hrtimer_init_hres(struct hrtimer_cpu_base *base)
 {
 	base->expires_next = KTIME_MAX;
-	base->hres_active = 0;
 }
 
 /*
@@ -713,8 +712,6 @@ void clock_was_set_delayed(void)
 
 #else
 
-static inline int __hrtimer_hres_active(struct hrtimer_cpu_base *b) { return 0; }
-static inline int hrtimer_hres_active(void) { return 0; }
 static inline int hrtimer_is_hres_enabled(void) { return 0; }
 static inline void hrtimer_switch_to_hres(void) { }
 static inline void
@@ -1592,6 +1589,7 @@ int hrtimers_prepare_cpu(unsigned int cp
 	}
 
 	cpu_base->cpu = cpu;
+	cpu_base->hres_active = 0;
 	hrtimer_init_hres(cpu_base);
 	return 0;
 }

[toc] | [prev] | [next] | [standalone]

#1724180 — [PATCH 05/25] hrtimer: Switch for loop to _ffs() evaluation

From: Anna-Maria Gleixner <anna-maria@linutronix.de>

Looping over all clock bases to find active bits is suboptimal if not all
bases are active.

Avoid this by converting it to a __ffs() evaluation.

Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
---
 kernel/time/hrtimer.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -465,17 +465,18 @@ static inline void hrtimer_update_next_t
 
 static ktime_t __hrtimer_get_next_event(struct hrtimer_cpu_base *cpu_base)
 {
-	struct hrtimer_clock_base *base = cpu_base->clock_base;
 	unsigned int active = cpu_base->active_bases;
 	ktime_t expires, expires_next = KTIME_MAX;
 
 	hrtimer_update_next_timer(cpu_base, NULL);
-	for (; active; base++, active >>= 1) {
+	while (active) {
+		unsigned int id = __ffs(active);
+		struct hrtimer_clock_base *base;
 		struct timerqueue_node *next;
 		struct hrtimer *timer;
 
-		if (!(active & 0x01))
-			continue;
+		active &= ~(1U << id);
+		base = cpu_base->clock_base + id;
 
 		next = timerqueue_getnext(&base->active);
 		timer = container_of(next, struct hrtimer, node);
@@ -1242,15 +1243,16 @@ static void __run_hrtimer(struct hrtimer
 
 static void __hrtimer_run_queues(struct hrtimer_cpu_base *cpu_base, ktime_t now)
 {
-	struct hrtimer_clock_base *base = cpu_base->clock_base;
 	unsigned int active = cpu_base->active_bases;
 
-	for (; active; base++, active >>= 1) {
+	while (active) {
+		unsigned int id = __ffs(active);
+		struct hrtimer_clock_base *base;
 		struct timerqueue_node *node;
 		ktime_t basenow;
 
-		if (!(active & 0x01))
-			continue;
+		active &= ~(1U << id);
+		base = cpu_base->clock_base + id;
 
 		basenow = ktime_add(now, base->offset);
 

[toc] | [prev] | [next] | [standalone]

#1724181 — [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer

From: Thomas Gleixner <tglx@linutronix.de>

The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
softirq context. This can be also achieved without the tasklet but with
CLOCK_MONOTONIC_SOFT as hrtimer base.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Oliver Neukum <oliver@neukum.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org
---
 drivers/net/usb/cdc_ncm.c   |   37 ++++++++++++++++---------------------
 include/linux/usb/cdc_ncm.h |    2 +-
 2 files changed, 17 insertions(+), 22 deletions(-)

--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -61,7 +61,6 @@ static bool prefer_mbim;
 module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM functions");
 
-static void cdc_ncm_txpath_bh(unsigned long param);
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
 static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
 static struct usb_driver cdc_ncm_driver;
@@ -777,10 +776,9 @@ int cdc_ncm_bind_common(struct usbnet *d
 	if (!ctx)
 		return -ENOMEM;
 
-	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	ctx->tx_timer.function = &cdc_ncm_tx_timer_cb;
-	ctx->bh.data = (unsigned long)dev;
-	ctx->bh.func = cdc_ncm_txpath_bh;
+	ctx->usbnet = dev;
 	atomic_set(&ctx->stop, 0);
 	spin_lock_init(&ctx->mtx);
 
@@ -967,10 +965,7 @@ void cdc_ncm_unbind(struct usbnet *dev,
 
 	atomic_set(&ctx->stop, 1);
 
-	if (hrtimer_active(&ctx->tx_timer))
-		hrtimer_cancel(&ctx->tx_timer);
-
-	tasklet_kill(&ctx->bh);
+	hrtimer_cancel(&ctx->tx_timer);
 
 	/* handle devices with combined control and data interface */
 	if (ctx->control == ctx->data)
@@ -1348,20 +1343,9 @@ static void cdc_ncm_tx_timeout_start(str
 				HRTIMER_MODE_REL);
 }
 
-static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx)
 {
-	struct cdc_ncm_ctx *ctx =
-			container_of(timer, struct cdc_ncm_ctx, tx_timer);
-
-	if (!atomic_read(&ctx->stop))
-		tasklet_schedule(&ctx->bh);
-	return HRTIMER_NORESTART;
-}
-
-static void cdc_ncm_txpath_bh(unsigned long param)
-{
-	struct usbnet *dev = (struct usbnet *)param;
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	struct usbnet *dev = ctx->usbnet;
 
 	spin_lock_bh(&ctx->mtx);
 	if (ctx->tx_timer_pending != 0) {
@@ -1379,6 +1363,17 @@ static void cdc_ncm_txpath_bh(unsigned l
 	}
 }
 
+static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+{
+	struct cdc_ncm_ctx *ctx =
+			container_of(timer, struct cdc_ncm_ctx, tx_timer);
+
+	if (!atomic_read(&ctx->stop))
+		cdc_ncm_txpath_bh(ctx);
+
+	return HRTIMER_NORESTART;
+}
+
 struct sk_buff *
 cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 {
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -92,7 +92,6 @@
 struct cdc_ncm_ctx {
 	struct usb_cdc_ncm_ntb_parameters ncm_parm;
 	struct hrtimer tx_timer;
-	struct tasklet_struct bh;
 
 	const struct usb_cdc_ncm_desc *func_desc;
 	const struct usb_cdc_mbim_desc *mbim_desc;
@@ -101,6 +100,7 @@ struct cdc_ncm_ctx {
 
 	struct usb_interface *control;
 	struct usb_interface *data;
+	struct usbnet *usbnet;
 
 	struct sk_buff *tx_curr_skb;
 	struct sk_buff *tx_rem_skb;

[toc] | [prev] | [next] | [standalone]

#1724236 — Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer

On Thu, Aug 31, 2017 at 12:23:46PM -0000, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
> softirq context. This can be also achieved without the tasklet but with
> CLOCK_MONOTONIC_SOFT as hrtimer base.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: netdev@vger.kernel.org


Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

[toc] | [prev] | [next] | [standalone]

#1724267 — Re: [PATCH 24/25] net/cdc_ncm: Replace tasklet with softirq hrtimer

Anna-Maria Gleixner <anna-maria@linutronix.de> writes:

> From: Thomas Gleixner <tglx@linutronix.de>
>
> The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
> softirq context. This can be also achieved without the tasklet but with
> CLOCK_MONOTONIC_SOFT as hrtimer base.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
>  drivers/net/usb/cdc_ncm.c   |   37 ++++++++++++++++---------------------
>  include/linux/usb/cdc_ncm.h |    2 +-
>  2 files changed, 17 insertions(+), 22 deletions(-)
>
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -61,7 +61,6 @@ static bool prefer_mbim;
>  module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM functions");
>  
> -static void cdc_ncm_txpath_bh(unsigned long param);
>  static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
>  static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
>  static struct usb_driver cdc_ncm_driver;
> @@ -777,10 +776,9 @@ int cdc_ncm_bind_common(struct usbnet *d
>  	if (!ctx)
>  		return -ENOMEM;
>  
> -	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>  	ctx->tx_timer.function = &cdc_ncm_tx_timer_cb;
> -	ctx->bh.data = (unsigned long)dev;
> -	ctx->bh.func = cdc_ncm_txpath_bh;
> +	ctx->usbnet = dev;
>  	atomic_set(&ctx->stop, 0);
>  	spin_lock_init(&ctx->mtx);
>  
> @@ -967,10 +965,7 @@ void cdc_ncm_unbind(struct usbnet *dev,
>  
>  	atomic_set(&ctx->stop, 1);
>  
> -	if (hrtimer_active(&ctx->tx_timer))
> -		hrtimer_cancel(&ctx->tx_timer);
> -
> -	tasklet_kill(&ctx->bh);
> +	hrtimer_cancel(&ctx->tx_timer);
>  
>  	/* handle devices with combined control and data interface */
>  	if (ctx->control == ctx->data)
> @@ -1348,20 +1343,9 @@ static void cdc_ncm_tx_timeout_start(str
>  				HRTIMER_MODE_REL);
>  }
>  
> -static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
> +static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx)
>  {
> -	struct cdc_ncm_ctx *ctx =
> -			container_of(timer, struct cdc_ncm_ctx, tx_timer);
> -
> -	if (!atomic_read(&ctx->stop))
> -		tasklet_schedule(&ctx->bh);
> -	return HRTIMER_NORESTART;
> -}
> -
> -static void cdc_ncm_txpath_bh(unsigned long param)
> -{
> -	struct usbnet *dev = (struct usbnet *)param;
> -	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> +	struct usbnet *dev = ctx->usbnet;
>  
>  	spin_lock_bh(&ctx->mtx);
>  	if (ctx->tx_timer_pending != 0) {
> @@ -1379,6 +1363,17 @@ static void cdc_ncm_txpath_bh(unsigned l
>  	}
>  }
>  
> +static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
> +{
> +	struct cdc_ncm_ctx *ctx =
> +			container_of(timer, struct cdc_ncm_ctx, tx_timer);
> +
> +	if (!atomic_read(&ctx->stop))
> +		cdc_ncm_txpath_bh(ctx);
> +
> +	return HRTIMER_NORESTART;
> +}
> +
>  struct sk_buff *
>  cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
>  {
> --- a/include/linux/usb/cdc_ncm.h
> +++ b/include/linux/usb/cdc_ncm.h
> @@ -92,7 +92,6 @@
>  struct cdc_ncm_ctx {
>  	struct usb_cdc_ncm_ntb_parameters ncm_parm;
>  	struct hrtimer tx_timer;
> -	struct tasklet_struct bh;
>  
>  	const struct usb_cdc_ncm_desc *func_desc;
>  	const struct usb_cdc_mbim_desc *mbim_desc;
> @@ -101,6 +100,7 @@ struct cdc_ncm_ctx {
>  
>  	struct usb_interface *control;
>  	struct usb_interface *data;
> +	struct usbnet *usbnet;
>  
>  	struct sk_buff *tx_curr_skb;
>  	struct sk_buff *tx_rem_skb;




I believe the struct usbnet pointer is redundant.  We already have lots
of pointers back and forth here.  This should work, but is not tested:

	struct usbnet *dev = usb_get_intfdata(ctx->control):




Bjørn

[toc] | [prev] | [next] | [standalone]

#1726823 — [PATCH 24/25 v2] net/cdc_ncm: Replace tasklet with softirq hrtimer

From: Thomas Gleixner <tglx@linutronix.de>

The bh tasklet is used in invoke the hrtimer (cdc_ncm_tx_timer_cb) in
softirq context. This can be also achieved without the tasklet but with
CLOCK_MONOTONIC_SOFT as hrtimer base.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Oliver Neukum <oliver@neukum.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: netdev@vger.kernel.org
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
[bigeasy: using usb_get_intfdata() as suggested by Bjørn Mork]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2017-08-31 15:57:04 [+0200], Bjørn Mork wrote:
> I believe the struct usbnet pointer is redundant.  We already have lots
> of pointers back and forth here.  This should work, but is not tested:
> 
> 	struct usbnet *dev = usb_get_intfdata(ctx->control):

I think so, too. Still untested as I don't have a working gadget around.

v1…v2: Updated as suggested by Bjørn and added Greg's Acked-by.

 drivers/net/usb/cdc_ncm.c   | 36 +++++++++++++++---------------------
 include/linux/usb/cdc_ncm.h |  1 -
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 8f572b9f3625..42f7bd90e6a4 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -61,7 +61,6 @@ static bool prefer_mbim;
 module_param(prefer_mbim, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(prefer_mbim, "Prefer MBIM setting on dual NCM/MBIM functions");
 
-static void cdc_ncm_txpath_bh(unsigned long param);
 static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx);
 static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *hr_timer);
 static struct usb_driver cdc_ncm_driver;
@@ -777,10 +776,8 @@ int cdc_ncm_bind_common(struct usbnet *dev, struct usb_interface *intf, u8 data_
 	if (!ctx)
 		return -ENOMEM;
 
-	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&ctx->tx_timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	ctx->tx_timer.function = &cdc_ncm_tx_timer_cb;
-	ctx->bh.data = (unsigned long)dev;
-	ctx->bh.func = cdc_ncm_txpath_bh;
 	atomic_set(&ctx->stop, 0);
 	spin_lock_init(&ctx->mtx);
 
@@ -967,10 +964,7 @@ void cdc_ncm_unbind(struct usbnet *dev, struct usb_interface *intf)
 
 	atomic_set(&ctx->stop, 1);
 
-	if (hrtimer_active(&ctx->tx_timer))
-		hrtimer_cancel(&ctx->tx_timer);
-
-	tasklet_kill(&ctx->bh);
+	hrtimer_cancel(&ctx->tx_timer);
 
 	/* handle devices with combined control and data interface */
 	if (ctx->control == ctx->data)
@@ -1348,20 +1342,9 @@ static void cdc_ncm_tx_timeout_start(struct cdc_ncm_ctx *ctx)
 				HRTIMER_MODE_REL);
 }
 
-static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+static void cdc_ncm_txpath_bh(struct cdc_ncm_ctx *ctx)
 {
-	struct cdc_ncm_ctx *ctx =
-			container_of(timer, struct cdc_ncm_ctx, tx_timer);
-
-	if (!atomic_read(&ctx->stop))
-		tasklet_schedule(&ctx->bh);
-	return HRTIMER_NORESTART;
-}
-
-static void cdc_ncm_txpath_bh(unsigned long param)
-{
-	struct usbnet *dev = (struct usbnet *)param;
-	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
+	struct usbnet *dev = usb_get_intfdata(ctx->control);
 
 	spin_lock_bh(&ctx->mtx);
 	if (ctx->tx_timer_pending != 0) {
@@ -1379,6 +1362,17 @@ static void cdc_ncm_txpath_bh(unsigned long param)
 	}
 }
 
+static enum hrtimer_restart cdc_ncm_tx_timer_cb(struct hrtimer *timer)
+{
+	struct cdc_ncm_ctx *ctx =
+			container_of(timer, struct cdc_ncm_ctx, tx_timer);
+
+	if (!atomic_read(&ctx->stop))
+		cdc_ncm_txpath_bh(ctx);
+
+	return HRTIMER_NORESTART;
+}
+
 struct sk_buff *
 cdc_ncm_tx_fixup(struct usbnet *dev, struct sk_buff *skb, gfp_t flags)
 {
diff --git a/include/linux/usb/cdc_ncm.h b/include/linux/usb/cdc_ncm.h
index 1a59699cf82a..62b506fddf8d 100644
--- a/include/linux/usb/cdc_ncm.h
+++ b/include/linux/usb/cdc_ncm.h
@@ -92,7 +92,6 @@
 struct cdc_ncm_ctx {
 	struct usb_cdc_ncm_ntb_parameters ncm_parm;
 	struct hrtimer tx_timer;
-	struct tasklet_struct bh;
 
 	const struct usb_cdc_ncm_desc *func_desc;
 	const struct usb_cdc_mbim_desc *mbim_desc;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1724182 — [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

From: Thomas Gleixner <tglx@linutronix.de>

The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org
---
 sound/drivers/dummy.c |   16 +++-------------
 1 file changed, 3 insertions(+), 13 deletions(-)

--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
 	ktime_t period_time;
 	atomic_t running;
 	struct hrtimer timer;
-	struct tasklet_struct tasklet;
 	struct snd_pcm_substream *substream;
 };
 
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
-	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
-	if (atomic_read(&dpcm->running))
-		snd_pcm_period_elapsed(dpcm->substream);
-}
-
 static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 {
 	struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
 	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
 	if (!atomic_read(&dpcm->running))
 		return HRTIMER_NORESTART;
-	tasklet_schedule(&dpcm->tasklet);
+
+	snd_pcm_period_elapsed(dpcm->substream);
 	hrtimer_forward_now(timer, dpcm->period_time);
 	return HRTIMER_RESTART;
 }
@@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
 static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
 {
 	hrtimer_cancel(&dpcm->timer);
-	tasklet_kill(&dpcm->tasklet);
 }
 
 static snd_pcm_uframes_t
@@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
 	if (!dpcm)
 		return -ENOMEM;
 	substream->runtime->private_data = dpcm;
-	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	dpcm->timer.function = dummy_hrtimer_callback;
 	dpcm->substream = substream;
 	atomic_set(&dpcm->running, 0);
-	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
-		     (unsigned long)dpcm);
 	return 0;
 }
 

[toc] | [prev] | [next] | [standalone]

#1724284 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

On Thu, 31 Aug 2017 16:21:17 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> > From: Thomas Gleixner <tglx@linutronix.de>
> > 
> > The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> > the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> > callback in softirq context as well which renders the tasklet useless.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> > Cc: Jaroslav Kysela <perex@perex.cz>
> > Cc: Takashi Iwai <tiwai@suse.com>
> > Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> > Cc: alsa-devel@alsa-project.org
> > ---
> >  sound/drivers/dummy.c |   16 +++-------------
> >  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> I prefer this patch as long as this driver can still receive callbacks
> from hrtimer subsystem.
> 
> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> Unfortunately, I have too poor machine to compile whole kernel now, thus
> didn't do any tests, sorry.
> 
> I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
> same purpose. I think we can simplify it, too. Please refer to a patch in
> the end of this message. (But not tested yet for the above reason...)

The pcsp is a bit special.  It's really high frequent irq calls for
controlling the beep on/off, thus offloading the whole isn't good.


thanks,

Takashi

[toc] | [prev] | [next] | [standalone]

#1724286 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

Hi,

On Aug 31 2017 21:23, Anna-Maria Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Cc: alsa-devel@alsa-project.org
> ---
>  sound/drivers/dummy.c |   16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)

I prefer this patch as long as this driver can still receive callbacks
from hrtimer subsystem.

Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Unfortunately, I have too poor machine to compile whole kernel now, thus
didn't do any tests, sorry.

I note that ALSA pcsp driver uses a combination of hrtimer/tasklet for the
same purpose. I think we can simplify it, too. Please refer to a patch in
the end of this message. (But not tested yet for the above reason...)

> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
>  	ktime_t period_time;
>  	atomic_t running;
>  	struct hrtimer timer;
> -	struct tasklet_struct tasklet;
>  	struct snd_pcm_substream *substream;
>  };
>  
> -static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
> -{
> -	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
> -	if (atomic_read(&dpcm->running))
> -		snd_pcm_period_elapsed(dpcm->substream);
> -}
> -
>  static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>  {
>  	struct dummy_hrtimer_pcm *dpcm;
> @@ -394,7 +386,8 @@ static enum hrtimer_restart dummy_hrtime
>  	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>  	if (!atomic_read(&dpcm->running))
>  		return HRTIMER_NORESTART;
> -	tasklet_schedule(&dpcm->tasklet);
> +
> +	snd_pcm_period_elapsed(dpcm->substream);
>  	hrtimer_forward_now(timer, dpcm->period_time);
>  	return HRTIMER_RESTART;
>  }
> @@ -421,7 +414,6 @@ static int dummy_hrtimer_stop(struct snd
>  static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>  {
>  	hrtimer_cancel(&dpcm->timer);
> -	tasklet_kill(&dpcm->tasklet);
>  }
>  
>  static snd_pcm_uframes_t
> @@ -466,12 +458,10 @@ static int dummy_hrtimer_create(struct s
>  	if (!dpcm)
>  		return -ENOMEM;
>  	substream->runtime->private_data = dpcm;
> -	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>  	dpcm->timer.function = dummy_hrtimer_callback;
>  	dpcm->substream = substream;
>  	atomic_set(&dpcm->running, 0);
> -	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
> -		     (unsigned long)dpcm);
>  	return 0;
>  }

From b2417f83e0ccbdfe1fc6870817ff0a1bd9243c77 Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Thu, 31 Aug 2017 22:52:33 +0900
Subject: [PATCH] ALSA: pcsp: code optimization for hrtimer in software IRQ
 context

In a development period for v4.14, code refactoring was done for hrtimer
subsystem. This enables to receive callbacks from hrtimer in software IRQ
context.

Currently, ALSA pcsp driver uses hrtimer callback to handle period elapse
of PCM buffer and actual calculation of pointer position is done in
software IRQ context of tasklet. This can be simplified to the introduced
hrtimer in software IRQ context.

This commit applies an optimization for the above reason.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/drivers/pcsp/pcsp.c     |  2 +-
 sound/drivers/pcsp/pcsp_lib.c | 24 +++++-------------------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c
index 0dd3f46eb03e..8fac38b81c4f 100644
--- a/sound/drivers/pcsp/pcsp.c
+++ b/sound/drivers/pcsp/pcsp.c
@@ -100,7 +100,7 @@ static int snd_card_pcsp_probe(int devnum, struct device *dev)
 	if (devnum != 0)
 		return -EINVAL;
 
-	hrtimer_init(&pcsp_chip.timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&pcsp_chip.timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	pcsp_chip.timer.function = pcsp_do_timer;
 
 	err = snd_card_new(dev, index, id, THIS_MODULE, 0, &card);
diff --git a/sound/drivers/pcsp/pcsp_lib.c b/sound/drivers/pcsp/pcsp_lib.c
index 2f5a35f38ce1..d2b67463ddd3 100644
--- a/sound/drivers/pcsp/pcsp_lib.c
+++ b/sound/drivers/pcsp/pcsp_lib.c
@@ -21,22 +21,6 @@ MODULE_PARM_DESC(nforce_wa, "Apply NForce chipset workaround "
 
 #define DMIX_WANTS_S16	1
 
-/*
- * Call snd_pcm_period_elapsed in a tasklet
- * This avoids spinlock messes and long-running irq contexts
- */
-static void pcsp_call_pcm_elapsed(unsigned long priv)
-{
-	if (atomic_read(&pcsp_chip.timer_active)) {
-		struct snd_pcm_substream *substream;
-		substream = pcsp_chip.playback_substream;
-		if (substream)
-			snd_pcm_period_elapsed(substream);
-	}
-}
-
-static DECLARE_TASKLET(pcsp_pcm_tasklet, pcsp_call_pcm_elapsed, 0);
-
 /* write the port and returns the next expire time in ns;
  * called at the trigger-start and in hrtimer callback
  */
@@ -121,8 +105,11 @@ static void pcsp_pointer_update(struct snd_pcsp *chip)
 	}
 	spin_unlock_irqrestore(&chip->substream_lock, flags);
 
-	if (periods_elapsed)
-		tasklet_schedule(&pcsp_pcm_tasklet);
+	if (!periods_elapsed)
+		return;
+
+	if (atomic_read(&pcsp_chip.timer_active))
+		snd_pcm_period_elapsed(substream);
 }
 
 enum hrtimer_restart pcsp_do_timer(struct hrtimer *handle)
@@ -195,7 +182,6 @@ void pcsp_sync_stop(struct snd_pcsp *chip)
 	pcsp_stop_playing(chip);
 	local_irq_enable();
 	hrtimer_cancel(&chip->timer);
-	tasklet_kill(&pcsp_pcm_tasklet);
 }
 
 static int snd_pcsp_playback_close(struct snd_pcm_substream *substream)
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1724325 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

On Thu, 31 Aug 2017 14:23:45 +0200,
Anna-Maria Gleixner wrote:
> 
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Cc: alsa-devel@alsa-project.org

I gave it at try, but it caused a kernel hang, unfortunately.

The reason is that snd_pcm_period_elapased() may stop the stream
(e.g. when reaching at the end).  With this patchset, it'll lead to
the call of hrtimer_cancel() from the hrtimer callback itself, thus it
stalls.

Below is the additional fix over your patch for working around it.
I believe it should cover most corner cases, and seems working fine
through quick tests, so far.


thanks,

Takashi

---
diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..b5dd64e3dab1 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
 	ktime_t base_time;
 	ktime_t period_time;
 	atomic_t running;
+	atomic_t callback_running;
 	struct hrtimer timer;
 	struct snd_pcm_substream *substream;
 };
@@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 	if (!atomic_read(&dpcm->running))
 		return HRTIMER_NORESTART;
 
+	atomic_inc(&dpcm->callback_running);
 	snd_pcm_period_elapsed(dpcm->substream);
+	atomic_dec(&dpcm->callback_running);
+	/* may be flipped during snd_pcm_period_elapsed() */
+	if (!atomic_read(&dpcm->running))
+		return HRTIMER_NORESTART;
+
 	hrtimer_forward_now(timer, dpcm->period_time);
+	atomic_dec(&dpcm->callback_running);
 	return HRTIMER_RESTART;
 }
 
@@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
 	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
 	atomic_set(&dpcm->running, 0);
-	hrtimer_cancel(&dpcm->timer);
+	/* issue hrtimer_cancel() only when called outside the callback */
+	if (!atomic_read(&dpcm->callback_running))
+		hrtimer_cancel(&dpcm->timer);
 	return 0;
 }
 
@@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
 	dpcm->timer.function = dummy_hrtimer_callback;
 	dpcm->substream = substream;
 	atomic_set(&dpcm->running, 0);
+	atomic_set(&dpcm->callback_running, 0);
 	return 0;
 }
 

[toc] | [prev] | [next] | [standalone]

#1724944 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

Hi,

On Sep 1 2017 00:36, Takashi Iwai wrote:
> I gave it at try, but it caused a kernel hang, unfortunately.
> 
> The reason is that snd_pcm_period_elapased() may stop the stream
> (e.g. when reaching at the end).  With this patchset, it'll lead to
> the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> stalls.
 
I can reproduce this bug.

> Below is the additional fix over your patch for working around it.
> I believe it should cover most corner cases, and seems working fine
> through quick tests, so far.

This patch looks good to me, too. But I have an alternative.

We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').
Usage of this helper function on .stop callback to skip cancellation can
avoid the stall. In this case, after stopping PCM substream, the hrtimer
callback should return HRTIMER_NORESTART to avoid restarting, as well as
your patch.  Please test a patch in this message.

> ---
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index 273d60c42125..b5dd64e3dab1 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
>   	ktime_t base_time;
>   	ktime_t period_time;
>   	atomic_t running;
> +	atomic_t callback_running;
>   	struct hrtimer timer;
>   	struct snd_pcm_substream *substream;
>   };
> @@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>   	if (!atomic_read(&dpcm->running))
>   		return HRTIMER_NORESTART;
>   
> +	atomic_inc(&dpcm->callback_running);
>   	snd_pcm_period_elapsed(dpcm->substream);
> +	atomic_dec(&dpcm->callback_running);
> +	/* may be flipped during snd_pcm_period_elapsed() */
> +	if (!atomic_read(&dpcm->running))
> +		return HRTIMER_NORESTART;
> +
>   	hrtimer_forward_now(timer, dpcm->period_time);
> +	atomic_dec(&dpcm->callback_running);
>   	return HRTIMER_RESTART;
>   }
>   
> @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
>   	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>   
>   	atomic_set(&dpcm->running, 0);
> -	hrtimer_cancel(&dpcm->timer);
> +	/* issue hrtimer_cancel() only when called outside the callback */
> +	if (!atomic_read(&dpcm->callback_running))
> +		hrtimer_cancel(&dpcm->timer);
>   	return 0;
>   }
>   
> @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
>   	dpcm->timer.function = dummy_hrtimer_callback;
>   	dpcm->substream = substream;
>   	atomic_set(&dpcm->running, 0);
> +	atomic_set(&dpcm->callback_running, 0);
>   	return 0;
>   }

From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Date: Fri, 1 Sep 2017 19:10:18 +0900
Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
 a callback of hrtimer

A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
because 'struct hrtimer_clock_base.running' is not NULL on the callback.
In hrtimer subsystem, this member is used to indicate the instance of
hrtimer gets callbacks and there's a helper function,
'hrtimer_callback_running()' to check it.

ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
of PCM buffer. When XRUN occurs on PCM substream, in a call of
'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
stop the substream. In current implementation, 'hrtimer_cancel()' is
used to wait for cancellation of hrtimer. However, as described, this
brings endless loop.

For this problem, this commit uses 'hrtimer_callback_running()' to
detect whether to be on a callback of hrtimer or not, then skip
cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
skipped.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/drivers/dummy.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index 273d60c42125..9caf754c6135 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -387,7 +387,11 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 	if (!atomic_read(&dpcm->running))
 		return HRTIMER_NORESTART;
 
+	/* In a case of XRUN, this calls .trigger to stop PCM substream. */
 	snd_pcm_period_elapsed(dpcm->substream);
+	if (!atomic_read(&dpcm->running))
+		return HRTIMER_NORESTART;
+
 	hrtimer_forward_now(timer, dpcm->period_time);
 	return HRTIMER_RESTART;
 }
@@ -407,7 +411,8 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
 	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
 	atomic_set(&dpcm->running, 0);
-	hrtimer_cancel(&dpcm->timer);
+	if (!hrtimer_callback_running(&dpcm->timer))
+		hrtimer_cancel(&dpcm->timer);
 	return 0;
 }
 
-- 
2.11.0


Regards

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

#1724996 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

On Fri, 01 Sep 2017 12:25:37 +0200,
Takashi Sakamoto wrote:
> 
> Hi,
> 
> On Sep 1 2017 00:36, Takashi Iwai wrote:
> > I gave it at try, but it caused a kernel hang, unfortunately.
> > 
> > The reason is that snd_pcm_period_elapased() may stop the stream
> > (e.g. when reaching at the end).  With this patchset, it'll lead to
> > the call of hrtimer_cancel() from the hrtimer callback itself, thus it
> > stalls.
>  
> I can reproduce this bug.
> 
> > Below is the additional fix over your patch for working around it.
> > I believe it should cover most corner cases, and seems working fine
> > through quick tests, so far.
> 
> This patch looks good to me, too. But I have an alternative.
> 
> We can use 'hrtimer_callback_running()' to detect whether to be on hrtimer
> callback or not (please read '__run_hrtimer()' in 'kernel/time/hrtimer.c').

A good point, this is a better choice.

> Usage of this helper function on .stop callback to skip cancellation can
> avoid the stall. In this case, after stopping PCM substream, the hrtimer
> callback should return HRTIMER_NORESTART to avoid restarting, as well as
> your patch.  Please test a patch in this message.
> 
> > ---
> > diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> > index 273d60c42125..b5dd64e3dab1 100644
> > --- a/sound/drivers/dummy.c
> > +++ b/sound/drivers/dummy.c
> > @@ -375,6 +375,7 @@ struct dummy_hrtimer_pcm {
> >   	ktime_t base_time;
> >   	ktime_t period_time;
> >   	atomic_t running;
> > +	atomic_t callback_running;
> >   	struct hrtimer timer;
> >   	struct snd_pcm_substream *substream;
> >   };
> > @@ -387,8 +388,15 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
> >   	if (!atomic_read(&dpcm->running))
> >   		return HRTIMER_NORESTART;
> >   
> > +	atomic_inc(&dpcm->callback_running);
> >   	snd_pcm_period_elapsed(dpcm->substream);
> > +	atomic_dec(&dpcm->callback_running);
> > +	/* may be flipped during snd_pcm_period_elapsed() */
> > +	if (!atomic_read(&dpcm->running))
> > +		return HRTIMER_NORESTART;
> > +
> >   	hrtimer_forward_now(timer, dpcm->period_time);
> > +	atomic_dec(&dpcm->callback_running);
> >   	return HRTIMER_RESTART;
> >   }
> >   
> > @@ -407,7 +415,9 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
> >   	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
> >   
> >   	atomic_set(&dpcm->running, 0);
> > -	hrtimer_cancel(&dpcm->timer);
> > +	/* issue hrtimer_cancel() only when called outside the callback */
> > +	if (!atomic_read(&dpcm->callback_running))
> > +		hrtimer_cancel(&dpcm->timer);
> >   	return 0;
> >   }
> >   
> > @@ -462,6 +472,7 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
> >   	dpcm->timer.function = dummy_hrtimer_callback;
> >   	dpcm->substream = substream;
> >   	atomic_set(&dpcm->running, 0);
> > +	atomic_set(&dpcm->callback_running, 0);
> >   	return 0;
> >   }
> 
> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Date: Fri, 1 Sep 2017 19:10:18 +0900
> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
>  a callback of hrtimer
> 
> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
> because 'struct hrtimer_clock_base.running' is not NULL on the callback.
> In hrtimer subsystem, this member is used to indicate the instance of
> hrtimer gets callbacks and there's a helper function,
> 'hrtimer_callback_running()' to check it.
> 
> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
> of PCM buffer. When XRUN occurs on PCM substream, in a call of
> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
> stop the substream. In current implementation, 'hrtimer_cancel()' is
> used to wait for cancellation of hrtimer. However, as described, this
> brings endless loop.

It's not only about XRUN.  When the stream finishes the draining, it
stops the stream gracefully -- that is the very normal operation.

> For this problem, this commit uses 'hrtimer_callback_running()' to
> detect whether to be on a callback of hrtimer or not, then skip
> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
> skipped.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

It's better to fold the fix into the original patch instead of
introducing a bug and fixing it.


Takashi

[toc] | [prev] | [next] | [standalone]

#1725412 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

On p 1 2017 20:58, Takashi Iwai wrote:
>> >From 07d61ba2a1c0e06e914443225e194d99f2d8c58d Mon Sep 17 00:00:00 2001
>> From: Takashi Sakamoto <o-takashi@sakamocchi.jp>
>> Date: Fri, 1 Sep 2017 19:10:18 +0900
>> Subject: [PATCH] ALSA: dummy: avoid stall due to a call of hrtimer_cancel() on
>>   a callback of hrtimer
>>
>> A call of 'htrimer_cancel()' on a callback of hrtimer brings endless loop
>> because 'struct hrtimer_clock_base.running' is not NULL on the callback.
>> In hrtimer subsystem, this member is used to indicate the instance of
>> hrtimer gets callbacks and there's a helper function,
>> 'hrtimer_callback_running()' to check it.
>>
>> ALSA dummy driver uses hrtimer to emulate hardware interrupt per period
>> of PCM buffer. When XRUN occurs on PCM substream, in a call of
>> 'snd_pcm_period_elapsed()', 'struct snd_pcm_ops.stop()' is called to
>> stop the substream. In current implementation, 'hrtimer_cancel()' is
>> used to wait for cancellation of hrtimer. However, as described, this
>> brings endless loop.
> 
> It's not only about XRUN.  When the stream finishes the draining, it
> stops the stream gracefully -- that is the very normal operation.

I overlooked it. Thanks for your indication.

>> For this problem, this commit uses 'hrtimer_callback_running()' to
>> detect whether to be on a callback of hrtimer or not, then skip
>> cancellation of hrtimer in hrtimer callbacks. Furthermore, at a case of
>> XRUN, hrtimer callback returns HRTIMER_NORESTART after a call of
>> 'snd_pcm_period_elapsed()' to discontinue hrtimr because cancellation is
>> skipped.
>>
>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> 
> It's better to fold the fix into the original patch instead of
> introducing a bug and fixing it.

Yep. I request the authors to include this fix.


Well, in sound subsystem, there're a few drivers which uses hrtimer:
  - snd-pcsp
  - snd-sh-dac-audio
  - snd-soc-imx-pcm-fiq

As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
in a point of state of PCM substream and it shall gain the same bug if 
improved. Later, I posted some patches for them.


Thanks

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

#1726046 — Re: [PATCH 23/25] ALSA/dummy: Replace tasklet with softirq hrtimer

Hi,

On Sep 2 2017 10:19, Takashi Sakamoto wrote:
> Well, in sound subsystem, there're a few drivers which uses hrtimer:
>   - snd-pcsp
>   - snd-sh-dac-audio
>   - snd-soc-imx-pcm-fiq
> 
> As a quick glance, 'snd-sh-dac-audio' includes the same bug, too. 
> Additionally, 'snd-soc-imx-pcm-fiq' maintains hrtimer with loose manner 
> in a point of state of PCM substream and it shall gain the same bug if 
> improved. Later, I posted some patches for them.

After reading code thoroughly, I conclude that no need to fix these two
drivers. They're programmed with own protections. The former
(snd-sh-dac-audio) has 'struct snd_sh_dac.empty' and the latter
(snd-soc-imx-pcm-fiq) has 'struct imx_pcm_runtime_data.playing' and
'.capturing', to avoid cancellation of hrtimer on hrtimer callback.

These ways are not necessarily efficient but actually have no trouble.
I leave them as is.


Regards

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

#1726826 — [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer

From: Thomas Gleixner <tglx@linutronix.de>

The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org
[o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
            of hrtimer]
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2017-09-02 10:19:45 [+0900], Takashi Sakamoto wrote:
> Yep. I request the authors to include this 
Thank you for providing a fix.

v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.

So this patch now is okay?

 sound/drivers/dummy.c | 23 +++++++++--------------
 1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index dd5ed037adf2..3d01fe17ed36 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
 	ktime_t period_time;
 	atomic_t running;
 	struct hrtimer timer;
-	struct tasklet_struct tasklet;
 	struct snd_pcm_substream *substream;
 };
 
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
-	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
-	if (atomic_read(&dpcm->running))
-		snd_pcm_period_elapsed(dpcm->substream);
-}
-
 static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 {
 	struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,12 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
 	if (!atomic_read(&dpcm->running))
 		return HRTIMER_NORESTART;
-	tasklet_schedule(&dpcm->tasklet);
+
+	/* In a case of XRUN, this calls .trigger to stop PCM substream. */
+	snd_pcm_period_elapsed(dpcm->substream);
+	if (!atomic_read(&dpcm->running))
+		return HRTIMER_NORESTART;
+
 	hrtimer_forward_now(timer, dpcm->period_time);
 	return HRTIMER_RESTART;
 }
@@ -414,14 +411,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
 	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
 	atomic_set(&dpcm->running, 0);
-	hrtimer_cancel(&dpcm->timer);
+	if (!hrtimer_callback_running(&dpcm->timer))
+		hrtimer_cancel(&dpcm->timer);
 	return 0;
 }
 
 static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
 {
 	hrtimer_cancel(&dpcm->timer);
-	tasklet_kill(&dpcm->tasklet);
 }
 
 static snd_pcm_uframes_t
@@ -466,12 +463,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
 	if (!dpcm)
 		return -ENOMEM;
 	substream->runtime->private_data = dpcm;
-	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	dpcm->timer.function = dummy_hrtimer_callback;
 	dpcm->substream = substream;
 	atomic_set(&dpcm->running, 0);
-	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
-		     (unsigned long)dpcm);
 	return 0;
 }
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1726830 — Re: [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer

On Tue, 05 Sep 2017 17:53:51 +0200,
Sebastian Andrzej Siewior wrote:
> 
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Cc: alsa-devel@alsa-project.org
> [o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
>             of hrtimer]
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> On 2017-09-02 10:19:45 [+0900], Takashi Sakamoto wrote:
> > Yep. I request the authors to include this 
> Thank you for providing a fix.
> 
> v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.
> 
> So this patch now is okay?

Note that you can try it by yourself easily, as it's a dummy driver
that doesn't need anything special.  Just run aplay for that device
(e.g. aplay -Dplughw:2 for card#2) can reproduce the original
problem.

> @@ -394,7 +386,12 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>  	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>  	if (!atomic_read(&dpcm->running))
>  		return HRTIMER_NORESTART;
> -	tasklet_schedule(&dpcm->tasklet);
> +
> +	/* In a case of XRUN, this calls .trigger to stop PCM substream. */

As mentioned, the stop happens not only with XRUN but also in a normal
situation by draining.

Other than that, looks OK to me (but not tested it).


thanks,

Takashi


> +	snd_pcm_period_elapsed(dpcm->substream);
> +	if (!atomic_read(&dpcm->running))
> +		return HRTIMER_NORESTART;
> +
>  	hrtimer_forward_now(timer, dpcm->period_time);
>  	return HRTIMER_RESTART;
>  }
> @@ -414,14 +411,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
>  	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>  
>  	atomic_set(&dpcm->running, 0);
> -	hrtimer_cancel(&dpcm->timer);
> +	if (!hrtimer_callback_running(&dpcm->timer))
> +		hrtimer_cancel(&dpcm->timer);
>  	return 0;
>  }
>  
>  static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>  {
>  	hrtimer_cancel(&dpcm->timer);
> -	tasklet_kill(&dpcm->tasklet);
>  }
>  
>  static snd_pcm_uframes_t
> @@ -466,12 +463,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
>  	if (!dpcm)
>  		return -ENOMEM;
>  	substream->runtime->private_data = dpcm;
> -	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>  	dpcm->timer.function = dummy_hrtimer_callback;
>  	dpcm->substream = substream;
>  	atomic_set(&dpcm->running, 0);
> -	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
> -		     (unsigned long)dpcm);
>  	return 0;
>  }
>  
> -- 
> 2.14.1
> 

[toc] | [prev] | [next] | [standalone]

#1726834 — Re: [PATCH 23/25 v2] ALSA/dummy: Replace tasklet with softirq hrtimer

On Sep 6 2017 00:53, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Cc: alsa-devel@alsa-project.org
> [o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
>              of hrtimer]
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> On 2017-09-02 10:19:45 [+0900], Takashi Sakamoto wrote:
>> Yep. I request the authors to include this
> Thank you for providing a fix.
> 
> v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.
> 
> So this patch now is okay?

I have a small nitpick.

>   sound/drivers/dummy.c | 23 +++++++++--------------
>   1 file changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index dd5ed037adf2..3d01fe17ed36 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
>   	ktime_t period_time;
>   	atomic_t running;
>   	struct hrtimer timer;
> -	struct tasklet_struct tasklet;
>   	struct snd_pcm_substream *substream;
>   };
>   
> -static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
> -{
> -	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
> -	if (atomic_read(&dpcm->running))
> -		snd_pcm_period_elapsed(dpcm->substream);
> -}
> -
>   static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>   {
>   	struct dummy_hrtimer_pcm *dpcm;
> @@ -394,7 +386,12 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>   	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>   	if (!atomic_read(&dpcm->running))
>   		return HRTIMER_NORESTART;
> -	tasklet_schedule(&dpcm->tasklet);
> +
> +	/* In a case of XRUN, this calls .trigger to stop PCM substream. */

As Iwai-san mentioned, in this function, .trigger can be called in two 
cases; XRUN occurs and draining is done. Thus, let you change the 
comment as 'In cases of XRUN and draining, this calls .trigger to stop 
PCM substream.'. I'm sorry to trouble you.

snd_pcm_period_elapsed()
->snd_pcm_update_hw_ptr0()
   ->snd_pcm_update_state()
     ->snd_pcm_drain_done()
       ...
       ->.trigger(TRIGGER_STOP)
     ->xrun()
       ->snd_pcm_stop()
         ...
         ->.trigger(TRIGGER_STOP)

> +	snd_pcm_period_elapsed(dpcm->substream);
> +	if (!atomic_read(&dpcm->running))
> +		return HRTIMER_NORESTART;
> +
>   	hrtimer_forward_now(timer, dpcm->period_time);
>   	return HRTIMER_RESTART;
>   }
> @@ -414,14 +411,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
>   	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>   
>   	atomic_set(&dpcm->running, 0);
> -	hrtimer_cancel(&dpcm->timer);
> +	if (!hrtimer_callback_running(&dpcm->timer))
> +		hrtimer_cancel(&dpcm->timer);
>   	return 0;
>   }
>   
>   static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>   {
>   	hrtimer_cancel(&dpcm->timer);
> -	tasklet_kill(&dpcm->tasklet);
>   }
>   
>   static snd_pcm_uframes_t
> @@ -466,12 +463,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
>   	if (!dpcm)
>   		return -ENOMEM;
>   	substream->runtime->private_data = dpcm;
> -	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>   	dpcm->timer.function = dummy_hrtimer_callback;
>   	dpcm->substream = substream;
>   	atomic_set(&dpcm->running, 0);
> -	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
> -		     (unsigned long)dpcm);
>   	return 0;
>   }


Regards

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

#1726838 — [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with softirq hrtimer

From: Thomas Gleixner <tglx@linutronix.de>

The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
callback in softirq context as well which renders the tasklet useless.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Cc: alsa-devel@alsa-project.org
[o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
            of hrtimer]
Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
On 2017-09-06 01:05:43 [+0900], Takashi Sakamoto wrote:
> As Iwai-san mentioned, in this function, .trigger can be called in two
> cases; XRUN occurs and draining is done. Thus, let you change the comment as
> 'In cases of XRUN and draining, this calls .trigger to stop PCM substream.'.
> I'm sorry to trouble you.
> 
> snd_pcm_period_elapsed()
> ->snd_pcm_update_hw_ptr0()
>   ->snd_pcm_update_state()
>     ->snd_pcm_drain_done()
>       ...
>       ->.trigger(TRIGGER_STOP)
>     ->xrun()
>       ->snd_pcm_stop()
>         ...
>         ->.trigger(TRIGGER_STOP)
> 

I think you asked me just to update the comment. Did I do it right?

v2…v3: updated the comment as per Takashi Sakamoto's suggestion.
v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.

 sound/drivers/dummy.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
index dd5ed037adf2..cdd851286f92 100644
--- a/sound/drivers/dummy.c
+++ b/sound/drivers/dummy.c
@@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
 	ktime_t period_time;
 	atomic_t running;
 	struct hrtimer timer;
-	struct tasklet_struct tasklet;
 	struct snd_pcm_substream *substream;
 };
 
-static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
-{
-	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
-	if (atomic_read(&dpcm->running))
-		snd_pcm_period_elapsed(dpcm->substream);
-}
-
 static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 {
 	struct dummy_hrtimer_pcm *dpcm;
@@ -394,7 +386,14 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
 	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
 	if (!atomic_read(&dpcm->running))
 		return HRTIMER_NORESTART;
-	tasklet_schedule(&dpcm->tasklet);
+	/*
+	 * In cases of XRUN and draining, this calls .trigger to stop PCM
+	 * substream.
+	 */
+	snd_pcm_period_elapsed(dpcm->substream);
+	if (!atomic_read(&dpcm->running))
+		return HRTIMER_NORESTART;
+
 	hrtimer_forward_now(timer, dpcm->period_time);
 	return HRTIMER_RESTART;
 }
@@ -414,14 +413,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
 	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
 
 	atomic_set(&dpcm->running, 0);
-	hrtimer_cancel(&dpcm->timer);
+	if (!hrtimer_callback_running(&dpcm->timer))
+		hrtimer_cancel(&dpcm->timer);
 	return 0;
 }
 
 static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
 {
 	hrtimer_cancel(&dpcm->timer);
-	tasklet_kill(&dpcm->tasklet);
 }
 
 static snd_pcm_uframes_t
@@ -466,12 +465,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
 	if (!dpcm)
 		return -ENOMEM;
 	substream->runtime->private_data = dpcm;
-	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
 	dpcm->timer.function = dummy_hrtimer_callback;
 	dpcm->substream = substream;
 	atomic_set(&dpcm->running, 0);
-	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
-		     (unsigned long)dpcm);
 	return 0;
 }
 
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1727151 — Re: [PATCH 23/25 v3] ALSA/dummy: Replace tasklet with softirq hrtimer

On Sep 6 2017 01:18, Sebastian Andrzej Siewior wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> The tasklet is used to defer the execution of snd_pcm_period_elapsed() to
> the softirq context. Using the CLOCK_MONOTONIC_SOFT base invokes the timer
> callback in softirq context as well which renders the tasklet useless.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-maria@linutronix.de>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Cc: alsa-devel@alsa-project.org
> [o-takashi: avoid stall due to a call of hrtimer_cancel() on a callback
>              of hrtimer]
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> On 2017-09-06 01:05:43 [+0900], Takashi Sakamoto wrote:
>> As Iwai-san mentioned, in this function, .trigger can be called in two
>> cases; XRUN occurs and draining is done. Thus, let you change the comment as
>> 'In cases of XRUN and draining, this calls .trigger to stop PCM substream.'.
>> I'm sorry to trouble you.
>>
>> snd_pcm_period_elapsed()
>> ->snd_pcm_update_hw_ptr0()
>>    ->snd_pcm_update_state()
>>      ->snd_pcm_drain_done()
>>        ...
>>        ->.trigger(TRIGGER_STOP)
>>      ->xrun()
>>        ->snd_pcm_stop()
>>          ...
>>          ->.trigger(TRIGGER_STOP)
>>
> 
> I think you asked me just to update the comment. Did I do it right?
> 
> v2…v3: updated the comment as per Takashi Sakamoto's suggestion.
> v1…v2: merged Takashi Sakamoto fixup of the original patch into v2.
> 
>   sound/drivers/dummy.c | 25 +++++++++++--------------
>   1 file changed, 11 insertions(+), 14 deletions(-)

This Looks good to me. I did quick test and confirmed that it brings no 
stalls.

Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

> diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c
> index dd5ed037adf2..cdd851286f92 100644
> --- a/sound/drivers/dummy.c
> +++ b/sound/drivers/dummy.c
> @@ -376,17 +376,9 @@ struct dummy_hrtimer_pcm {
>   	ktime_t period_time;
>   	atomic_t running;
>   	struct hrtimer timer;
> -	struct tasklet_struct tasklet;
>   	struct snd_pcm_substream *substream;
>   };
>   
> -static void dummy_hrtimer_pcm_elapsed(unsigned long priv)
> -{
> -	struct dummy_hrtimer_pcm *dpcm = (struct dummy_hrtimer_pcm *)priv;
> -	if (atomic_read(&dpcm->running))
> -		snd_pcm_period_elapsed(dpcm->substream);
> -}
> -
>   static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>   {
>   	struct dummy_hrtimer_pcm *dpcm;
> @@ -394,7 +386,14 @@ static enum hrtimer_restart dummy_hrtimer_callback(struct hrtimer *timer)
>   	dpcm = container_of(timer, struct dummy_hrtimer_pcm, timer);
>   	if (!atomic_read(&dpcm->running))
>   		return HRTIMER_NORESTART;
> -	tasklet_schedule(&dpcm->tasklet);
> +	/*
> +	 * In cases of XRUN and draining, this calls .trigger to stop PCM
> +	 * substream.
> +	 */
> +	snd_pcm_period_elapsed(dpcm->substream);
> +	if (!atomic_read(&dpcm->running))
> +		return HRTIMER_NORESTART;
> +
>   	hrtimer_forward_now(timer, dpcm->period_time);
>   	return HRTIMER_RESTART;
>   }
> @@ -414,14 +413,14 @@ static int dummy_hrtimer_stop(struct snd_pcm_substream *substream)
>   	struct dummy_hrtimer_pcm *dpcm = substream->runtime->private_data;
>   
>   	atomic_set(&dpcm->running, 0);
> -	hrtimer_cancel(&dpcm->timer);
> +	if (!hrtimer_callback_running(&dpcm->timer))
> +		hrtimer_cancel(&dpcm->timer);
>   	return 0;
>   }
>   
>   static inline void dummy_hrtimer_sync(struct dummy_hrtimer_pcm *dpcm)
>   {
>   	hrtimer_cancel(&dpcm->timer);
> -	tasklet_kill(&dpcm->tasklet);
>   }
>   
>   static snd_pcm_uframes_t
> @@ -466,12 +465,10 @@ static int dummy_hrtimer_create(struct snd_pcm_substream *substream)
>   	if (!dpcm)
>   		return -ENOMEM;
>   	substream->runtime->private_data = dpcm;
> -	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	hrtimer_init(&dpcm->timer, CLOCK_MONOTONIC_SOFT, HRTIMER_MODE_REL);
>   	dpcm->timer.function = dummy_hrtimer_callback;
>   	dpcm->substream = substream;
>   	atomic_set(&dpcm->running, 0);
> -	tasklet_init(&dpcm->tasklet, dummy_hrtimer_pcm_elapsed,
> -		     (unsigned long)dpcm);
>   	return 0;
>   }


Thanks

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web