On Thu, 12 May 2016 16:17:47 +0200,
Ricard Wanderlof wrote:
>
>
> On Thu, 12 May 2016, Takashi Iwai wrote:
>
> > On Thu, 12 May 2016 14:51:07 +0200,
> > Ricard Wanderlof 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@xxxxxxxx>
> > > > > Signed-off-by: Arvid Nihlgård Lindell <arvidnl@xxxxxxxx>
> > > >
> > > > 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?
>
> The code as it currently stands has macros like
>
> #define mb() __asm__ __volatile__ ("lock; addl $0,0(%%esp)": : :"memory")
> #define rmb() mb()
> #define wmb() __asm__ __volatile__ ("": : :"memory")
>
> which surely are gcc specific. So surely it can't be a requirement that
> the patch work on other compilers as well?
Well, it's not the requirement. It's just a question.
> It is relevant to verify which gcc versions support the builtins used in
> the patch however. I believe the kernel has requirements on which gcc
> versions it can be compiled with, but perhaps we need to be more lenient
> with userspace?
Yes. And I bet that the kernel can be still built with a gcc version
that doesn't support the new atomic primitive, too.
> > > > 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.
>
> In total, the various snd_atomic_* macros are used ~20 times in pcm_rate.c
> and ~30 times in pcm_plugin.c . If the atomic functionality is indeed
> necessary (and it appears to be), it would seem worth while abstracting
> the nitty gritty using macros I think, rather than cluttering up the code.
In other words, it's used only there. Most of other codes don't care
about it. So, the usefulness of the atomic operation there is really
doubtful.
> But that seems to be a different issue altogether than what the patch is
> intended to solve.
>
> However, one thing struck me. In our case we've seen the problem that the
> patch is intended to solve on the MIPS architecture. In this case we get
> the following definitions for mb() et al:
>
> /* Generic __sync_synchronize is available from gcc 4.1 */
>
> #define mb() __sync_synchronize()
> #define rmb() mb()
> #define wmb() mb()
>
> Could in fact the problem be that __sync_synchronize() is buggy on this
> platform, if the problem hasn't turned up anywhere else? One would expect
> otherwise that this issue would have come up much sooner.
Yeah, that's why I stated the primary question. The atomic operation
there is mostly cosmetic, not really helpful. In the whole other
operation, we don't guarantee the thread safety at all. So,
implementing the atomic op only in a very small part of the whole code
appears like a superfluous optimization to me.
That said: my preferred option is to scratch the whole atomic thing.
thanks,
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@xxxxxxxxxxxxxxxx
> > > > > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> > > > _______________________________________________
> > > > Alsa-devel mailing list
> > > > Alsa-devel@xxxxxxxxxxxxxxxx
> > > > 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
> > >
> >
>
> --
> 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@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
