[alsa-devel] [PATCH 0/3] ALSA: control: fix issue of concurrent access of ELEM_READ/ELEM_WRITE
Hi,
This patchset is to fix an issue of concurrent access to operate ELEM_READ/ELEM_WRITE, which I found in my previous patch:
[PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123561.html
In current implementation of ALSA control core, concurrent accesses with ELEM_READ/ELEM_WRITE are managed with counting semaphore, however lock acquisition for both of these operations is a type of 'read'. In this case, ELEM_READ can be going to be handled for a target element when ELEM_WRITE is handled for the element. A purpose of the ELEM_WRITE operation is to change state of the target element. It's better to guarantee that concurrent accesses to the same element with these two operations are properly managed.
For the above aim, this patchset uses the counting semaphore as 'write' lock for ELEM_WRITE operation. But I can imagine an discussion to add 'per-element' lock for this issue. In a comment of patch 03, I investigate it. Please refer to it.
Takashi Sakamoto (3): ALSA: control: queue events within locking of controls_rwsem for ELEM_WRITE operation ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation
sound/core/control.c | 78 +++++++++++++++++++++++++--------------------------- 1 file changed, 38 insertions(+), 40 deletions(-)
Any control event is queued by a call of snd_ctl_notify(). This function adds the event to each queue of opened file data corresponding to ALSA control character devices. This function acquired two types of lock; a counting semaphore for a list of the opened file data and a spinlock for card data opened by the file. Typically, this function is called after acquiring a counting semaphore for a list of elements in the card data.
In current implementation of a handler for ELEM_WRITE request, the function is called after releasing the semaphore for a list of elements in the card data. This release is not necessarily needed.
This commit removes the release to call the function within the critical section so that later commits are simple.
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 9e7a4571488b..79fdb366ac8d 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -948,9 +948,8 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, } if (result > 0) { struct snd_ctl_elem_id id = control->id; - up_read(&card->controls_rwsem); snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); - return 0; + result = 0; } } up_read(&card->controls_rwsem);
ALSA control core handles ELEM_READ/ELEM_WRITE requests within lock acquisition of a counting semaphore. The lock is acquired in helper functions in the end of call path before calling implementations of each driver.
ioctl(2) with SNDRV_CTL_ELEM_READ ... ->snd_ctl_ioctl() ->snd_ctl_elem_read_user() ->snd_ctl_elem_read() ->down_read(controls_rwsem) ->snd_ctl_find_id() ->struct snd_kcontrol.get() ->up_read(controls_rwsem)
ioctl(2) with SNDRV_CTL_ELEM_WRITE ... ->snd_ctl_ioctl() ->snd_ctl_elem_write_user() ->snd_ctl_elem_write() ->down_read(controls_rwsem) ->snd_ctl_find_id() ->struct snd_kcontrol.put() ->up_read(controls_rwsem)
This commit moves the lock acquisition to middle of the call graph to simplify the helper functions. As a result:
ioctl(2) with SNDRV_CTL_ELEM_READ ... ->snd_ctl_ioctl() ->snd_ctl_elem_read_user() ->down_read(controls_rwsem) ->snd_ctl_elem_read() ->snd_ctl_find_id() ->struct snd_kcontrol.get() ->up_read(controls_rwsem)
ioctl(2) with SNDRV_CTL_ELEM_WRITE ... ->snd_ctl_ioctl() ->snd_ctl_elem_write_user() ->down_read(controls_rwsem) ->snd_ctl_elem_write() ->snd_ctl_find_id() ->struct snd_kcontrol.put() ->up_read(controls_rwsem)
Signed-off-by: Takashi Sakamoto o-takashi@sakamocchi.jp --- sound/core/control.c | 77 ++++++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 39 deletions(-)
diff --git a/sound/core/control.c b/sound/core/control.c index 79fdb366ac8d..1c1fc0898afb 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -881,24 +881,18 @@ static int snd_ctl_elem_read(struct snd_card *card, struct snd_kcontrol *kctl; struct snd_kcontrol_volatile *vd; unsigned int index_offset; - int result;
- down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); - if (kctl == NULL) { - result = -ENOENT; - } else { - index_offset = snd_ctl_get_ioff(kctl, &control->id); - vd = &kctl->vd[index_offset]; - if ((vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && - kctl->get != NULL) { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->get(kctl, control); - } else - result = -EPERM; - } - up_read(&card->controls_rwsem); - return result; + if (kctl == NULL) + return -ENOENT; + + index_offset = snd_ctl_get_ioff(kctl, &control->id); + vd = &kctl->vd[index_offset]; + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_READ) && kctl->get == NULL) + return -EPERM; + + snd_ctl_build_ioff(&control->id, kctl, index_offset); + return kctl->get(kctl, control); }
static int snd_ctl_elem_read_user(struct snd_card *card, @@ -913,8 +907,11 @@ static int snd_ctl_elem_read_user(struct snd_card *card,
snd_power_lock(card); result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result >= 0) + if (result >= 0) { + down_read(&card->controls_rwsem); result = snd_ctl_elem_read(card, control); + up_read(&card->controls_rwsem); + } snd_power_unlock(card); if (result >= 0) if (copy_to_user(_control, control, sizeof(*control))) @@ -931,29 +928,28 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, unsigned int index_offset; int result;
- down_read(&card->controls_rwsem); kctl = snd_ctl_find_id(card, &control->id); - if (kctl == NULL) { - result = -ENOENT; - } else { - index_offset = snd_ctl_get_ioff(kctl, &control->id); - vd = &kctl->vd[index_offset]; - if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || - kctl->put == NULL || - (file && vd->owner && vd->owner != file)) { - result = -EPERM; - } else { - snd_ctl_build_ioff(&control->id, kctl, index_offset); - result = kctl->put(kctl, control); - } - if (result > 0) { - struct snd_ctl_elem_id id = control->id; - snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); - result = 0; - } + if (kctl == NULL) + return -ENOENT; + + index_offset = snd_ctl_get_ioff(kctl, &control->id); + vd = &kctl->vd[index_offset]; + if (!(vd->access & SNDRV_CTL_ELEM_ACCESS_WRITE) || kctl->put == NULL || + (file && vd->owner && vd->owner != file)) { + return -EPERM; } - up_read(&card->controls_rwsem); - return result; + + snd_ctl_build_ioff(&control->id, kctl, index_offset); + result = kctl->put(kctl, control); + if (result < 0) + return result; + + if (result > 0) { + struct snd_ctl_elem_id id = control->id; + snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &id); + } + + return 0; }
static int snd_ctl_elem_write_user(struct snd_ctl_file *file, @@ -970,8 +966,11 @@ static int snd_ctl_elem_write_user(struct snd_ctl_file *file, card = file->card; snd_power_lock(card); result = snd_power_wait(card, SNDRV_CTL_POWER_D0); - if (result >= 0) + if (result >= 0) { + down_read(&card->controls_rwsem); result = snd_ctl_elem_write(card, file, control); + up_read(&card->controls_rwsem); + } snd_power_unlock(card); if (result >= 0) if (copy_to_user(_control, control, sizeof(*control)))
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 o-takashi@sakamocchi.jp --- 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 1c1fc0898afb..249140c15d64 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -967,9 +967,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)
On Sun, 20 Aug 2017 06:49:05 +0200, Takashi Sakamoto wrote:
Hi,
This patchset is to fix an issue of concurrent access to operate ELEM_READ/ELEM_WRITE, which I found in my previous patch:
[PATCH 0/5] ALSA: control: fix issue of concurrent access for TLV operations http://mailman.alsa-project.org/pipermail/alsa-devel/2017-August/123561.html
In current implementation of ALSA control core, concurrent accesses with ELEM_READ/ELEM_WRITE are managed with counting semaphore, however lock acquisition for both of these operations is a type of 'read'. In this case, ELEM_READ can be going to be handled for a target element when ELEM_WRITE is handled for the element. A purpose of the ELEM_WRITE operation is to change state of the target element. It's better to guarantee that concurrent accesses to the same element with these two operations are properly managed.
For the above aim, this patchset uses the counting semaphore as 'write' lock for ELEM_WRITE operation. But I can imagine an discussion to add 'per-element' lock for this issue. In a comment of patch 03, I investigate it. Please refer to it.
Takashi Sakamoto (3): ALSA: control: queue events within locking of controls_rwsem for ELEM_WRITE operation ALSA: control: code refactoring for ELEM_READ/ELEM_WRITE operations ALSA: control: use counting semaphore as write lock for ELEM_WRITE operation
Applied all three patches. Thanks.
Takashi
participants (2)
-
Takashi Iwai -
Takashi Sakamoto