Commit 38ea4c3d authored by Takashi Iwai's avatar Takashi Iwai
Browse files

ALSA: control: Optimize locking for look-up 



For a fast look-up of a control element via either numid or name
matching (enabled via CONFIG_SND_CTL_FAST_LOOKUP), a locking isn't
needed at all thanks to Xarray.  OTOH, the locking is still needed for
a slow linked-list traversal, and that's rather a rare case.

In this patch, we reduce the use of locking at snd_ctl_find_*() API
functions, and switch from controls_rwsem to controls_rwlock for
avoiding unnecessary lock inversions.  This also resulted in a nice
cleanup, as *_unlocked() version of snd_ctl_find_*() APIs can be
dropped.

snd_ctl_find_id_mixer_unlocked() is still left just as an alias of
snd_ctl_find_id_mixer(), since soc-card.c has a wrapper and there are
several users.  Once after converting there, we can remove it later.

Signed-off-by: default avatarTakashi Iwai <tiwai@suse.de>
Link: https://patch.msgid.link/20240809104234.8488-3-tiwai@suse.de
parent f428cc9e

include/sound/control.h

+2 −20
Original line number Diff line number Diff line
@@ -140,9 +140,7 @@ int snd_ctl_remove_id(struct snd_card * card, struct snd_ctl_elem_id *id); 
int snd_ctl_rename_id(struct snd_card * card, struct snd_ctl_elem_id *src_id, struct snd_ctl_elem_id *dst_id);

void snd_ctl_rename(struct snd_card *card, struct snd_kcontrol *kctl, const char *name);

int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, int active);

struct snd_kcontrol *snd_ctl_find_numid_locked(struct snd_card *card, unsigned int numid);

struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card, unsigned int numid);

struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card, const struct snd_ctl_elem_id *id);

struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card, const struct snd_ctl_elem_id *id);



/**
@@ -167,27 +165,11 @@ snd_ctl_find_id_mixer(struct snd_card *card, const char *name) 
	return snd_ctl_find_id(card, &id);

}



/**

 * snd_ctl_find_id_mixer_locked - find the control instance with the given name string

 * @card: the card instance

 * @name: the name string

 *

 * Finds the control instance with the given name and

 * @SNDRV_CTL_ELEM_IFACE_MIXER. Other fields are set to zero.

 *

 * This is merely a wrapper to snd_ctl_find_id_locked().

 * The caller must down card->controls_rwsem before calling this function.

 *

 * Return: The pointer of the instance if found, or %NULL if not.

 */

/* to be removed */

static inline struct snd_kcontrol *

snd_ctl_find_id_mixer_locked(struct snd_card *card, const char *name)

{

	struct snd_ctl_elem_id id = {};



	id.iface = SNDRV_CTL_ELEM_IFACE_MIXER;

	strscpy(id.name, name, sizeof(id.name));

	return snd_ctl_find_id_locked(card, &id);

	return snd_ctl_find_id_mixer(card, name);

}



int snd_ctl_create(struct snd_card *card);

include/sound/core.h

+1 −1
Original line number Diff line number Diff line
@@ -99,7 +99,7 @@ struct snd_card { 
	struct device *ctl_dev;		/* control device */

	unsigned int last_numid;	/* last used numeric ID */

	struct rw_semaphore controls_rwsem;	/* controls lock (list and values) */

	rwlock_t controls_rwlock;	/* lock for ctl_files list */

	rwlock_t controls_rwlock;	/* lock for lookup and ctl_files list */

	int controls_count;		/* count of all controls */

	size_t user_ctl_alloc_size;	// current memory allocation by user controls.

	struct list_head controls;	/* all controls for this card */

sound/core/control.c

+37 −70
Original line number Diff line number Diff line
@@ -470,7 +470,7 @@ static int __snd_ctl_add_replace(struct snd_card *card, 
	if (id.index > UINT_MAX - kcontrol->count)

		return -EINVAL;



	old = snd_ctl_find_id_locked(card, &id);

	old = snd_ctl_find_id(card, &id);

	if (!old) {

		if (mode == CTL_REPLACE)

			return -EINVAL;
@@ -491,10 +491,12 @@ static int __snd_ctl_add_replace(struct snd_card *card, 
	if (snd_ctl_find_hole(card, kcontrol->count) < 0)

		return -ENOMEM;



	scoped_guard(write_lock_irq, &card->controls_rwlock) {

		list_add_tail(&kcontrol->list, &card->controls);

		card->controls_count += kcontrol->count;

		kcontrol->id.numid = card->last_numid + 1;

		card->last_numid += kcontrol->count;

	}



	add_hash_entries(card, kcontrol);
@@ -579,12 +581,15 @@ static int __snd_ctl_remove(struct snd_card *card, 


	if (snd_BUG_ON(!card || !kcontrol))

		return -EINVAL;

	list_del(&kcontrol->list);



	if (remove_hash)

		remove_hash_entries(card, kcontrol);



	scoped_guard(write_lock_irq, &card->controls_rwlock) {

		list_del(&kcontrol->list);

		card->controls_count -= kcontrol->count;

	}



	for (idx = 0; idx < kcontrol->count; idx++)

		snd_ctl_notify_one(card, SNDRV_CTL_EVENT_MASK_REMOVE, kcontrol, idx);

	snd_ctl_free_one(kcontrol);
@@ -634,7 +639,7 @@ int snd_ctl_remove_id(struct snd_card *card, struct snd_ctl_elem_id *id) 
	struct snd_kcontrol *kctl;



	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, id);

	kctl = snd_ctl_find_id(card, id);

	if (kctl == NULL)

		return -ENOENT;

	return snd_ctl_remove_locked(card, kctl);
@@ -659,7 +664,7 @@ static int snd_ctl_remove_user_ctl(struct snd_ctl_file * file, 
	int idx;



	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, id);

	kctl = snd_ctl_find_id(card, id);

	if (kctl == NULL)

		return -ENOENT;

	if (!(kctl->vd[0].access & SNDRV_CTL_ELEM_ACCESS_USER))
@@ -691,7 +696,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, 
	int ret;



	down_write(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, id);

	kctl = snd_ctl_find_id(card, id);

	if (kctl == NULL) {

		ret = -ENOENT;

		goto unlock;
@@ -745,7 +750,7 @@ int snd_ctl_rename_id(struct snd_card *card, struct snd_ctl_elem_id *src_id, 
	int saved_numid;



	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, src_id);

	kctl = snd_ctl_find_id(card, src_id);

	if (kctl == NULL)

		return -ENOENT;

	saved_numid = kctl->id.numid;
@@ -787,6 +792,7 @@ snd_ctl_find_numid_slow(struct snd_card *card, unsigned int numid) 
{

	struct snd_kcontrol *kctl;



	guard(read_lock_irqsave)(&card->controls_rwlock);

	list_for_each_entry(kctl, &card->controls, list) {

		if (kctl->id.numid <= numid && kctl->id.numid + kctl->count > numid)

			return kctl;
@@ -796,72 +802,51 @@ snd_ctl_find_numid_slow(struct snd_card *card, unsigned int numid) 
#endif /* !CONFIG_SND_CTL_FAST_LOOKUP */



/**

 * snd_ctl_find_numid_locked - find the control instance with the given number-id

 * snd_ctl_find_numid - find the control instance with the given number-id

 * @card: the card instance

 * @numid: the number-id to search

 *

 * Finds the control instance with the given number-id from the card.

 *

 * The caller must down card->controls_rwsem before calling this function

 * (if the race condition can happen).

 *

 * Return: The pointer of the instance if found, or %NULL if not.

 *

 * Note that this function takes card->controls_rwlock lock internally.

 */

struct snd_kcontrol *

snd_ctl_find_numid_locked(struct snd_card *card, unsigned int numid)

struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card,

					unsigned int numid)

{

	if (snd_BUG_ON(!card || !numid))

		return NULL;

	lockdep_assert_held(&card->controls_rwsem);



#ifdef CONFIG_SND_CTL_FAST_LOOKUP

	return xa_load(&card->ctl_numids, numid);

#else

	return snd_ctl_find_numid_slow(card, numid);

#endif

}

EXPORT_SYMBOL(snd_ctl_find_numid_locked);



/**

 * snd_ctl_find_numid - find the control instance with the given number-id

 * @card: the card instance

 * @numid: the number-id to search

 *

 * Finds the control instance with the given number-id from the card.

 *

 * Return: The pointer of the instance if found, or %NULL if not.

 *

 * Note that this function takes card->controls_rwsem lock internally.

 */

struct snd_kcontrol *snd_ctl_find_numid(struct snd_card *card,

					unsigned int numid)

{

	guard(rwsem_read)(&card->controls_rwsem);

	return snd_ctl_find_numid_locked(card, numid);

}

EXPORT_SYMBOL(snd_ctl_find_numid);



/**

 * snd_ctl_find_id_locked - find the control instance with the given id

 * snd_ctl_find_id - find the control instance with the given id

 * @card: the card instance

 * @id: the id to search

 *

 * Finds the control instance with the given id from the card.

 *

 * The caller must down card->controls_rwsem before calling this function

 * (if the race condition can happen).

 *

 * Return: The pointer of the instance if found, or %NULL if not.

 *

 * Note that this function takes card->controls_rwlock lock internally.

 */

struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card,

struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,

				     const struct snd_ctl_elem_id *id)

{

	struct snd_kcontrol *kctl;



	if (snd_BUG_ON(!card || !id))

		return NULL;

	lockdep_assert_held(&card->controls_rwsem);



	if (id->numid != 0)

		return snd_ctl_find_numid_locked(card, id->numid);

		return snd_ctl_find_numid(card, id->numid);

#ifdef CONFIG_SND_CTL_FAST_LOOKUP

	kctl = xa_load(&card->ctl_hash, get_ctl_id_hash(id));

	if (kctl && elem_id_matches(kctl, id))
@@ -870,31 +855,13 @@ struct snd_kcontrol *snd_ctl_find_id_locked(struct snd_card *card, 
		return NULL; /* we can rely on only hash table */

#endif

	/* no matching in hash table - try all as the last resort */

	guard(read_lock_irqsave)(&card->controls_rwlock);

	list_for_each_entry(kctl, &card->controls, list)

		if (elem_id_matches(kctl, id))

			return kctl;



	return NULL;

}

EXPORT_SYMBOL(snd_ctl_find_id_locked);



/**

 * snd_ctl_find_id - find the control instance with the given id

 * @card: the card instance

 * @id: the id to search

 *

 * Finds the control instance with the given id from the card.

 *

 * Return: The pointer of the instance if found, or %NULL if not.

 *

 * Note that this function takes card->controls_rwsem lock internally.

 */

struct snd_kcontrol *snd_ctl_find_id(struct snd_card *card,

				     const struct snd_ctl_elem_id *id)

{

	guard(rwsem_read)(&card->controls_rwsem);

	return snd_ctl_find_id_locked(card, id);

}

EXPORT_SYMBOL(snd_ctl_find_id);



static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl,
@@ -1199,7 +1166,7 @@ static int snd_ctl_elem_info(struct snd_ctl_file *ctl, 
	struct snd_kcontrol *kctl;



	guard(rwsem_read)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, &info->id);

	kctl = snd_ctl_find_id(card, &info->id);

	if (!kctl)

		return -ENOENT;

	return __snd_ctl_elem_info(card, kctl, info, ctl);
@@ -1235,7 +1202,7 @@ static int snd_ctl_elem_read(struct snd_card *card, 
	int ret;



	guard(rwsem_read)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, &control->id);

	kctl = snd_ctl_find_id(card, &control->id);

	if (!kctl)

		return -ENOENT;
@@ -1303,7 +1270,7 @@ static int snd_ctl_elem_write(struct snd_card *card, struct snd_ctl_file *file, 
	int result;



	down_write(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, &control->id);

	kctl = snd_ctl_find_id(card, &control->id);

	if (kctl == NULL) {

		up_write(&card->controls_rwsem);

		return -ENOENT;
@@ -1381,7 +1348,7 @@ static int snd_ctl_elem_lock(struct snd_ctl_file *file, 
	if (copy_from_user(&id, _id, sizeof(id)))

		return -EFAULT;

	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, &id);

	kctl = snd_ctl_find_id(card, &id);

	if (!kctl)

		return -ENOENT;

	vd = &kctl->vd[snd_ctl_get_ioff(kctl, &id)];
@@ -1402,7 +1369,7 @@ static int snd_ctl_elem_unlock(struct snd_ctl_file *file, 
	if (copy_from_user(&id, _id, sizeof(id)))

		return -EFAULT;

	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, &id);

	kctl = snd_ctl_find_id(card, &id);

	if (!kctl)

		return -ENOENT;

	vd = &kctl->vd[snd_ctl_get_ioff(kctl, &id)];
@@ -1903,7 +1870,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file, 
	container_size = header.length;

	container = buf->tlv;



	kctl = snd_ctl_find_numid_locked(file->card, header.numid);

	kctl = snd_ctl_find_numid(file->card, header.numid);

	if (kctl == NULL)

		return -ENOENT;

sound/core/control_compat.c

+1 −1
Original line number Diff line number Diff line
@@ -168,7 +168,7 @@ static int get_ctl_type(struct snd_card *card, struct snd_ctl_elem_id *id, 
	int err;



	guard(rwsem_read)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, id);

	kctl = snd_ctl_find_id(card, id);

	if (!kctl)

		return -ENOENT;

	info = kzalloc(sizeof(*info), GFP_KERNEL);

sound/core/control_led.c

+1 −1
Original line number Diff line number Diff line
@@ -254,7 +254,7 @@ static int snd_ctl_led_set_id(int card_number, struct snd_ctl_elem_id *id, 
	if (!card)

		return -ENXIO;

	guard(rwsem_write)(&card->controls_rwsem);

	kctl = snd_ctl_find_id_locked(card, id);

	kctl = snd_ctl_find_id(card, id);

	if (!kctl)

		return -ENOENT;

	ioff = snd_ctl_get_ioff(kctl, id);