Re: [PATCH 3/4] ALSA: timer: Introduce virtual userspace-driven timers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Le 28/07/2024 à 11:42, Ivan Orlov a écrit :
On 7/28/24 10:30, Christophe JAILLET wrote:
Le 28/07/2024 à 10:51, Ivan Orlov a écrit :
On 7/28/24 07:59, Christophe JAILLET wrote:
Le 26/07/2024 à 09:47, Ivan Orlov a écrit :
Implement two ioctl calls in order to support virtual userspace-driven
ALSA timers.

The first ioctl is SNDRV_TIMER_IOCTL_CREATE, which gets the
snd_utimer_info struct as a parameter and returns a file descriptor of
a virtual timer. It also updates the `id` field of the snd_utimer_info
struct, which provides a unique identifier for the timer (basically,
the subdevice number which can be used when creating timer instances).

This patch also introduces a tiny id allocator for the userspace-driven timers, which guarantees that we don't have more than 128 of them in the
system.

Another ioctl is SNDRV_TIMER_IOCTL_TRIGGER, which allows us to trigger
the virtual timer (and calls snd_timer_interrupt for the timer under
the hood), causing all of the timer instances binded to this timer to
execute their callbacks.

The maximum amount of ticks available for the timer is 1 for the sake of simplification of the userspace API. 'start', 'stop', 'open' and 'close'
callbacks for the userspace-driven timers are empty since we don't
really do any hardware initialization here.

Suggested-by: Axel Holzinger <aholzinger@xxxxxx>
Signed-off-by: Ivan Orlov <ivan.orlov0322@xxxxxxxxx>
---

...

+#ifdef CONFIG_SND_UTIMER
+/*
+ * Since userspace-driven timers are passed to userspace, we need to have an identifier + * which will allow us to use them (basically, the subdevice number of udriven timer).
+ *
+ * We have a pool of SNDRV_UTIMERS_MAX_COUNT ids from 0 to (SNDRV_UTIMERS_MAX_COUNT - 1). + * When we take one of them, the corresponding entry in snd_utimer_ids becomes true.
+ */
+static bool snd_utimer_ids[SNDRV_UTIMERS_MAX_COUNT];
+
+static void snd_utimer_put_id(struct snd_utimer *utimer)
+{
+    int timer_id = utimer->id;
+
+    snd_BUG_ON(timer_id < 0 || timer_id >= SNDRV_UTIMERS_MAX_COUNT);
+    snd_utimer_ids[timer_id] = false;
+}
+
+static int snd_utimer_take_id(void)
+{
+    size_t i;
+
+    for (i = 0; i < SNDRV_UTIMERS_MAX_COUNT; i++) {
+        if (!snd_utimer_ids[i]) {
+            snd_utimer_ids[i] = true;
+            return i;
+        }
+    }
+
+    return -EBUSY;
+}

Also the bitmap API could be useful here.


Awesome, will use it in V2.

Hmm, maybe DEFINE_IDA(), ida_alloc_max() and ida_free() would be even better.


It looks like IDA allocator uses XArrays under the hood to allocate ids between 0 and INT_MAX... Considering the fact, that we currently could have up to 128 userspace-driven timers in the system, using XArrays seems a bit redundant, and I believe bitmap approach would be more efficient. What do you think?


I may be wrong but I think that ida allocates hunks for 1024 bits (128 bytes * 8) at a time. (see [1])

So with this extra sape and the sapce for the xarray, it would waste a few bytes of memory, yes.

With ida, there is also some locking that may be unnecessary (but harmless)


Hoping, I got it right, here are a few numbers:

On a x86_64, with allmodconfig:

Your initial patch:
   text	   data	    bss	    dec	    hex	filename
  55020	   1783	    268	  57071	   deef	sound/core/timer.o

With ida:
  54763	   1631	    116	  56510	   dcbe	sound/core/timer.o
+ 128 bytes of runtime memory allocation

With bitmap:
  54805	   1535	    132	  56472	   dc98	sound/core/timer.o


I think that the code would be slightly more elegant with ida, but implementing it with a bitmap does not add that much complexity.

CJ


[1]: https://elixir.bootlin.com/linux/v6.10.2/source/include/linux/idr.h#L238




[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux