Re: [PATCH] alsa-lib:Make snd_atomic_write_* truly atomic

[Takashi Iwai <tiwai@suse.de>]

On Thu, 12 May 2016 14:51:07 +0200,
Ricard Wanderlof wrote:
> 
> 
> On Wed, 13 Apr 2016, Takashi Iwai wrote:
> 
> > On Wed, 13 Apr 2016 12:42:01 +0200,
> > Andreas x Larsson wrote:
> > > 
> > > Under some usecases a race condition appears inside
> > > the snd_atomic_write_* functions. The 'begin' and 'end'
> > > variables are updated with the ++ operator which is not
> > > atomic but needs to be. This can be achieved with the
> > > gcc atomic_* built-ins. Combined with __ATOMIC_SEQ_CST
> > > as the memory model, memory barriers are introduced so
> > > those can also be removed from the code.
> > > 
> > > Signed-off-by: Andreas x Larsson <andrelan@axis.com>
> > > Signed-off-by: Arvid Nihlgård Lindell <arvidnl@axis.com>
> > 
> > Isn't this dependent on gcc version?
> 
> Do you mean that the problem is dependent on gcc version, or the solution? 
> If the problem only occurs with certain gcc versions, but the _atomic_* 
> functions are the proper solution in all cases, then I would think that is 
> the way to go.

I meant the solution, your patch.  Does it work with older version of
gcc, and any other compilers than gcc?

> > And, if we use the gcc atomic operation, the whole macros should be 
> > rewritten in another way -- or consider to drop the whole. It's only 
> > partly used in pcm_rate.c and pcm_plugin.c in anyway.
> 
> Do you mean that the inline functions should be replaced with
> #define macros, or that they should be omitted entirely and the resulting 
> code be integrated into the relevant places in the .c files?

No, no, I questioned about the usefulness of snd_atomic_*()
themselves.  Does it make sense to provide this thin wrapper?
And, above all, does it make sense at all to use this?  When looking
at the current code, it's used in only very limited places.


Takashi

> 
> /Ricard
> 
> > 
> > > ---
> > >   alsa-lib/include/iatomic.h | 6 ++----
> > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/alsa-lib/include/iatomic.h b/alsa-lib/include/iatomic.h
> > > index acdd3e2..7fafe20 100644
> > > --- a/alsa-lib/include/iatomic.h
> > > +++ b/alsa-lib/include/iatomic.h
> > > @@ -140,14 +140,12 @@ static __inline__ void 
> > > snd_atomic_write_init(snd_atomic_write_t *w)
> > > 
> > >   static __inline__ void snd_atomic_write_begin(snd_atomic_write_t *w)
> > >   {
> > > -   w->begin++;
> > > -   wmb();
> > > +   __atomic_add_fetch(&w->begin, 1,__ATOMIC_SEQ_CST);
> > >   }
> > > 
> > >   static __inline__ void snd_atomic_write_end(snd_atomic_write_t *w)
> > >   {
> > > -   wmb();
> > > -   w->end++;
> > > +   __atomic_add_fetch(&w->end, 1, __ATOMIC_SEQ_CST);
> > >   }
> > > 
> > >   static __inline__ void snd_atomic_read_init(snd_atomic_read_t *r, 
> > > snd_atomic_write_t *w)
> > > -- 
> > > 2.1.4
> > > _______________________________________________
> > > Alsa-devel mailing list
> > > Alsa-devel@alsa-project.org
> > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel@alsa-project.org
> > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > 
> 
> -- 
> Ricard Wolf Wanderlöf                           ricardw(at)axis.com
> Axis Communications AB, Lund, Sweden            www.axis.com
> Phone +46 46 272 2016                           Fax +46 46 13 61 30
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel