diff mbox

[FFmpeg-devel,2/4] lavfi/avfilter: simplify filter registration

Message ID CAE9qxYCPO_Fesjeii+0hGz7XPR2Jw5+fboE26VuDp_V5NiZFHw@mail.gmail.com
State New
Headers show

Commit Message

Rostislav Pehlivanov Nov. 30, 2017, 11:54 a.m. UTC 
On 29 November 2017 at 16:40, Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Wed, Nov 29, 2017 at 03:51:34AM +0000, Rostislav Pehlivanov wrote:
> > On 28 November 2017 at 20:23, Michael Niedermayer <michael@niedermayer.cc
> >
> > wrote:
> >
> > > On Mon, Nov 27, 2017 at 04:30:19AM +0000, Rostislav Pehlivanov wrote:
> > > > Atomics were entirely pointless and did nothing but slow and
> complicate
> > > > the process down. This could be improved further still but the main
> > > > objective of this commit is to simplify.
> > > >
> > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > > > ---
> > > >  libavfilter/avfilter.c | 8 +++++---
> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > *_register() can be called by the user app in a unsyncronized way
> > > for example from a module like vlc used AFAIK.
> > > or from libs loaded by an application which use libavfilter or other
> > > internally.
> > >
> > > These registration functions should stay thread safe
> > >
> > >
> > > [...]
> > >
> > > --
> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
> 87040B0FAB
> > >
> > > Those who would give up essential Liberty, to purchase a little
> > > temporary Safety, deserve neither Liberty nor Safety -- Benjamin
> Franklin
> > >
> > > _______________________________________________
> > > ffmpeg-devel mailing list
> > > ffmpeg-devel@ffmpeg.org
> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > >
> > >
> > What variable actually needs to be protected?
>
> The linked list of registered "entities"
> That is if the addition is not synchronized by some means, an atomic
> exchange a mutex or something then a race can generally occur
> and only one addition might succeed or something else might get
> entangled
>
> This is in practical terms a very unlikely event to occur of course
>
> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> You can kill me, but you cannot change the truth.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Could you test the attached patch? Pretty much everything is synchronized.

Comments

Michael Niedermayer Nov. 30, 2017, 2:01 p.m. UTC | #1 
On Thu, Nov 30, 2017 at 11:54:25AM +0000, Rostislav Pehlivanov wrote:
> On 29 November 2017 at 16:40, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Wed, Nov 29, 2017 at 03:51:34AM +0000, Rostislav Pehlivanov wrote:
> > > On 28 November 2017 at 20:23, Michael Niedermayer <michael@niedermayer.cc
> > >
> > > wrote:
> > >
> > > > On Mon, Nov 27, 2017 at 04:30:19AM +0000, Rostislav Pehlivanov wrote:
> > > > > Atomics were entirely pointless and did nothing but slow and
> > complicate
> > > > > the process down. This could be improved further still but the main
> > > > > objective of this commit is to simplify.
> > > > >
> > > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> > > > > ---
> > > > >  libavfilter/avfilter.c | 8 +++++---
> > > > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > >
> > > > *_register() can be called by the user app in a unsyncronized way
> > > > for example from a module like vlc used AFAIK.
> > > > or from libs loaded by an application which use libavfilter or other
> > > > internally.
> > > >
> > > > These registration functions should stay thread safe
> > > >
> > > >
> > > > [...]
> > > >
> > > > --
> > > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
> > 87040B0FAB
> > > >
> > > > Those who would give up essential Liberty, to purchase a little
> > > > temporary Safety, deserve neither Liberty nor Safety -- Benjamin
> > Franklin
> > > >
> > > > _______________________________________________
> > > > ffmpeg-devel mailing list
> > > > ffmpeg-devel@ffmpeg.org
> > > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> > > >
> > > >
> > > What variable actually needs to be protected?
> >
> > The linked list of registered "entities"
> > That is if the addition is not synchronized by some means, an atomic
> > exchange a mutex or something then a race can generally occur
> > and only one addition might succeed or something else might get
> > entangled
> >
> > This is in practical terms a very unlikely event to occur of course
> >
> > [...]
> > --
> > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
> >
> > You can kill me, but you cannot change the truth.
> >
> > _______________________________________________
> > ffmpeg-devel mailing list
> > ffmpeg-devel@ffmpeg.org
> > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >
> >
> Could you test the attached patch? Pretty much everything is synchronized.

>  avfilter.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 8ab986fa3b0f735e025e5c0a3235ca4c7e227f1b  v2-0002-lavfi-avfilter-simplify-filter-registration.patch
> From fb9789206c76c876d6a79e81df68c6b8041f03c3 Mon Sep 17 00:00:00 2001
> From: Rostislav Pehlivanov <atomnuker@gmail.com>
> Date: Mon, 27 Nov 2017 04:12:26 +0000
> Subject: [PATCH v2 2/2] lavfi/avfilter: simplify filter registration
> 
> Atomics were entirely pointless and did nothing but slow and complicate
> the process down. This could be improved further still but the main
> objective of this commit is to simplify.
> 
> Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
> ---
>  libavfilter/avfilter.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
> index b98b32bacb..76c9f12be9 100644
> --- a/libavfilter/avfilter.c
> +++ b/libavfilter/avfilter.c
> @@ -19,7 +19,6 @@
>   * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> -#include "libavutil/atomic.h"
>  #include "libavutil/avassert.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/buffer.h"
> @@ -37,6 +36,7 @@
>  #define FF_INTERNAL_FIELDS 1
>  #include "framequeue.h"
>  
> +#include <stdatomic.h>
>  #include "audio.h"
>  #include "avfilter.h"
>  #include "filters.h"
> @@ -574,7 +574,7 @@ int avfilter_process_command(AVFilterContext *filter, const char *cmd, const cha
>  }
>  
>  static AVFilter *first_filter;
> -static AVFilter **last_filter = &first_filter;
> +static _Atomic (AVFilter **) last_filter = ATOMIC_VAR_INIT(&first_filter);
>  
>  const AVFilter *avfilter_get_by_name(const char *name)
>  {
> @@ -592,16 +592,16 @@ const AVFilter *avfilter_get_by_name(const char *name)
>  
>  int avfilter_register(AVFilter *filter)
>  {
>      /* the filter must select generic or internal exclusively */
>      av_assert0((filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE) != AVFILTER_FLAG_SUPPORT_TIMELINE);
>  
>      filter->next = NULL;
>  
> +    /* Iterate through the list until the last entry has been reached */
> +    do {
> +        *atomic_load(&last_filter) = filter;
> +        atomic_store(&last_filter, &filter->next);
> +    } while (*atomic_load(&last_filter));

assume 2 register run at the same time

both execute atomic_load(&last_filter)
and obtain a pointer to a pointer to first_filter, which they both
store their (different) filter arguments in (non atomically)

Thats a race & undefined behavior
and even if this store was atomic, the later would overwrite the
earlier store before the earlier code had finished the rest of its
steps

[...]
Hendrik Leppkes Nov. 30, 2017, 2:20 p.m. UTC | #2 
On Thu, Nov 30, 2017 at 12:54 PM, Rostislav Pehlivanov
<atomnuker@gmail.com> wrote:
> On 29 November 2017 at 16:40, Michael Niedermayer <michael@niedermayer.cc>
> wrote:
>
>> On Wed, Nov 29, 2017 at 03:51:34AM +0000, Rostislav Pehlivanov wrote:
>> > On 28 November 2017 at 20:23, Michael Niedermayer <michael@niedermayer.cc
>> >
>> > wrote:
>> >
>> > > On Mon, Nov 27, 2017 at 04:30:19AM +0000, Rostislav Pehlivanov wrote:
>> > > > Atomics were entirely pointless and did nothing but slow and
>> complicate
>> > > > the process down. This could be improved further still but the main
>> > > > objective of this commit is to simplify.
>> > > >
>> > > > Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
>> > > > ---
>> > > >  libavfilter/avfilter.c | 8 +++++---
>> > > >  1 file changed, 5 insertions(+), 3 deletions(-)
>> > >
>> > > *_register() can be called by the user app in a unsyncronized way
>> > > for example from a module like vlc used AFAIK.
>> > > or from libs loaded by an application which use libavfilter or other
>> > > internally.
>> > >
>> > > These registration functions should stay thread safe
>> > >
>> > >
>> > > [...]
>> > >
>> > > --
>> > > Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC7
>> 87040B0FAB
>> > >
>> > > Those who would give up essential Liberty, to purchase a little
>> > > temporary Safety, deserve neither Liberty nor Safety -- Benjamin
>> Franklin
>> > >
>> > > _______________________________________________
>> > > ffmpeg-devel mailing list
>> > > ffmpeg-devel@ffmpeg.org
>> > > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> > >
>> > >
>> > What variable actually needs to be protected?
>>
>> The linked list of registered "entities"
>> That is if the addition is not synchronized by some means, an atomic
>> exchange a mutex or something then a race can generally occur
>> and only one addition might succeed or something else might get
>> entangled
>>
>> This is in practical terms a very unlikely event to occur of course
>>
>> [...]
>> --
>> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>>
>> You can kill me, but you cannot change the truth.
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>>
> Could you test the attached patch? Pretty much everything is synchronized.
>

The only way to have two threads modify a linked list at the same time
is with a "compare exchange" (cas) instruction, so why not just keep
the old logic and replace the avpriv atomic with a stdatomic one?

ie.
while(*f || avpriv_atomic_ptr_cas((void * volatile *)f, NULL, filter))

becomes: (plus appropriate type changes/casts as required)
while(*f || atomic_compare_exchange_strong(f, NULL, filter) == false)

- Hendrik
diff mbox

Patch

From fb9789206c76c876d6a79e81df68c6b8041f03c3 Mon Sep 17 00:00:00 2001
From: Rostislav Pehlivanov <atomnuker@gmail.com>
Date: Mon, 27 Nov 2017 04:12:26 +0000
Subject: [PATCH v2 2/2] lavfi/avfilter: simplify filter registration

Atomics were entirely pointless and did nothing but slow and complicate
the process down. This could be improved further still but the main
objective of this commit is to simplify.

Signed-off-by: Rostislav Pehlivanov <atomnuker@gmail.com>
---
 libavfilter/avfilter.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/libavfilter/avfilter.c b/libavfilter/avfilter.c
index b98b32bacb..76c9f12be9 100644
--- a/libavfilter/avfilter.c
+++ b/libavfilter/avfilter.c
@@ -19,7 +19,6 @@ 
  * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
  */
 
-#include "libavutil/atomic.h"
 #include "libavutil/avassert.h"
 #include "libavutil/avstring.h"
 #include "libavutil/buffer.h"
@@ -37,6 +36,7 @@ 
 #define FF_INTERNAL_FIELDS 1
 #include "framequeue.h"
 
+#include <stdatomic.h>
 #include "audio.h"
 #include "avfilter.h"
 #include "filters.h"
@@ -574,7 +574,7 @@  int avfilter_process_command(AVFilterContext *filter, const char *cmd, const cha
 }
 
 static AVFilter *first_filter;
-static AVFilter **last_filter = &first_filter;
+static _Atomic (AVFilter **) last_filter = ATOMIC_VAR_INIT(&first_filter);
 
 const AVFilter *avfilter_get_by_name(const char *name)
 {
@@ -592,16 +592,16 @@  const AVFilter *avfilter_get_by_name(const char *name)
 
 int avfilter_register(AVFilter *filter)
 {
-    AVFilter **f = last_filter;
-
     /* the filter must select generic or internal exclusively */
     av_assert0((filter->flags & AVFILTER_FLAG_SUPPORT_TIMELINE) != AVFILTER_FLAG_SUPPORT_TIMELINE);
 
     filter->next = NULL;
 
-    while(*f || avpriv_atomic_ptr_cas((void * volatile *)f, NULL, filter))
-        f = &(*f)->next;
-    last_filter = &filter->next;
+    /* Iterate through the list until the last entry has been reached */
+    do {
+        *atomic_load(&last_filter) = filter;
+        atomic_store(&last_filter, &filter->next);
+    } while (*atomic_load(&last_filter));
 
     return 0;
 }
-- 
2.15.1.424.g9478a66081