Add some optimizations

[lukasz1992]

Mainly to reduce code size (and binary size).

[WXbet]

Positive:

• Significant reduction of code duplication and binary size
• Simpler, more maintainable code

Problematic:

1. Performance regression in hot path: The stream cipher kernel is the hottest code in the library. The #ifdef duplication was intentional - it produces two specialized functions without runtime branches. Now _dvbcsa_bs_stream_cipher_kernel is called with an int init parameter but is not static inline and is exported from a separate .c file. The compiler cannot optimize away the branches since it doesn't know whether init is 0 or 1 at compile time. This means unnecessary branches in every iteration of the inner loop.

2. Race condition in lazy init of kperm: if (!kperm[1]) is not thread-safe. If two threads call dvbcsa_key_schedule_block simultaneously, data races can occur. The old static table did not have this problem.

3. Sbox0 loop instead of unrolling: The manual unrolling was likely performance-conscious. The loop variant may be slower depending on compiler/architecture.

4. kperm is now mutable global state (.bss) instead of .rodata - no memory savings, just moved from read-only to mutable.

Conclusion:

The PR sacrifices performance for code aesthetics in a cryptographic low-level library where performance is the primary goal. The race condition is a real bug. For a project like libdvbcsa, which is designed for maximum throughput, I would not merge this PR as-is. The deduplication of dvbcsa_key_schedule_block/dvbcsa_load_le64 (commits 4-5) is harmless and reasonable, but the kernel changes (commits 1-3) are questionable.

[WXbet]

Recommendation: Reduce duplication without sacrificing performance

1. stream_cipher_kernel: Use static inline + constant propagation

Instead of a runtime int init parameter, use a macro or static inline with a compile-time constant:

// In header or same TU:
static inline void
_dvbcsa_bs_stream_cipher_kernel(int INIT, ...)
{
    if (INIT) { /* ... */ }
    else      { /* ... */ }
}

// Call sites:
_dvbcsa_bs_stream_cipher_kernel(1, ...);  // compiler eliminates dead branch
_dvbcsa_bs_stream_cipher_kernel(0, ...);

Since INIT is a literal constant at each call site, any modern compiler (-O2) will eliminate the dead branch entirely — same generated code as the duplicated version, but only one source copy.

2. kperm: Keep it static const (read-only)

The lazy-init approach introduces mutable global state, a potential data race, and no real benefit. If the table is too large to spell out, generate it at build time with a small script and #include the result — still const, still in .rodata, zero runtime cost.

3. sbox0: Let the compiler decide

Replace the manual unroll with a loop only if you verify (via objdump / benchmarks) that the compiler unrolls it anyway at -O2. If not, keep the explicit unroll or use #pragma GCC unroll / a macro-based unroll.

---

Summary:

The key fix is to keep the kernel function static inline in the same translation unit so the compiler can constant-fold the init parameter. The permutation table should stay static const (generated at build time if needed) to avoid mutable global state and thread-safety issues. Loop unrolling changes should be validated with benchmarks before changing. With these adjustments we get the same maintainability improvements without any performance regression.

[lukasz1992]

Understood, quite important notes.

1. I am not sure how to do it properly (to not export function to shared library and do not issue compilation warning). Let me focus on dropping double include and avoiding macros, as you said - keeping my idea.
2. I only meant reducing library size - IMHO compiling-code->executing code->compiling generated file is not a good-practice for such small library. And for avoiding data race libpthread would be needed (or using atomic functions, forcing the compilation to linux-only) - dropping the idea.
3. 5th line of configure.ac enables loop unrolling - so it is safe.

I rewrote the proposal of changes. Then I have done some benchmarking (x86_64/ssse3/avx2/native) but in any case I was unable to observe performance drops - no more than 0.2%. Tested only on x86_64 cpu.

[WXbet]

Let me check your changes later. Be aware of we are using this lib with aarch64, arm and mipsel architectures in embedded systems.

[WXbet]

The updated PR removes the kperm race condition (good) and reduces to 3 commits. Commit 2 (reuse _ecm functions) is clean and harmless. However:

1. Commit 3 has a critical correctness bug: The sbox0 extraction breaks the "ultimate carry" computation for regs->r. After removing the separate {} scoping blocks, tmp1 and tmp3 on the regs->r line now reference the rotate-left values (B[-1][1], B[-1][3]) instead of the Z/E carry values from the last sbox0 iteration. This silently produces incorrect cryptographic output.

2. Commit 1 is improved but still not guaranteed to optimize: the kernel function is static and called with constant args, so the compiler may clone and eliminate branches via IPA-CP — but it's a large function and not marked inline, so this is compiler-dependent. Adding static inline or __attribute__((always_inline)) would make the optimization reliable.

So check #5

[lukasz1992]

Thanks, it looks good. Oops, have not properly copied the correct version. Just updated.
PS: name sbox0 was just my guess, not sure whether it is appropiate.

In that case let me send only one commit, as you already put the rest in #5 (I put your commit also here, to avoid conflicts)