Since the introduction of threaded IO, we moved to C11 and _Atomic, but apparently that feature of C11 is only supported from GCC 4.9 and upwards. So since CentOS 7 and Ubuntu 14.04 use GCC 4.8 (which does support C11, but not _Atomic), redis 6.0 does not compile on these by default. These OSses, are still widely used, and since the threaded IO is disabled by default, maybe we want to reconsider. Besides, maybe we want redis (even the latest) to keep being very portable and keep supporting old and/or primitive platforms and compilers (for instance CentOS 6.0 or some embedded systems who don't have any support for C11)?

Options: 1. Make threaded IO and _Atomic an optional feature that needs to be enabled at compile time 2. Mess up the code in a way that will use other synchronization mechanism when _Atomic is missing. 3. Keep the current code and have redis 6.0 unsupported on these (and other) platforms.

I'd like to get feedback and maybe other alternatives. Thanks.

Comment From: trevor211

An alternative option: using scl.

For example, on centos7, we can do like this:

Step1, we install some tools. sudo yum -y install centos-release-scl devtoolset-7 llvm-toolset-7

Step2, enable scl before compile. scl enable devtoolset-7 llvm-toolset-7 bash

Step3, compile. make

Comment From: oranagra

@trevor211 i suppose the generated binary will depend on newer runtime libraries, and will not work out of the box on any CentOS 7 without installing the scl there too. right? not sure if that's acceptable for some deployments.

Comment From: trevor211

I did the test just now, and it turned out that the generated binary did work out of the box on another Centos7 not having the scl installed.

I did the experiment with the following steps.

Step1, create 2 docker containers (let's take them container A and B) using the same centos7 image. Step2, pull the lastest redis source code on container A, and just make, it failed. Step3, install scl and some tools as my previous commet on container A. Step4, do make on container A, and it succeeded. Step5, run redis-server on container A, and it succeeded. Step6, exit from scl and run redis-server again on container A, and it succeeded. Step7, copy redis-server binary from container A to container B, and run redis-server on container B, it succeeded.

Comment From: ShooterIT

I've always wanted to participate in this topic :) As we know, the default behavior of all _Atomic operations provides for sequentially consistent ordering. For _Atomicusing for threaded IO in redis, we use inter-thread synchronization of io_threads_pending[i], so other io threads could read the newest clients of io_threads_list rather than dirty values. I think acquire/release semantics may be enough https://en.cppreference.com/w/c/atomic/memory_order. BTW, I think we should add comments for this usage.

  • Mess up the code in a way that will use other synchronization mechanism when _Atomic is missing.

@oranagra We can use redisAtomic to replace _Atomic if we want to do this, and implement read and write function for redisAtomic, what we need concern about is memory order, memory_order_acquire for read values, memory_order_release for write values. Maybe just like leveldb https://github.com/google/leveldb/blob/v1.20/port/atomic_pointer.h, but now the newest version of leveldb also uses C++ 11. And this will bring a lot of complexity, we can use pthread_mutex_t easily if the compiling environment don't supports C11 and tell users that may not have good performance if enable threaded IO.

Comment From: oranagra

@ShooterIT i think i may have missed your point, but let me try to clear things up.

What i meant by option 2, it to revert back to the mechanism that were used before redis 6.0, which are implemented in our atomicvar.h (e.g. atomicIncr). The main difference between that and _Atomic is that they're harder to use. i.e. unlike just declaring a variable as _Atomic, we need to wrap any access to it with a function. so they code is less readable and more bug prone.

Comment From: ShooterIT

Maybe my statement is a bit confusing. Firstly, we want to introduce _Atomic keyword and how it can guarantee our program execute safely, you already must know. For _Atomic default behavior, it not only guarantees the operation's atomicity but also performs an acquire operation, a store performs a release operation, and read-modify-write performs both an acquire operation and a release operation, plus a single total order exists in which all threads observe all modifications in the same order.

For _Atomicusing for threaded IO in redis, we use inter-thread synchronization of io_threads_pending[i], so other io threads could read the newest clients of io_threads_list rather than dirty values. I think acquire/release semantics may be enough https://en.cppreference.com/w/c/atomic/memory_order. BTW, I think we should add comments for this usage.

For your option 2, I know our implement in atomicvar.h, I think it is ok for readability. But the implement of access variable does not meet our needs, because we use __ATOMIC_RELAXED

Relaxed operation: there are no synchronization or ordering constraints imposed on other reads or writes, only this operation's atomicity is guaranteed

Of course, we also can add memory_order_seq_cst implementation for our atomic variable operations.

Comment From: oranagra

Update: The core team have decided to step back from the C11 requirement. We see it causes pain to many, and we feel that the value (in code clarity) is not worth the pain, and that it was too early for Redis to depend on it. We'll reconsider it again in the future.

Comment From: literakl

I compiled 6.0.6 with scl on CentOS 7.8 as recommended by Trevor but make test failed. Shall I use the binary?

[ok]: XADD can CREATE an empty stream [ok]: XSETID can set a specific ID [ok]: XSETID cannot SETID with smaller ID [ok]: XSETID cannot SETID on non-existent key [ok]: AOF rewrite of list with quicklist encoding, string data [ok]: pending querybuf: check size of pending_querybuf after set a big value

Logged warnings (pid 48558): (none)

[exception]: Executing test client: wrong # args: should be "close channelId". wrong # args: should be "close channelId" while executing "close $fd write"

Comment From: oranagra

@literakl that's an unrelated problem with the test code, see: https://github.com/redis/redis/pull/7548

Comment From: oranagra

Quick update to anyone following this: We understand the pain this issue is causing to many, which is why we decided to undo this change (requirement for C11), but on the other hand we feel that it would not be responsible to release it in a patch level release which might cause some unexpected problems to an innocent upgrade.

The plan is to release 6.2 RC around the end of the year, and hopefully a final somewhere in January.

Comment From: sangeethdba

until then we have to have c11 to install redis 6.0 .please confirm

Comment From: oranagra

@sangeethdba yes, that's correct. if you feel adventurous and really need 6.0 features, you can probably cherry pick 445a4b669a3a7232a18bf23340c5f7d580aa92c7 to 6.0 safely.