On Sun, 15 May 2016 20:32:09 +0200,
Alexander E. Patrakov wrote:
>
> 13.05.2016 20:04, Takashi Iwai пишет:
> > On Fri, 13 May 2016 16:07:12 +0200,
> > Takashi Iwai wrote:
> >>
> >> On Thu, 12 May 2016 15:45:23 +0200,
> >> Takashi Iwai wrote:
> >>>
> >>> On Tue, 10 May 2016 10:40:08 +0200,
> >>> Alexander E. Patrakov wrote:
> >>>>
> >>>> 09.05.2016 17:44, Takashi Iwai wrote:
> >>>>> On Sun, 01 May 2016 14:43:42 +0200,
> >>>>> Alexander E. Patrakov wrote:
> >>>>>> Even if no files have been changed, the config may become outdated
> >>>>>> due to hot-plugged devices.
> >>>>>>
> >>>>>> Note: this patch has a huge potential to crash badly-written applications
> >>>>>> that keep config pointers and strings for too long. But I see no other
> >>>>>> way to support hot-pluggable devices.
> >>>>> Can we opt in this feature, e.g. by enabling it via some new API
> >>>>> function? Enabling this blindly appears too risky.
> >>>>
> >>>> Hello Takashi,
> >>>>
> >>>> I have read your remark, and your concern does make sense. However, I
> >>>> have some doubts whether any new API is needed, and some suggestions
> >>>> for further changes. Here are my ideas.
> >>>>
> >>>> 1. We already have such API. It is called snd_config_top(),
> >>>> snd_config_update_r() and snd_pcm_open_lconf(). Would you accept a
> >>>> documentation patch that says that snd_pcm_open cannot deal with
> >>>> hot-plugged devices, and directs users to snd_pcm_open_lconf()? Of
> >>>> course, this depends on Tanu's willingness to accept conversion to
> >>>> snd_pcm_open_lconf() in PulseAudio.
> >>>
> >>> Yeah, it'd be good to mention there.
> >>>
> >>>> 2. Even if a user calls snd_pcm_open_lconf(), currently, when
> >>>> attempting to figure out the driver, mixer configuration is updated
> >>>> with snd_config_update() - i.e. global configuration is updated, and
> >>>> this IMHO defeats the point of having a separate configuration. To fix
> >>>> the bug, I would need to make sure that the mixer configuration is
> >>>> read from the same snd_config_t structure that was passed to
> >>>> snd_pcm_open_lconf(). I am not sure about the difficulty of this task.
> >>>>
> >>>> 3. One of my early attempts of the patch (without the hunk about
> >>>> snd_ctl_open_lconf()) was crashing due to bad pointers. Now I realize
> >>>> that it is already a bug in alsa-lib - the current code would crash,
> >>>> too, if the configuration file has changed between the calls to
> >>>> snd_pcm_update() in snd_pcm_open() and in snd_ctl_open() (which is
> >>>> called from a hook that determines the driver). Again, I would need to
> >>>> make sure that the mixer configuration is read from the same
> >>>> snd_config_t structure that was passed to snd_pcm_open_noupdate(), see
> >>>> above.
> >>>
> >>> Right. The crash doesn't happen so often because the update of config
> >>> tree doesn't happen also so often.
> >>>
> >>>> 4. I still think that the file mtime check in snd_config_update_r does
> >>>> not make any sense. It fails to notice changes in included files, as
> >>>> well as in the list of plugged-in devices. In other words, it never
> >>>> worked except for the initial read, and it's too risky to make it
> >>>> actually work as documented. How about making it read the
> >>>> configuration from files only on the first call for a given config,
> >>>> even if they have changed?
> >>>
> >>> Yes, that sounds more reasonable as a first move. It doesn't work
> >>> 100% reliably, and if you really want a proper update, you can do it
> >>> by calling snd_config_update_free_global() beforehand.
> >>>
> >>> There is a minimal protection by pthread mutex in conf.c, but it's
> >>> only at regenerating the config tree. We need refcounting by
> >>> referrer for a proper protection. Once after having the refcount
> >>> protection, we can safely update the config tree unconditionally.
> >>
> >> So here is a quick hack. It adds a simple refcount in snd_config_t.
> >> snd_config_update_get() gives a top config with a refcount so that it
> >> won't be deleted until disposed by snd_config_put().
> >>
> >> In this patch, the config update itself is kept in traditional way,
> >> i.e. lazy update with mtime check. For the forced refresh, I added
> >> snd_config_refresh() which removes snd_config_global_update so that at
> >> the next snd_config_update() will reread properly. PA can call this
> >> function appropriately, e.g. when hotplugged.
> >>
> >> The patch is just a proof of concept and even untested. Once when
> >> confirmed that it's the way to go, I'll cook up more.
> >
> > There was a typo indeed. The revised patch is below.
> > aplay worked at least :) And now it has comments.
>
> I have read the patch, and also tested with aplay, speaker-test and
> pulseaudio.
>
> Here are my thoughts so far.
>
> First, the patch combines two separate logical changes: proper locking
> and refcounting.
>
> Locking looks almost OK (one small issue with snd_config_delete() and
> two "maybe" suggestions, see below). The mutex is recursive, so
> e.g. calling snd_ctl_open() from PCM hooks (and thus from inside
> snd_pcm_open()) is not a problem.
There is nothing new in regard of locking. It existed before the
patch, snd_config_update() and snd_config_update_free_global() already
took it. So, this must be pretty safe even in the new code.
> Regarding the new snd_config_refresh() API, do we really need it?
> There are precedents of applications (e.g. Phonon) using
> snd_config_update_free_global() for the same purpose. The only new
> thing that snd_config_refresh() brings is the guarantee that it either
> does not exist or does proper locking.
OK, this can be removed. I thought it would be nicer, but reducing
the unneeded API would be better indeed.
> It may be, instead, a good idea to add the same kind of locking to
> snd_config_update_free_global(). And maybe to snd_config_update()?
> Ideally, I would like to be able to safely call
> snd_config_update_free_global() at any time in a multithreaded
> application that does not call any other snd_config_*() functions
> directly.
As said, snd_config_update() and snd_config_update_free_global() have
already locks and they can be called safely. The problem is that the
tree itself that is being used in snd_*_open() (i.e. snd_config global
tree) isn't race-free. The introduction of refcount helps for that.
> Now to refcounting.
>
> The new functions, snd_config_update_get()/snd_config_put(), as a new
> public API, have received zero testing from me. With this patch,
> snd_config_delete() will be called sometimes on snd_config, but so far
> in my testing with aplay and speaker-test the relevant refcount is >
> 0. So it looks like there is no danger for existing applications.
>
> As snd_config_delete() is still public API, it looks like there is a
> (bad) code path that accesses refcount without locking. Fixing it
> would require adding a lock to snd_config_delete(), but then what's
> the point of snd_config_put()?
The difference is that snd_config_delete() can't be protected with
mutex, as this function itself is called recursively from the
destructor in the mutex. So, we may see snd_config_delete() as a
function to delete the local tree while snd_config_put() (or maybe
better named as snd_config_unref() or such) is a function to
unreference (and eventually delete) the tree in a thread safe way.
Takashi
> I have also investigated the hooks PCM specifically for the question
> whether it is safe to refresh the config while such PCM is open, and
> found that, eventually, the control name is copied. This is done in
> snd_ctl_elem_id_set_name. So this particular PCM type seems to be
> safe.
>
> Also, I have tried a patched pulseaudio. The patch adds a call to
> snd_config_refresh() in pa_alsa_open_by_device_string. It now
> interacts with a hot-plugged USB device successfully. But so it does
> if I call snd_config_update_free_global() (except for locking).
>
> And I still think that I need to do come more testing...
>
> --
> Alexander E. Patrakov
>
> >
> >
> > Takashi
> >
> > ---
> > diff --git a/include/conf.h b/include/conf.h
> > index 087c05dc6bcf..72fd7c06a8d9 100644
> > --- a/include/conf.h
> > +++ b/include/conf.h
> > @@ -94,6 +94,10 @@ int snd_config_update_r(snd_config_t **top, snd_config_update_t **update, const
> > int snd_config_update_free(snd_config_update_t *update);
> > int snd_config_update_free_global(void);
> >
> > +void snd_config_refresh(void);
> > +int snd_config_update_get(snd_config_t **top);
> > +void snd_config_put(snd_config_t *top);
> > +
> > int snd_config_search(snd_config_t *config, const char *key,
> > snd_config_t **result);
> > int snd_config_searchv(snd_config_t *config,
> > diff --git a/src/conf.c b/src/conf.c
> > index f8b7a6686529..1386a38b0b27 100644
> > --- a/src/conf.c
> > +++ b/src/conf.c
> > @@ -434,6 +434,7 @@ static pthread_once_t snd_config_update_mutex_once = PTHREAD_ONCE_INIT;
> > struct _snd_config {
> > char *id;
> > snd_config_type_t type;
> > + int refcount; /* default = 0 */
> > union {
> > long integer;
> > long long integer64;
> > @@ -1833,6 +1834,10 @@ int snd_config_remove(snd_config_t *config)
> > int snd_config_delete(snd_config_t *config)
> > {
> > assert(config);
> > + if (config->refcount > 0) {
> > + config->refcount--;
> > + return 0;
> > + }
> > switch (config->type) {
> > case SND_CONFIG_TYPE_COMPOUND:
> > {
> > @@ -3851,6 +3856,60 @@ int snd_config_update(void)
> > return err;
> > }
> >
> > +/**
> > + * \brief Clear the local config update cache
> > + *
> > + * This function clears the local config update cache to guarantee that
> > + * the next call of #snd_config_update will reread the config files.
> > + */
> > +void snd_config_refresh(void)
> > +{
> > + snd_config_lock();
> > + if (snd_config_global_update)
> > + snd_config_update_free(snd_config_global_update);
> > + snd_config_global_update = NULL;
> > + snd_config_unlock();
> > +}
> > +
> > +/**
> > + * \brief Updates #snd_config and take its reference.
> > + * \return 0 if #snd_config was up to date, 1 if #snd_config was
> > + * updated, otherwise a negative error code.
> > + *
> > + * Unlike #snd_config_update, this function increases a reference counter
> > + * so that the obtained tree won't be deleted until unreferenced by
> > + * #snd_config_put.
> > + */
> > +int snd_config_update_get(snd_config_t **top)
> > +{
> > + int err;
> > +
> > + *top = NULL;
> > + snd_config_lock();
> > + err = snd_config_update_r(&snd_config, &snd_config_global_update, NULL);
> > + if (err >= 0) {
> > + *top = snd_config;
> > + if (*top)
> > + (*top)->refcount++;
> > + }
> > + snd_config_unlock();
> > + return err;
> > +}
> > +
> > +/**
> > + * \brief Unreference the config tree.
> > + *
> > + * Decreases a reference counter of the given config tree. This is the
> > + * counterpart of #snd_config_update_get.
> > + */
> > +void snd_config_put(snd_config_t *cfg)
> > +{
> > + snd_config_lock();
> > + if (cfg)
> > + snd_config_delete(cfg);
> > + snd_config_unlock();
> > +}
> > +
> > /**
> > * \brief Frees a private update structure.
> > * \param[in] update The private update structure to free.
> > diff --git a/src/control/control.c b/src/control/control.c
> > index 8a5d530f2674..6078189a6a70 100644
> > --- a/src/control/control.c
> > +++ b/src/control/control.c
> > @@ -968,12 +968,16 @@ static int snd_ctl_open_noupdate(snd_ctl_t **ctlp, snd_config_t *root, const cha
> > */
> > int snd_ctl_open(snd_ctl_t **ctlp, const char *name, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(ctlp && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_ctl_open_noupdate(ctlp, snd_config, name, mode);
> > + err = snd_ctl_open_noupdate(ctlp, top, name, mode);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/hwdep/hwdep.c b/src/hwdep/hwdep.c
> > index 5dc791c99189..c8b368fa68a2 100644
> > --- a/src/hwdep/hwdep.c
> > +++ b/src/hwdep/hwdep.c
> > @@ -168,12 +168,16 @@ static int snd_hwdep_open_noupdate(snd_hwdep_t **hwdep, snd_config_t *root, cons
> > */
> > int snd_hwdep_open(snd_hwdep_t **hwdep, const char *name, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(hwdep && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_hwdep_open_noupdate(hwdep, snd_config, name, mode);
> > + err = snd_hwdep_open_noupdate(hwdep, top, name, mode);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/pcm/pcm.c b/src/pcm/pcm.c
> > index 203e7a52491b..42da2c8a8b55 100644
> > --- a/src/pcm/pcm.c
> > +++ b/src/pcm/pcm.c
> > @@ -2288,12 +2288,16 @@ static int snd_pcm_open_noupdate(snd_pcm_t **pcmp, snd_config_t *root,
> > int snd_pcm_open(snd_pcm_t **pcmp, const char *name,
> > snd_pcm_stream_t stream, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(pcmp && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_pcm_open_noupdate(pcmp, snd_config, name, stream, mode, 0);
> > + err = snd_pcm_open_noupdate(pcmp, top, name, stream, mode, 0);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/rawmidi/rawmidi.c b/src/rawmidi/rawmidi.c
> > index 0c89b8b984b9..d965edaf1f42 100644
> > --- a/src/rawmidi/rawmidi.c
> > +++ b/src/rawmidi/rawmidi.c
> > @@ -305,12 +305,16 @@ static int snd_rawmidi_open_noupdate(snd_rawmidi_t **inputp, snd_rawmidi_t **out
> > int snd_rawmidi_open(snd_rawmidi_t **inputp, snd_rawmidi_t **outputp,
> > const char *name, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert((inputp || outputp) && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_rawmidi_open_noupdate(inputp, outputp, snd_config, name, mode);
> > + err = snd_rawmidi_open_noupdate(inputp, outputp, top, name, mode);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/seq/seq.c b/src/seq/seq.c
> > index 4405e68a7fe9..c1aa7981d557 100644
> > --- a/src/seq/seq.c
> > +++ b/src/seq/seq.c
> > @@ -974,12 +974,16 @@ static int snd_seq_open_noupdate(snd_seq_t **seqp, snd_config_t *root,
> > int snd_seq_open(snd_seq_t **seqp, const char *name,
> > int streams, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(seqp && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_seq_open_noupdate(seqp, snd_config, name, streams, mode, 0);
> > + err = snd_seq_open_noupdate(seqp, top, name, streams, mode, 0);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/timer/timer.c b/src/timer/timer.c
> > index a25e4f797ce4..6af6d8224df1 100644
> > --- a/src/timer/timer.c
> > +++ b/src/timer/timer.c
> > @@ -201,12 +201,16 @@ static int snd_timer_open_noupdate(snd_timer_t **timer, snd_config_t *root, cons
> > */
> > int snd_timer_open(snd_timer_t **timer, const char *name, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(timer && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_timer_open_noupdate(timer, snd_config, name, mode);
> > + err = snd_timer_open_noupdate(timer, top, name, mode);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> > diff --git a/src/timer/timer_query.c b/src/timer/timer_query.c
> > index 93d2455d07fc..23bf0711aafe 100644
> > --- a/src/timer/timer_query.c
> > +++ b/src/timer/timer_query.c
> > @@ -159,12 +159,16 @@ static int snd_timer_query_open_noupdate(snd_timer_query_t **timer, snd_config_t
> > */
> > int snd_timer_query_open(snd_timer_query_t **timer, const char *name, int mode)
> > {
> > + snd_config_t *top;
> > int err;
> > +
> > assert(timer && name);
> > - err = snd_config_update();
> > + err = snd_config_update_get(&top);
> > if (err < 0)
> > return err;
> > - return snd_timer_query_open_noupdate(timer, snd_config, name, mode);
> > + err = snd_timer_query_open_noupdate(timer, top, name, mode);
> > + snd_config_put(top);
> > + return err;
> > }
> >
> > /**
> >
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
