Hi,
On Sat, Jan 23, 2021 at 10:10:20AM +0100, Takashi Iwai wrote:
On Sat, 23 Jan 2021 04:10:25 +0100, Takashi Sakamoto wrote:
Hi,
On Fri, Jan 22, 2021 at 03:04:56PM +0100, Takashi Iwai wrote:
On Fri, 22 Jan 2021 09:20:32 +0100, Takashi Sakamoto wrote:
ALSA control core allows usespace application to register control element set by call of ioctl(2) with SNDRV_CTL_IOCTL_ELEM_ADD request. The added control elements are called as 'user-defined'. Currently sound card has limitation on the number of the user-defined control element set up to 32.
The limitation is inconvenient to drivers in ALSA firewire stack since the drivers expect userspace applications to implement function to control device functionalities such as mixing and routing. As the userspace application, snd-firewire-ctl-services project starts: https://github.com/alsa-project/snd-firewire-ctl-services/
The project supports many devices supported by ALSA firewire stack. The limitation is mostly good since routing and mixing controls can be represented by control element set, which includes multiple control element with the same parameters. Nevertheless, it's actually inconvenient to device which has many varied functionalities. For example, plugin effect such as channel strip and reverb has many parameters. For the case, many control elements are required to configure the parameters and control element set cannot aggregates them for the parameters. At present, the implementations for below models requires more control element sets than 32:
- snd-bebob-ctl-service
- Apogee Ensemble (31 sets for 34 elements)
- snd-dice-ctl-service
- TC Electronic Konnekt 24d (78 sets for 94 elements)
- TC Electronic Studio Konnekt 48 (98 sets for 114 elements)
- TC Electronic Konnekt Live (88 sets for 104 elements)
- TC Electronic Impact Twin (70 sets for 86 elements)
- Focusrite Liquid Saffire 56 (37 sets for 52 elements)
This commit expands the limitation according to requirement from the above applications. As a result, userspace applications can add control element sets up to 150 per sound card. It results in 154,200 user-defined control elements as maximum since one control element set can include 1028 control elements.
Thinking of this change again after reading your description, I find that a more flexible and safer approach would be to limit the number of total elements. That is, count the number of items in each element, and set the max to (32 * MAX_CONTROL_COUNT). This will keep the same max as the current implementation can achieve, while it allows more elements as long as they contain lower number of items.
So, something like below (totally untested).
thanks,
Takashi
--- a/sound/core/control.c +++ b/sound/core/control.c @@ -18,10 +18,11 @@ #include <sound/info.h> #include <sound/control.h>
-/* max number of user-defined controls */ -#define MAX_USER_CONTROLS 32 #define MAX_CONTROL_COUNT 1028
+/* max number of user-defined controls */ +#define MAX_USER_CONTROLS (32 * MAX_CONTROL_COUNT)
struct snd_kctl_ioctl { struct list_head list; /* list of all ioctls */ snd_kctl_ioctl_func_t fioctl; @@ -520,6 +521,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, struct snd_card *card = file->card; struct snd_kcontrol *kctl; int idx, ret;
int count;
down_write(&card->controls_rwsem); kctl = snd_ctl_find_id(card, id);
@@ -536,10 +538,11 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, ret = -EBUSY; goto error; }
- count = kctl->count; ret = snd_ctl_remove(card, kctl); if (ret < 0) goto error;
- card->user_ctl_count--;
- card->user_ctl_count -= count;
error: up_write(&card->controls_rwsem); return ret; @@ -1435,18 +1438,18 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, return err; }
- /* Check the number of elements for this userspace control. */
- count = info->owner;
- if (count == 0)
count = 1;- /*
*/
- The number of userspace controls are counted control by control,
- not element by element.
- if (card->user_ctl_count + 1 > MAX_USER_CONTROLS)
- if (card->user_ctl_count + count > MAX_USER_CONTROLS) return -ENOMEM;
- /* Check the number of elements for this userspace control. */
- count = info->owner;
- if (count == 0)
count = 1;- /* Arrange access permissions if needed. */ access = info->access; if (access == 0)
@@ -1535,7 +1538,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, * which locks the element. */
- card->user_ctl_count++;
card->user_ctl_count += count;
unlock: up_write(&card->controls_rwsem);
I have no objection to any change as long as it allows the service programs to add control elements enough for target device. However, it's unclear what is flexible and safe in the above patch.
When adding user-defined control element set, some objects are allocated for below structures with some variable-length members:
- struct snd_kcontrol (in include/sound/control.h)
- struct user_element (in sound/core/control.h)
Current implementation is to avoid too much allocation for the above against userspace applications with bugs or mis-programming. It's reasonable to limit the allocation according to count of added control element set for the purpose.
On the other hand, when counting the number of added control element for the limitation, the above becomes loose. In the worst case, the patch allows 32,896 sets to be allocated and against comments in my previous patch.
OK, my previous patch was too simplified (I forgot to take the private_data into account), but the point is that what we want is to cap the worst case memory consumption.
If I calculate correctly, user_element is 320 bytes, and the value is up to 512 bytes for each item, and snd_kcontrol is 144 bytes, and snd_kcontrol_volatile is 16 bytes per item. And each element may contain 1028 items. So, the worst case scenario of the memory consumption is: (320 + 512*1028 + 144 + 16*1024) * 32 = 17383936 That is, currently we allow 16MB at most.
By increasing MAX_USER_CONTROLS to 150, it'll become 77MB.
And, think what if you'd need to increase more in future, e.g. 512 elements. The max consumption also increases linearly.
OTOH, imagine that we cap the memory consumption to 16MB instead of limiting only the MAX_USER_CONTROLS. This lets user still allow to allocate more elements with smaller number of items (that is the common use case). In this way, we don't take a risk of higher memory consumption while user can deploy the user elements more flexibly.
The memory object for data of Type-Length-Value style is underestimate in your calculation for the worst case. For user-defined control element set, the size is (1024 * 128) per control element set[1].
Of cource, it's possible to judge that usual userspace application don't use data of Type-Length-Value style so much. However, we are assuming the worst case now.
``` Objects linearly increased according to the number of user-defined control element sets: * struct snd_kcontrol (144 bytes) * struct user_element (320 bytes) * data of TLV ((1024 * 128) bytes as maximum)
Objects linearly increased according to the number of control elements: * data of values (max. 1024 bytes in System V ABI with LP64 data type) * data of snd_kcontrol_volatile (16 bytes)
Memory consumption under current limitation: (144 + 320 + 1024 * 128) * 32 + (1024 + 16) * 1028 = 5,278,272
Scenario for the worst case when appying the patch: * adding 32,896 control element sets * for elements: (1024 + 16) * 1028 * 32 = 34,211,840 * for element sets: (144 + 320 + 1024 * 128) * 32896 = 4,327,008,256
Scenario for the worst case when applying my patch: * adding 150 control element sets * for elements: (1024 + 16) * 1028 * 150 = 160,368,000 * for element sets: (144 + 320 + 1024 * 128) * 150 = 19,730,400 ```
[1] https://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound.git/tree/sound/c...
Regards
Takashi Sakamoto