Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

seqlock: Enforce C11 Atomics #19

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

25077667
Copy link

Pull Request Summary

This pull request aims to enhance the existing seqlock implementation by making it more portable, improving code readability, and adhering to coding standards. The changes involve modifications to both the Makefile and the seqlock code itself.

Changes Made

Makefile Enhancements

  • Added compilation flags to improve code quality and compliance:
    • -Wextra: This flag enables additional warnings beyond -Wall, helping catch potential issues in the code.
    • -pedantic: Enforced strict adherence to the C standard, promoting standardized and portable code.

Code Improvements

  • Transitioned to using <stdatomic.h> for atomic operations, which enhances synchronization and makes the code more standardized.
  • Employed atomic_thread_fence for memory barriers, enhancing portability across different architectures.
  • Replaced architecture-dependent inline assembly instructions with standardized atomic operations. This improves cross-architecture compatibility and maintainability.
  • Revised the ATOMIC_COPY macro to utilize atomic load and store operations for atomic data copying.

Data Types and Typedefs

  • Redefined the seqlock_t type using _Atomic(uint32_t) to ensure atomic access to the variable, further improving synchronization.

Motivation

These enhancements were motivated by the goal of creating a more reliable and maintainable seqlock implementation. The changes aim to address portability concerns across various architectures while improving the overall readability and standardization of the codebase.

Request for Review

Kindly review the changes made in this pull request. Your feedback is highly valuable, and any suggestions for further improvements are welcome.

Thank you for considering this pull request.

Best regards,
scc
Feel free to customize the context further to suit the specifics of your project and the details you'd like to emphasize.

seqlock/Makefile Outdated Show resolved Hide resolved
@jserv jserv changed the title Refactor seqlock with C11's _Atomic seqlock: Enforce C11 Atomics Aug 27, 2023
#elif defined(__aarch64__)
#define spin_wait() __asm__ __volatile__("isb\n")
#if defined(__i386__) || defined(__x86_64__) || defined(__aarch64__)
#define spin_wait() atomic_thread_fence(memory_order_seq_cst)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have to confirm the generated code is identical to the intended instruction sequence.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be generating

dmb     ish

with Apple clang version 14.0.3 (clang-1403.0.22.14.1).

I would figure what's the output in gcc x86

Copy link
Author

@25077667 25077667 Aug 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In gcc 13.2.1 with x86-64, both

  1. using __builtin_ia32_pause()
  2. using atomic_thread_fence(memory_order_seq_cst)

are generating

data16 cs nopw 0x0(%rax,%rax,1)
nop
xchg   %ax,%ax

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In LLVM clang version 15.0.7 with x86-64, adding an extra pause when __builtin_ia32_pause is used.

    125 0000000000001190 <seqlock_acquire_rd>:
    126     1190:       50                      push   %rax
    127     1191:       0f ae f0                mfence
    128     1194:       8b 07                   mov    (%rdi),%eax
    129     1196:       a8 01                   test   $0x1,%al
    130     1198:       75 08                   jne    11a2 <seqlock_acquire_rd+0x12>
    131     119a:       a8 01                   test   $0x1,%al
    132     119c:       75 0f                   jne    11ad <seqlock_acquire_rd+0x1d>
    133     119e:       59                      pop    %rcx
    134     119f:       c3                      ret
    135     11a0:       f3 90                   pause
    136     11a2:       0f ae f0                mfence
    137     11a5:       8b 07                   mov    (%rdi),%eax
    138     11a7:       a8 01                   test   $0x1,%al
    139     11a9:       75 f5                   jne    11a0 <seqlock_acquire_rd+0x10>
    140     11ab:       eb ed                   jmp    119a <seqlock_acquire_rd+0xa>
    141     11ad:       48 8d 3d 6f 0e 00 00    lea    0xe6f(%rip),%rdi        # 2023 <_IO_stdin_used+0x23>
    142     11b4:       48 8d 35 82 0e 00 00    lea    0xe82(%rip),%rsi        # 203d <_IO_stdin_used+0x3d>
    143     11bb:       48 8d 0d 85 0e 00 00    lea    0xe85(%rip),%rcx        # 2047 <_IO_stdin_used+0x47>
    144     11c2:       ba 40 00 00 00          mov    $0x40,%edx
    145     11c7:       e8 84 fe ff ff          call   1050 <__assert_fail@plt>
    146     11cc:       0f 1f 40 00             nopl   0x0(%rax)

@25077667 25077667 requested a review from jserv August 27, 2023 14:42
seqlock/seqlock.c Outdated Show resolved Hide resolved
Copy link
Contributor

@jserv jserv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove seqlock/tests, which was generated.

@25077667
Copy link
Author

25077667 commented Sep 2, 2023

Remove seqlock/tests, which was generated.

I'm sorry for that commit. I fixed it.

#include <stdbool.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

#include "seqlock.h"

#define SEQLOCK_WRITER 1U

#if defined(__i386__) || defined(__x86_64__)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to #if !defined(__aarch64) for generic C11 Atomics path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants