[4.4,3/3] ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation

Commit Message

Alexander Grund March 24, 2023, 5:18 p.m. UTC

From: Takashi Sakamoto <[e-mail]>

In ALSA control interface, applications can execute two types of request
for value of members on each element; ELEM_READ and ELEM_WRITE. In ALSA
control core, these two requests are handled within read lock of a
counting semaphore, therefore several processes can run to execute these
two requests at the same time. This has an issue because ELEM_WRITE
requests have an effect to change state of the target element. Concurrent
access should be controlled for each of ELEM_READ/ELEM_WRITE case.

This commit uses the counting semaphore as write lock for ELEM_WRITE
requests, while use it as read lock for ELEM_READ requests. The state of
a target element is maintained exclusively between ELEM_WRITE/ELEM_READ
operations.

There's a concern. If the counting semaphore is acquired for read lock
in implementations of 'struct snd_kcontrol.put()' in each driver, this
commit shall cause dead lock. As of v4.13-rc5, 'snd-mixer-oss.ko',
'snd-emu10k1.ko' and 'snd-soc-sst-atom-hifi2-platform.ko' includes codes
for read locks, but these are not in a call graph from
'struct snd_kcontrol.put(). Therefore, this commit is safe.

In current implementation, the same solution is applied for the other
operations to element; e.g. ELEM_LOCK and ELEM_UNLOCK. There's another
discussion about an overhead to maintain concurrent access to an element
during operating the other elements on the same card instance, because the
lock primitive is originally implemented to maintain a list of elements on
the card instance. There's a substantial difference between
per-element-list lock and per-element lock.

Here, let me investigate another idea to add per-element lock to maintain
the concurrent accesses with inquiry/change requests to an element. It's
not so frequent for applications to operate members on elements, while
adding a new lock primitive to structure increases memory footprint for
all of element sets somehow. Experimentally, inquiry operation is more
frequent than change operation and usage of counting semaphore for the
inquiry operation brings no blocking to the other inquiry operations. Thus
the overhead is not so critical for usual applications. For the above
reasons, in this commit, the per-element lock is not introduced.

Signed-off-by: Takashi Sakamoto <[e-mail]>
Signed-off-by: Takashi Iwai <[e-mail]>
Signed-off-by: Alexander Grund <[e-mail]>
---
 sound/core/control.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nobuhiro Iwamatsu March 29, 2023, 10:39 p.m. UTC | #1

Hi,

Thanks for your patch.

> -----Original Message-----
> From: [e-mail] <[e-mail]> On
> Behalf Of Alexander Grund
> Sent: Saturday, March 25, 2023 2:18 AM
> To: [e-mail]
> Cc: [e-mail]
> Subject: [cip-dev] [PATCH 4.4 3/3] ALSA: control: use counting semaphore as
> write lock for ELEM_WRITE operation
> 
> From: Takashi Sakamoto <[e-mail]>

Please add backports commit id. 

> 
> In ALSA control interface, applications can execute two types of request for
> value of members on each element; ELEM_READ and ELEM_WRITE. In ALSA
> control core, these two requests are handled within read lock of a counting
> semaphore, therefore several processes can run to execute these two requests
> at the same time. This has an issue because ELEM_WRITE requests have an
> effect to change state of the target element. Concurrent access should be
> controlled for each of ELEM_READ/ELEM_WRITE case.
> 
> This commit uses the counting semaphore as write lock for ELEM_WRITE
> requests, while use it as read lock for ELEM_READ requests. The state of a
> target element is maintained exclusively between ELEM_WRITE/ELEM_READ
> operations.
> 
> There's a concern. If the counting semaphore is acquired for read lock in
> implementations of 'struct snd_kcontrol.put()' in each driver, this commit shall
> cause dead lock. As of v4.13-rc5, 'snd-mixer-oss.ko', 'snd-emu10k1.ko' and
> 'snd-soc-sst-atom-hifi2-platform.ko' includes codes for read locks, but these
> are not in a call graph from 'struct snd_kcontrol.put(). Therefore, this commit is
> safe.
> 
> In current implementation, the same solution is applied for the other operations
> to element; e.g. ELEM_LOCK and ELEM_UNLOCK. There's another discussion
> about an overhead to maintain concurrent access to an element during
> operating the other elements on the same card instance, because the lock
> primitive is originally implemented to maintain a list of elements on the card
> instance. There's a substantial difference between per-element-list lock and
> per-element lock.
> 
> Here, let me investigate another idea to add per-element lock to maintain the
> concurrent accesses with inquiry/change requests to an element. It's not so
> frequent for applications to operate members on elements, while adding a new
> lock primitive to structure increases memory footprint for all of element sets
> somehow. Experimentally, inquiry operation is more frequent than change
> operation and usage of counting semaphore for the inquiry operation brings no
> blocking to the other inquiry operations. Thus the overhead is not so critical for
> usual applications. For the above reasons, in this commit, the per-element lock
> is not introduced.
> 
> Signed-off-by: Takashi Sakamoto <[e-mail]>
> Signed-off-by: Takashi Iwai <[e-mail]>
> Signed-off-by: Alexander Grund <[e-mail]>
> ---
>  sound/core/control.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c index
> 3ca81e85a1492..04b939df3768b 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -963,9 +963,9 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file
> *file,
>  	snd_power_lock(card);
>  	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
>  	if (result >= 0) {
> -		down_read(&card->controls_rwsem);
> +		down_write(&card->controls_rwsem);
>  		result = snd_ctl_elem_write(card, file, control);
> -		up_read(&card->controls_rwsem);
> +		up_write(&card->controls_rwsem);
>  	}
>  	snd_power_unlock(card);
>  	if (result >= 0)
> --
> 2.40.0

Best regards,
  Nobuhiro

Alexander Grund March 31, 2023, 3:51 p.m. UTC | #2

On Thu, Mar 30, 2023 at 12:39 AM, Nobuhiro Iwamatsu wrote:

> 
> Please add backports commit id.

Can you help me here please: What exactly should this look like?
I backported/cherry-picked this from 5bbb1ab5bd0b01c4f0b19ae03fdfec487f839517 from the linux-4.14.y branch

Do I need to submit the updated patch as a new mail? If so how'd I do that, e.g. with git send-mail?

Ulrich Hecht April 3, 2023, 8:56 a.m. UTC | #3

> On 03/31/2023 5:51 PM CEST Alexander Grund <[e-mail]> wrote:
> Do I need to submit the updated patch as a new mail? If so how'd I do that, e.g. with git send-mail? 

Normally, even for small changes, you would re-format the whole series with "git format-patch --cover-letter --subject-prefix 'PATCH v2'" and send it all again with git send-mail, adding a note to the cover letter on what has changed in this revision.

CU
Uli

Alexander Grund April 3, 2023, 5:13 p.m. UTC | #4

> 
> Please add backports commit id.

I would have added this below the first line (after an empty line):
> 
> commit 5bbb1ab5bd0b01c4f0b19ae03fdfec487f839517 upstream.

Would that have been correct? Or would something else have been required in addition?

> 
> 
> 
> Normally, even for small changes, you would re-format the whole series
> with "git format-patch --cover-letter --subject-prefix 'PATCH v2'" and
> send it all again with git send-mail, adding a note to the cover letter on
> what has changed in this revision.
> 
> 

So it isn't a problem that this opens a new (series of) threads instead of replying to the mails requesting the changes, is it?

I use `git send-email` directly (I have set sendemail.annotate=true & sendemail.confirm=always), so my command would have looked like this: `git send-email --cover-letter --subject-prefix="PATCH v2" --suppress-cc=all  --to=[e-mail] cip/linux-4.4.y-st..fix-alsa` which is basically how I created the current 4 mails.

I'm wondering if this is correct, especially as I used the prefix "PATCH 4.4" before to denote that it is meant for the 4.4.y branch. This doesn't seem to be required, so what should I use as the prefix instead for a (future) patch/patch series?

Anyway I noticed you already applied the commits fixing the mentioned issues before tagging, thanks!

I'd still appreciate a reply or a link to a page answering those question if one already exists, so the process can be smoother next time.
Thanks a lot!

Patch

diff --git a/sound/core/control.c b/sound/core/control.c
index 3ca81e85a1492..04b939df3768b 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -963,9 +963,9 @@  static int snd_ctl_elem_write_user(struct snd_ctl_file *file,
 	snd_power_lock(card);
 	result = snd_power_wait(card, SNDRV_CTL_POWER_D0);
 	if (result >= 0) {
-		down_read(&card->controls_rwsem);
+		down_write(&card->controls_rwsem);
 		result = snd_ctl_elem_write(card, file, control);
-		up_read(&card->controls_rwsem);
+		up_write(&card->controls_rwsem);
 	}
 	snd_power_unlock(card);
 	if (result >= 0)