[FFmpeg-devel] [PATCH] Stop using _explicit atomic operations where not necessary.

[Anton Khirnov]

Quoting Andreas Rheinhardt (2021-06-06 19:20:38)
> Anton Khirnov:
> > Quoting Andreas Rheinhardt (2021-06-06 11:13:00)
> >> Anton Khirnov:
> >>> Memory ordering constraints other than the default (sequentially
> >>> consistent) can behave in very unintuitive and unexpected ways, and so
> >>> should be avoided unless there is a strong (typically performance)
> >>> reason for using them. This holds especially for memory_order_relaxed,
> >>> which imposes no ordering constraints.
> >>
> >> I disagree with this: Adding ordering constraints is wrong, as it can
> >> provide synchronization between different threads and this can mask
> >> races. Providing synchronization as a byproduct should be avoided: Users
> >> should know what they get. And if a user wants an allocation limit in
> >> thread A to be in effect for allocations in thread B, then the user
> >> should provide the synchronization him/herself.
> > 
> > I do not agree. C11 default is sequential consistency, therefore our
> > public API should provide sequential consistency.
> > 
> 
> 1. The atomics here are not part of the public API at all: The
> documentation of none of the functions involved mentions atomics or
> synchronization etc. at all (which means that if a user wants
> synchronization between threads, he should take care of it himself). We
> could change the implementation to not use atomics at all (e.g. by
> wrapping everything in mutexes, which needn't be sequentially consistent).
> (Furthermore, in case of libavformat/fifo.c this does not even come
> close to affecting the public API, given that we are talking about
> synchronization between the user thread and a thread owned by us; in
> case of av_cpu_count() the atomics only exist because of an av_log(),
> there is no reason for a user to believe that synchronization is even
> involved.)
> 2. I don't buy the argument:
> Which memory ordering to use should be decided based upon what one wants
> and needs; one should not blindly use one because it is the default or
> recommended by some authority (doing so smells like cargo cult).

That is an overly idealistic view of the situation. I think very few
people around here have an indepth understanding of concurrency issues,
e.g. of the difference between using memory_order_seq_cst and
memory_order_relaxed. This stuff is complicated and unintuitive,
according to pretty much everyone who ever dealt with it.

And you cannot expect that everyone will study this topic in detail just
because they need to touch an atomic operation. So it seems perfectly
reasonable to me to defer to a suitable authority (like the people who
designed c11 atomics) and use their recommended defaults unless there is
a strong reason to do otherwise.

> In case
> of av_get_cpu_flags()+av_force_cpu_flags(), all one wants is that one
> can just load and write the value; given that there is no restriction
> loads a valid value (i.e. no torn reads/writes) and one wants that one
> can access this function by multiple threads simultaneously without
> races. Atomics provide this even with memory_order_relaxed. In case of
> av_cpu_count() all that is desired is that av_log() is only called once;
> an atomic_exchange provides this automatically, even with
> memory_order_relaxed. The av_max_alloc()+av_malloc/... combination is
> just like av_get/force_cpu_flags().
> 
> 
> > My other concern is that people are cargo culting this code around
> > without understanding what it really does. Again, sequentially
> > consistent atomics make that safer, since they do what you'd expect them
> > to.
> > 
> 
> Are you really advocating that we write our code in such a way that
> makes it safe for cargo-culters to copy it?

Yes I am.

People copy code around, that's just a fact of life. Two separate people
told me recently that their patches used memory_order_relaxed only
because they based them off some other code that was already using it.

So IMO we should strive to write code that is not only safe and correct,
but also robust against future modifications and being copied around.
That includes not using potentially-unsafe operations without a good
reason.

There is also the question of code readability. When I see
atomic_load(&foo);
then it's just an atomic load. But when I see
atomic_load_explicit(&foo, memory_order_bar);
I also need to think about the reason the author chose to use that
specific memory order.

As for your example program, you could also just remove the av_log()
from av_cpu_count(). Which should be done anyway because av_log(NULL is
evil.

-- 
Anton Khirnov