Skip to content

knot: drop kru.inc.c static_assert that requires lock-free 16-bit atomics#29572

Merged
commodo merged 2 commits into
openwrt:masterfrom
commodo:fix-knot
May 30, 2026
Merged

knot: drop kru.inc.c static_assert that requires lock-free 16-bit atomics#29572
commodo merged 2 commits into
openwrt:masterfrom
commodo:fix-knot

Conversation

@commodo
Copy link
Copy Markdown
Contributor

@commodo commodo commented May 28, 2026

📦 Package Details

Maintainer: : @salzmdan , @Payne-X6

Description:

The RRL (Rate Limiting) module's kru.inc.c contains a static assertion
that ATOMIC_CHAR16_T_LOCK_FREE == 2, requiring lock-free 16-bit atomic
operations. ARMv5 processors like arm926ej-s provide no hardware
atomic primitives, causing the assertion to fail at compile time.

Detect pre-ARMv7 ARM targets by checking for the absence of armv7/armv8
in TARGET_CFLAGS and disable the RRL module for those builds.

🧪 Run Testing Details

  • OpenWrt Version:
  • OpenWrt Target/Subtarget:
  • OpenWrt Device:

✅ Formalities

  • I have reviewed the CONTRIBUTING.md file for detailed contributing guidelines.

@commodo
Copy link
Copy Markdown
Contributor Author

commodo commented May 28, 2026

Should hopefully fix
https://downloads.openwrt.org/snapshots/faillogs/arm_arm926ej-s/packages/knot/compile.txt

salzmdan
salzmdan reviewed May 28, 2026
View reviewed changes
Comment thread net/knot/test.sh Outdated
@commodo
Copy link
Copy Markdown
Contributor Author

commodo commented May 29, 2026

will merge this in 1-2 days if no objections

@Payne-X6
Copy link
Copy Markdown
Contributor

Payne-X6 commented May 29, 2026 •
edited
Loading

We would prefer a patch that would disable this static assert.

diff --git a/src/knot/modules/rrl/kru.inc.c b/src/knot/modules/rrl/kru.inc.c
index a05c1e999..4efbe2b25 100644
--- a/src/knot/modules/rrl/kru.inc.c
+++ b/src/knot/modules/rrl/kru.inc.c
@@ -462,7 +462,6 @@ static inline bool kru_limited_update(struct kru *kru, struct query_ctx *ctx, bo
                load_at = (_Atomic uint16_t *)ctx->load;
        }

-       static_assert(ATOMIC_CHAR16_T_LOCK_FREE == 2, "insufficient atomics");
        const uint16_t price = ctx->price16;
        const uint32_t limit = ctx->limit16;  // 2^16 has to be representable
        uint16_t load_orig = atomic_load_explicit(load_at, memory_order_relaxed);

We have already decided to disable this assertion in future versions, and the only reason it is still included is out of concern for performance.

@commodo
Copy link
Copy Markdown
Contributor Author

commodo commented May 29, 2026

Will spin-up a patch like that.

commodo added a commit to commodo/packages that referenced this pull request May 29, 2026
@commodo
knot: drop kru.inc.c static_assert that requires lock-free 16-bit ato…
3cce39b 
…mics

src/knot/modules/rrl/kru.inc.c contains

  static_assert(ATOMIC_CHAR16_T_LOCK_FREE == 2, "insufficient atomics");

which fails at compile time on ARMv5 (e.g. arm926ej-s) and any other
target whose toolchain does not advertise lock-free 16-bit atomics,
breaking the RRL (Rate Limiting) module build for those arches.

Per the upstream maintainer's review of PR openwrt#29572,
this static assertion is intended to be dropped in a future release
and is only still present out of concern for performance, not
correctness — they recommended a patch over a Makefile-level RRL
disable. Replace the previous `--with-module-rrl=no` approach for
pre-ARMv7 ARM targets with the upstream-recommended source patch
so RRL keeps working on every target the rest of knot already
builds for.

Bump PKG_RELEASE for the patch swap.

Suggested-by: Daniel Salzman <daniel.salzman@nic.cz>
Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
@commodo commodo changed the title knot: disable RRL module on pre-ARMv7 targets (no lock-free atomics) knot: drop kru.inc.c static_assert that requires lock-free 16-bit atomics May 29, 2026
commodo added 2 commits May 29, 2026 19:19
@commodo
knot: drop kru.inc.c static_assert that requires lock-free 16-bit ato…
ca2ee27 
…mics

The RRL module's kru.inc.c has a `static_assert(ATOMIC_CHAR16_T_LOCK_FREE
== 2, ...)` that breaks the build on any target whose toolchain does
not advertise lock-free 16-bit atomics (e.g. ARMv5 arm926ej-s).
Upstream considers the assertion non-essential and plans to drop it;
they recommended a patch over a Makefile-level RRL disable. Bump
PKG_RELEASE for the patch swap.

Suggested-by: Daniel Salzman <daniel.salzman@nic.cz>
Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
@commodo
knot: add functional test.sh covering all CLI subpackages
f8139a6 
Cover each user-facing subpackage with a real functional check instead
of relying solely on the CI's generic --version probe:

- knot:           knotc conf-check on a minimal YAML server config
- knot-dig:       kdig -h (CLI parser smoke check)
- knot-host:      khost -h
- knot-nsupdate:  feed 'quit' through the REPL
- knot-zonecheck: validate a minimal example.com zone file end to end
- knot-keymgr:    initialise a KASP DB in a temp directory

knot-libs, knot-libzscanner, and knot-tests are library/harness
subpackages; the generic ELF/SONAME checks already cover them.

Signed-off-by: Alexandru Ardelean <alex@shruggie.ro>
@commodo commodo merged commit cd7c9dd into openwrt:master May 30, 2026
12 checks passed
@commodo commodo deleted the fix-knot branch May 30, 2026 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@salzmdan salzmdan salzmdan left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@commodo @Payne-X6 @salzmdan