If users want to use c11 _Atomic, they could modify Makefile

STD=-std=c11 -pedantic -DREDIS_STATIC=''

Would you mind having a look if you have time @JohnSully I notice that you use many atomic variables in your KeyDB.

Thanks @ShooterIT !

For server.unixtime, server.lruclock, server.next_client_id, Salvatore started using atomic variable from commits
ece6587 1f598fc
I just restored these operations back

i would rather you find the commit that removed them and logically revert it, since there's a risk that these changed after they were first introduced (so we're reinstating their old version).
can you please look into that, maybe they're already good, but better be sure.

Maybe we want to suppress these helgrind warnings and add it to run in the daily CI github action?

Would C11 _Atomic provide any efficiency improvement here? if so, maybe we want to change the Makefile to automatically use it when supported?
If not, maybe we want to add a make target for it, and/or mention it in the the README?

Can you please test that it builds on Centos7 and possibly 6 too.
7 would be easy by modifying the the daily CI yaml trigger and pushing to a branch on your forked repo.

@yossigo please take a look.

you find the commit that removed them and logically revert it,

this is the commit dd5b105, I think i did revert logically. Please help me review it.

I test with Helgrind just because i want to find errors early.

I don't find C11 _Atomic has efficiency improvement, i provide it because some people would like to use new features of language and more importantly we ever supported it. I have no idea to change the Makefile to automatically use _Atomic when supported, maybe target is a good idea.

I'm sorry i don't have Centos, my company machine kernel is as follow, and two gcc versions of 4.8(not support _Atomic) and 8.2((supports _Atomic)) are installed on it. All redis tests are passed both of two versions.
Linux version 3.10.0_3-0-0-24 (gcc version 4.8.2 20140120 (Red Hat 4.8.2-16) (GCC) )

Hi @ShooterIT

I'm not sure I understand the need for introducing these in all the places you've chosen, e.g. in createClient. Is this based upon the output of a tool? Note that your changes are not free, and will introduce performance regressions especially on ARM. That said for the atomics you've changed from relaxed to higher consistency will generally only repro on ARM if they were bugs anyways.

KeyDB of course uses atomics a lot but our threading model is also very different so its not a guide for what should be Atomic in Redis.

Thanks @JohnSully

For server.unixtime, server.lruclock, server.next_client_id, Salvatore started using atomic variable from commits
ece6587 1f598fc
I just restored these operations back

Salvatore did add comment on his commits. Yes, for these variable, I also think it is safe without being defined as atomic variable.

Modules TSC: use atomic var for server.unixtime.

This avoids Helgrind complaining, but we are actually not using
atomicGet() to get the unixtime value for now: too many places where it
is used and given tha time_t is word-sized it should be safe in all the
archs we support as it is.

On the other hand, Helgrind, when Redis is compiled with "make helgrind"
in order to force the __sync macros, will detect the write in
updateCachedTime() as a read (because atomic functions are used) and
will not complain about races.

This commit also includes minor refactoring of mutex initializations and
a "helgrind" target in the Makefile.

The default behavior of C11 _Atomic operations provides for sequentially consistent ordering, so now it should not have performance regressions than before.
On my opinion, memory_order_acquire for reading variables,memory_order_release for writing variables. that will be enough. On strongly-ordered systems, maybe memory_order_relaxed is enough for our needs. But on weakly-ordered systems, there may be many errors that are difficult to debug. BTW, servers based on ARM are more popular now.

On strongly-ordered systems — x86, SPARC TSO, IBM mainframe, etc. — release-acquire ordering is automatic for the majority of operations. No additional CPU instructions are issued for this synchronization mode; only certain compiler optimizations are affected (e.g., the compiler is prohibited from moving non-atomic stores past the atomic store-release or performing non-atomic loads earlier than the atomic load-acquire). On weakly-ordered systems (ARM, Itanium, PowerPC), special CPU load or memory fence instructions are used.

Even so, I still use ’sequentially consistent‘ semantic just the same as before, it need not require much complicated derivation and analysis for correctness. Redis can run on many different archs, we should guarantee correctness theoretically. And more and more people take part in developing redis, that also is very import to make it easy to understand and make modification less error-prone. What's more, actually I don't find there are no obvious performance difference on x86 arch between 'acquire/release' and 'sequentially consistent'.

For ARM, there are some changes, the CPU instructions of different atomic variable operations become the same gradually https://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html. Maybe CPU and compiler designers want to make it easy to use because it is too hard actually. I borrow one ARM server(as follow) from my colleague, both 'relaxed' and 'consistent' memory order of atomic variables can run correctly and without much performance difference.
Linux 4.14.0_1-1-0-20 #1 SMP Fri May 29 10:17:03 CST 2020 aarch64 aarch64 aarch64 GNU/Linux

I know KeyDB has different threading model, but i think you have much experience with atomic variable. Thank you very much for reviewing my PR.

@oranagra For using C11 _Atomic, how about this

diff --git a/src/Makefile b/src/Makefile
index 6bbf9e175..82f292eb6 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -20,7 +20,7 @@ DEPENDENCY_TARGETS=hiredis linenoise lua
 NODEPS:=clean distclean
 
 # Default settings
-STD=-std=c99 -pedantic -DREDIS_STATIC=''
+STD=-pedantic -DREDIS_STATIC=''
 ifneq (,$(findstring clang,$(CC)))
 ifneq (,$(findstring FreeBSD,$(uname_S)))
   STD+=-Wno-c11-extensions
@@ -34,6 +34,13 @@ INSTALL_BIN=$(PREFIX)/bin
 INSTALL=install
 PKG_CONFIG?=pkg-config
 
+# Use c99 by default, but we can specify to use c11
+ifeq ($(C11),yes)
+STD+=-std=c11
+else
+STD+=-std=c99
+endif
+
 # Default allocator defaults to Jemalloc if it's not an ARM
 MALLOC=libc
 ifneq ($(uname_M),armv6l)

@ShooterIT if there are no downsides of using the other atomic types (gcc and sync builtins) then i prefer to stick to c99 even if c11 is supported. the benefit is that if there are any other differences we always build and test with c99 (and not just rarely do so), and it'll present any use of a c11 feature to creep in and be detected only in a daily CI.

If there are any performance advantages with _Atomic, then your suggested patch above makes sense.

Another thought on that subject, all the pthread_mutex_t variables are usually dead code, we ca detect that at build time and don't bother to create and initialize them.
This possibly saves some resources.
On the other hand, i'm not at all sure that we have any coverage for this in our CI tests? i.e. can can simply delete used_memory_mutex and everything will build successfully and all CI will pass.

maybe we don't need to support the mutex fallback at all? or maybe we should add a daily CI cycle that covers that configuration (at least to see that it builds and runs).

hi @oranagra AFAIK, operations of c11 _Atomic is the same as atomic builtins for gcc implementation. this is part of the code in <stdatomic.h> of gcc 8.2 on my linux server. So i think there is no much difference between c11 _Atomic and gcc atomic builtins.

#define atomic_store_explicit(PTR, VAL, MO)				\
  __extension__								\
  ({									\
    __auto_type __atomic_store_ptr = (PTR);				\
    __typeof__ (*__atomic_store_ptr) __atomic_store_tmp = (VAL);	\
    __atomic_store (__atomic_store_ptr, &__atomic_store_tmp, (MO));	\
  })

#define atomic_store(PTR, VAL)				\
  atomic_store_explicit (PTR, VAL, __ATOMIC_SEQ_CST)


#define atomic_load_explicit(PTR, MO)					\
  __extension__								\
  ({									\
    __auto_type __atomic_load_ptr = (PTR);				\
    __typeof__ (*__atomic_load_ptr) __atomic_load_tmp;			\
    __atomic_load (__atomic_load_ptr, &__atomic_load_tmp, (MO));	\
    __atomic_load_tmp;							\
  })

#define atomic_load(PTR)  atomic_load_explicit (PTR, __ATOMIC_SEQ_CST)

For pthread_mutex_t dead code, actually i am also feel entangled. we can clearly know whether we should use pthread_mutex or not at build time, we must use some micros if we do that make it less readable. There will be some dead code if not. Maybe salvatore also didn't make it less readable as before, but actually there are more and more atomic variable now, i think we can reconstruct it. As you said, we don't need to support the mutex fallback at all, i agree with this point, it is already 2020 :), using pthread_mutex to support atomic variable is a relatively rare case.

@ShooterIT i'm not certain if we want to remove the code for that fallback or not. after all, the whole point of this PR is to make redis more portable than it is in 6.0. (maybe some old or embedded system may still depend on them?)
I do think that we don't need to consider their implication on performance (considering that they're only used if all modern options aren't available).

So i'm not certain, i wanna trim this code. @yossigo what do you think?

What I am certain, is that i don't like dead code be completely uncovered by the test (like if we remove used_memory_mutex today, we won't notice that the code doesn't compile in this mode). so if we keep the current approach, i think we need to add another daily CI cycle that covers these.

@oranagra Copy that, i wait for your final conclusion.

To test dead code pthread_mutex, but I don't what environment need use pthread_mutex fallback, maybe a very old compiler or OS

Let's see what we're going to do, @oranagra

• Support C11 _Atomic or not
• Use pthread_mutex fallback and add CI to cover it, or don't use pthread_mutex fallback

Following a discussion with @yossigo we feel that we should keep supporting _Atomic (since this is the only one that's standard, maybe some day a compiler will support that and not the others), and also because it doesn't add a lot of mess (local to atomicvar.h).

Regarding the pthread_mutex, we concluded that we should drop them.
unlike the above it does mess up the code a lot (outside of atomicvar.h), there is a risk that this code will accidentally get active somehow resulting in low performance (in that case if the accident is solvable, we rather fail the build), and we concluded that any realistic platform for Redis is likely to have support for the other atomic builtins.

@redis/core-team any objections?
@ShooterIT if that approved, i'll ask that you clean all the mutex variables, and replace the fallback with #error.

Yes, C11 _Atomic is standard, It offers exact definition of atomic variable operations semantic, and i think we will _Atomic gradually. This is standard！

I also agree that we drop pthread_mutex fallback, Redis is an in-memory database, we should keep its performance, what worse, it is error-prone, we may forget to define a pthread_mutex variable, and we need add CI for it. I hope you can reach an agreement.

@oranagra I'm okay with that proposal.

@oranagra I remove pthread_mutex fallback for atomic variable.

How to support C11 _Atomic, how about #7707 (comment)

@ShooterIT the commit that removes the mutexes looks fine.

my problem with the makefile change you suggested in #7707 (comment) is that it'll normally use _Atomic and we're unlikely to know notice that the alternative is broken, or if we violated redis by using some C11 feature elsewhere. i suppose we'll detect that in the daily CI when testing CentOS7 (after we remove the SCL portions).

please add a commit that removes the scl portions from

come to think of it, the above argument goes the other way around too, i.e. if we always use atomic_builtins, how do we know that the _Atomic is not broken?
Maybe we can figure out a way to make sure that all the variables declared with redisAtomic are always accessed with our atomic accessor macros and vice versa? i can't think of any way.

so maybe indeed the best thing would be to use C11 by default and then at least we have the CentOS7 CI job cover one of the alternatives. what do you think?

For C11 _Atomic, my operation system is macOS and complier is clang, it will report errors if i didn't declare the variables with redisAtomic but i accessed them with our atomic macros, i think that can help us check declarations of atomic variables. I can't have a idea to guarantee we access them with our atomic macros.

It will be more safe if we use C11 _Atomic by default, it also is safe if we forget to access them with our atomic macros. Our initial motivation is to make redis more compatible and keep it run correctly. I agree with using C11 by default, but we need cover both atomic builtin and sync builtin, make sure it can be compiled at least.

@oranagra Let's me summarize all works for this PR.

Redis 6.0 introduces I/O threads, it is so cool and efficient, we use C11 _Atomic to establish inter-thread synchronization without mutex. But the compiler that must supports C11 _Atomic can compile redis code, that brings a lot of inconvenience since some common platforms can't support by default such as CentOS7, so we want to implement redis atomic type to make it more portable.

We have implemented our atomic variable for redis that only has 'relaxed' operations in src/atomicvar.h, so we implement some operations with 'sequentially-consistent', just like the default behavior of C11 _Atomic that can establish inter-thread synchronization. And we replace all uses of C11 _Atomic with redis atomic variable.

Our implementation of redis atomic variable uses C11 _Atomic, __atomic or __sync macros if available, it supports most common platforms, and we will detect automatically which feature we use. In Makefile we use a dummy file to detect if the compiler supports C11 _Atomic. Now for gcc, we can compile redis code theoretically if your gcc version is not less than 4.1.2(starts to support __sync_xxx operations). Otherwise, we remove use mutex fallback to implement redis atomic variable for performance and test. You will get compiling errors if your compiler doesn't support all features of above.

For cover redis atomic variable tests, we add other CI jobs that build redis on CentOS6 and CentOS7 and workflow daily jobs that run the tests on them. For them, we just install gcc by default in order to cover different compiler versions, gcc is 4.4.7 by default installation on CentOS6 and 4.8.5 on CentOS7.

We restore the feature that we can test redis with Helgrind to find data race errors. But you need install Valgrind in the default path configuration firstly before running your tests, since we use macros in helgrind.h to tell Helgrind inter-thread happens-before relationship explicitly for avoiding false positives. Please open an issue on github if you find data race errors relate to this commit.

Unrelated:
Fix redefinition of typedef 'RedisModuleUserChangedFunc'
For some old version compilers, they will report errors or warnings, if we redefine function type.

@redis/core-team this PR is ready for review. please have a look at the previous post for a summary (and squash commit comment).

@ShooterIT thanks a lot for handling this.
i hope it'll make the life of many people easier. (and prevent some issues and questions being opened here and in stack overflow!)

@ShooterIT i noticed that server.unixtime is set via atomicSet but is read like a normal variable (not via atomicGet)
is that on purpose or was it overlooked?

hi, @guybe7 i just kept old method before 6.0, https://github.com/redis/redis/blob/5.0/src/server.c#L1077
i think it is not necessary to set server.unixtime to atomic variable.