Introduce C11 _Atomics #490

adetunjii · 2024-05-12T00:54:46Z

Replaces custom atomics logic with C11 default atomics logic.
Drops "atomicvar_api" field from server info

Closes #485

codecov · 2024-05-13T03:15:12Z

Codecov Report

Attention: Patch coverage is 77.14286% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 69.64%. Comparing base (3de5c71) to head (d3f984a).
Report is 1 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #490      +/-   ##
============================================
- Coverage     69.82%   69.64%   -0.18%     
============================================
  Files           109      109              
  Lines         61797    61776      -21     
============================================
- Hits          43150    43025     -125     
- Misses        18647    18751     +104

Files	Coverage Δ
src/db.c	`88.15% <ø> (ø)`
src/evict.c	`96.69% <ø> (ø)`
src/functions.c	`95.62% <ø> (ø)`
src/rdb.c	`75.84% <100.00%> (-0.05%)`	⬇️
src/replication.c	`86.71% <100.00%> (ø)`
src/threads_mngr.c	`93.61% <100.00%> (-0.14%)`	⬇️
src/zmalloc.c	`83.39% <100.00%> (-0.07%)`	⬇️
src/aof.c	`80.75% <85.71%> (-0.03%)`	⬇️
src/module.c	`9.34% <50.00%> (+<0.01%)`	⬆️
src/networking.c	`85.11% <80.00%> (+0.03%)`	⬆️
... and 4 more

... and 10 files with indirect coverage changes

aiven-sal · 2024-05-13T08:11:04Z

src/module.c

@@ -2434,7 +2434,7 @@ void VM_Yield(ValkeyModuleCtx *ctx, int flags, const char *busy_reply) {
                 * loop (ae.c) and avoid potential race conditions. */

                int acquiring;
-                atomicGet(server.module_gil_acquring, acquiring);
+                atomicGet(server.module_gil_acquiring, acquiring);


It looks like you forgot to replace this atomicGet with atomic_load_explicit here.

zuiderkwast

Looks good so far.

I think we can also wait for another changes which is in progress. It's is a change for I/O threads which will probably use atomic variables a lot.

My idea is that we replace all atomic{Get,Set,Incr} and also delete atomicvar.h in one PR just before Valkey 8 release candidate 1, when no new features will be added. Then, if some users have a problem when testing the release candidate, we can revert it before we release 8.0.

@madolson What do you think?

madolson · 2024-05-13T18:14:26Z

@zuiderkwast I guess i would prefer we do the C11 migration now. Then anyone who is building from unstable will be able to start relying on the new improvements. I would rather find out sooner that someone might still have build issues.

aiven-sal

You need to remove

valkey/src/zmalloc.c

Line 54 in dd2b27e

#include "atomicvar.h"

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).
And then you can remove atomicvar.h completely.

aiven-sal · 2024-05-14T10:27:51Z

src/bio.c

+                int last_status = atomic_load_explicit(&server.aof_bio_fsync_status, memory_order_relaxed);
+
+                atomic_store_explicit(&server.aof_bio_fsync_status, C_ERR, memory_order_relaxed);
+                atomic_store_explicit(&server.aof_bio_fsync_errno, errno, memory_order_relaxed);


You correctly rewrote the old code, but, to me, it looks like the old code itself can be improved.
Setting _status and _errno in this way, means that, when those values are read in https://github.com/valkey-io/valkey/pull/490/files#diff-1abc5651133d108c0c420d9411925373c711133e7748d9e4f4c97d5fb543fdd9R4591-R4593 , _errno may be not set yet.
Now, maybe it isn't very important to preserve the correct errno in this case, but considering how rare (hopefully) this kind of failures are, it shouldn't hurt much to move to a less relaxed memory ordering.
Maybe some form of Release/Acquire.
E.g.
in bio.c :

atomic_store_explicit(&server.aof_bio_fsync_errno,errno,memory_order_relaxed); atomic_store_explicit(&server.aof_bio_fsync_status,C_ERR,memory_order_release);

in server.c:

int aof_bio_fsync_status = atomic_load_explicit(&server.aof_bio_fsync_status,memory_order_relaxed); if (aof_bio_fsync_status == C_ERR) { atomic_thread_fence(memory_order_acquire) server.aof_last_write_errno = atomic_load_explicit(&server.aof_bio_fsync_errno,memory_order_relaxed); return DISK_ERROR_TYPE_AOF; }

This should perform just like the relaxed code when no error occurs and it will incur some small performance degradation only if fsync fails.

What do you think?

The performance degradation should be negligible.

zuiderkwast · 2024-05-14T13:38:00Z

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).

"c11-builtin" is what's used today when C11 atomics are used.

We can also consider dropping this info field.

adetunjii · 2024-05-14T14:18:26Z

and replace

valkey/src/server.c

Line 5631 in dd2b27e

"atomicvar_api:%s\r\n", REDIS_ATOMIC_API,

with something meaningful (I'm not sure what that would be).

"c11-builtin" is what's used today when C11 atomics are used.

We can also consider dropping this info field.

@zuiderkwast I think we should just drop it since we're making C11 the standard.

zuiderkwast · 2024-05-14T18:15:33Z

Ok, please mention it in the PR description that we're dropping the INFO field "atomicvar_api". It's a user-facing change.

Signed-off-by: adetunjii <adetunjithomas1@outlook.com>

…ix/c11-atomics

aiven-sal

I think you can now rm src/atomicvar.h .
Thanks for taking the time to work on my comments!

aiven-sal · 2024-05-15T15:48:44Z

src/server.c

-        int aof_bio_fsync_status;
-        atomicGet(server.aof_bio_fsync_status,aof_bio_fsync_status);
+        int aof_bio_fsync_status = atomic_load_explicit(&server.aof_bio_fsync_status, memory_order_relaxed);
+        atomic_thread_fence(memory_order_acquire);


My suggestion was to put the fence inside the if block, because it's only needed if you are going to access _errno. It that way you incur no performance penalty when _status != C_ERR (not that this is necessarily important).
If you think that the code would be clearer and more maintainable applying the fence unconditionally (I can agree with this), you can just use memory_order_acquire directly in atomic_load_explicit instead of memory_order_relaxed and drop the atomic_thread_fence call.

For clarity sake, the best would be to use memory_order_acquire directly in atomic_load_explicit.

Signed-off-by: Samuel Adetunji <adetunjithomas1@outlook.com>

…ix/c11-atomics

aiven-sal

LGTM
Thank you!

zuiderkwast · 2024-05-16T16:48:43Z

We need to remove all CI jobs for CentOS 7. They fail because it doesn't support C11. Shall we change these jobs to CentOS 8? I think the point is to have one of the oldest OSes that's still supported. (CentOS 7 is EOL in June this year.)

adetunjii · 2024-05-17T10:56:46Z

We need to remove all CI jobs for CentOS 7. They fail because it doesn't support C11. Shall we change these jobs to CentOS 8? I think the point is to have one of the oldest OSes that's still supported. (CentOS 7 is EOL in June this year.)

Yeah. I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR

zuiderkwast · 2024-05-17T11:33:18Z

I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR

I don't like to merge a PR which breaks the CI. It affects the daily CI too. We can do it in a separate PR, but then we should merge that CI PR before we merge this PR.

adetunjii · 2024-05-17T11:34:32Z

Okay sure. I’ll have a look at it Get Outlook for iOS<https://aka.ms/o0ukef>

…

________________________________ From: Viktor Söderqvist ***@***.***> Sent: Friday, May 17, 2024 12:33:39 PM To: valkey-io/valkey ***@***.***> Cc: Samuel Adetunji ***@***.***>; Author ***@***.***> Subject: Re: [valkey-io/valkey] Introduce C11 _Atomics (PR #490) I think it's best to remove old CI jobs for CentoS 7. Let's fix that in another PR I don't like to merge a PR which breaks the CI. It affects the daily CI too. We can do it in a separate PR, but then we should merge that CI PR before we merge this PR. — Reply to this email directly, view it on GitHub<#490 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AKXTHW3KXAVQZ235T2KQEZDZCXTJHAVCNFSM6AAAAABHSLAAJSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMJXGM4TQOJUGM>. You are receiving this because you authored the thread.Message ID: ***@***.***>

madolson · 2024-05-19T19:22:01Z

Shall we change these jobs to CentOS 8?

CentOS 8 is already EoL, it never got long term support from RHEL from the looks of it. CentOS also doesn't exist anymore, it was evolved into CentOS stream. We could try AlmaLinux 9 or CentOS stream 9 as a replacement. I pinged some folks, but my initial guess is to move to CentOS stream.

EDIT: Ideally we should have a matrix and do CentOS stream 9 and AlmaLinux 8. AlmaLinux 8 will be maintained until 2029, so it seems like a great replacement for our CentOS 7 test.

zuiderkwast

I've looked though this diff now. Looks good, but I think I spotted two typos.

src/networking.c

zuiderkwast

LGTM.

Don't merge until we have replaced CentOS 7 in CI jobs.

madolson · 2024-05-21T20:47:48Z

I documented what we should do about centos 7 here: #527, in case someone else has time to pick it up.

madolson requested a review from zuiderkwast May 12, 2024 16:29

aiven-sal reviewed May 13, 2024

View reviewed changes

zuiderkwast reviewed May 13, 2024

View reviewed changes

adetunjii requested review from zuiderkwast and aiven-sal May 14, 2024 01:34

aiven-sal reviewed May 14, 2024

View reviewed changes

zuiderkwast added the release-notes This issue should get a line item in the release notes label May 14, 2024

adetunjii requested a review from aiven-sal May 15, 2024 00:16

adetunjii force-pushed the fix/c11-atomics branch 5 times, most recently from 7e8380c to 8b7cfa0 Compare May 15, 2024 01:08

adetunjii and others added 3 commits May 15, 2024 02:09

Introduce C11 _Atomics

883573d

Signed-off-by: adetunjii <adetunjithomas1@outlook.com>

replace atomicvar header file

32dc844

Signed-off-by: adetunjii <adetunjithomas1@outlook.com>

drop atomic_var field from server info

08ce3ee

Signed-off-by: adetunjii <adetunjithomas1@outlook.com>

adetunjii force-pushed the fix/c11-atomics branch from 8b7cfa0 to 08ce3ee Compare May 15, 2024 01:11

Merge branch 'unstable' of https://github.com/adetunjii/valkey into f…

98407ce

…ix/c11-atomics

aiven-sal reviewed May 15, 2024

View reviewed changes

remove atomicvar header file

14ba161

Signed-off-by: Samuel Adetunji <adetunjithomas1@outlook.com>

adetunjii requested a review from aiven-sal May 15, 2024 23:45

Merge branch 'unstable' of https://github.com/adetunjii/valkey into f…

d3f984a

…ix/c11-atomics

aiven-sal approved these changes May 16, 2024

View reviewed changes

zuiderkwast reviewed May 20, 2024

View reviewed changes

src/networking.c Show resolved Hide resolved

src/networking.c Show resolved Hide resolved

zuiderkwast approved these changes May 21, 2024

View reviewed changes

aiven-sal mentioned this pull request May 22, 2024

Upgrade centos 7 to centos stream 9 #531

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce C11 _Atomics #490

Introduce C11 _Atomics #490

adetunjii commented May 12, 2024 •

edited

codecov bot commented May 13, 2024 •

edited

aiven-sal May 13, 2024

zuiderkwast left a comment

madolson commented May 13, 2024

aiven-sal left a comment

aiven-sal May 14, 2024

adetunjii May 14, 2024

zuiderkwast commented May 14, 2024 •

edited

adetunjii commented May 14, 2024

zuiderkwast commented May 14, 2024

aiven-sal left a comment

aiven-sal May 15, 2024

adetunjii May 15, 2024

aiven-sal left a comment

zuiderkwast commented May 16, 2024

adetunjii commented May 17, 2024

zuiderkwast commented May 17, 2024

adetunjii commented May 17, 2024 via email

madolson commented May 19, 2024 •

edited

zuiderkwast left a comment

zuiderkwast left a comment

madolson commented May 21, 2024

Introduce C11 _Atomics #490

Are you sure you want to change the base?

Introduce C11 _Atomics #490

Conversation

adetunjii commented May 12, 2024 • edited

codecov bot commented May 13, 2024 • edited

Codecov Report

aiven-sal May 13, 2024

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

madolson commented May 13, 2024

aiven-sal left a comment

Choose a reason for hiding this comment

aiven-sal May 14, 2024

Choose a reason for hiding this comment

adetunjii May 14, 2024

Choose a reason for hiding this comment

zuiderkwast commented May 14, 2024 • edited

adetunjii commented May 14, 2024

zuiderkwast commented May 14, 2024

aiven-sal left a comment

Choose a reason for hiding this comment

aiven-sal May 15, 2024

Choose a reason for hiding this comment

adetunjii May 15, 2024

Choose a reason for hiding this comment

aiven-sal left a comment

Choose a reason for hiding this comment

zuiderkwast commented May 16, 2024

adetunjii commented May 17, 2024

zuiderkwast commented May 17, 2024

adetunjii commented May 17, 2024 via email

madolson commented May 19, 2024 • edited

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

madolson commented May 21, 2024

adetunjii commented May 12, 2024 •

edited

codecov bot commented May 13, 2024 •

edited

zuiderkwast commented May 14, 2024 •

edited

madolson commented May 19, 2024 •

edited