Re: [PATCH] ALSA: control: expand limitation on the number of user-defined control element set per card

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

 



On Sun, 24 Jan 2021 06:52:25 +0100,
Takashi Sakamoto wrote:
> 
> 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.

Right, that should be taken into account.

> ```
> 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

Forget my previous patch.  The code change suggested there is
obviously not sufficient, but you need only consider about its idea.

> 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
> ```

See that your patch increases the memory consumption so much?
That's my point.

We may give more user elements without increasing the worst-case
memory footprint in normal scenarios.  So scratch MAX_USER_CONTROLS
but count each user element's memory consumption and define its total
limit instead.


Takashi



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Pulse Audio]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux