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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c73da5fd-51a9-4744-9687-0ebc56c34fca@wanadoo.fr>
Date: Sun, 28 Jul 2024 12:29:51 +0200
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To: Ivan Orlov <ivan.orlov0322@...il.com>, perex@...ex.cz, tiwai@...e.com,
 corbet@....net, broonie@...nel.org, shuah@...nel.org
Cc: linux-kselftest@...r.kernel.org, linux-doc@...r.kernel.org,
 linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org, aholzinger@....de
Subject: Re: [PATCH 3/4] ALSA: timer: Introduce virtual userspace-driven
 timers

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@....de>
>>>>> Signed-off-by: Ivan Orlov <ivan.orlov0322@...il.com>
>>>>> ---
>>>>
>>>> ...
>>>>
>>>>> +#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