add `MP_SMALL_STACK_SIZE` option #538

sjaeckel · 2022-10-27T15:03:13Z

This adds an option to use a heap-buffer for the usually stack-based
MP_WARRAY-sized temporary buffers.

Per default it will reserve a single buffer, which can be modified

at compile-time via the MP_WARRAY_NUM define
at run-time by calling mp_warray_init()

The internal structure can only be created once. If one wants to modify
the maximum number of elements, the entire structure has to be free'd
by calling mp_warray_free().

In case one wants to use this option with multiple threads, one shall
use the mp_warray_init() function and pass appropriate locking functions.

Timings

op / algo	ECC	RSA
decrypt_key
encrypt_key
sign_hash
verify_hash

Related to: #441 #447 #511

sjaeckel · 2022-11-12T14:58:24Z

@jlaitine could you maybe have a look and tell me whether that's an acceptable solution for you?

czurnieden

Found nothing except some typos.

But the effect on the ECC computations is interesting!

.github/workflows/main.yml

doc/bn.tex

s_mp_warray_get.c

czurnieden · 2023-06-21T13:02:00Z

What happened to this PR? Did you change your mind? I find it quite interesting to have a choice between stack and heap for this. On the other side: it breaks our general thread-safety and needs locks.
But I'm still supportive.

sjaeckel · 2024-03-11T15:09:00Z

What happened to this PR? Did you change your mind?

No, I just didn't have the passion to follow up :)

On the other side: it breaks our general thread-safety and needs locks.

That's indeed a thing. I'm not sure whether it'd make sense to replace the locks by taking care of that ourselves and using lock-free/atomic operations under the hood!?

czurnieden · 2024-03-15T01:52:05Z

Sorry, took a bit longer to check.

Better like that!?

Yes, looks reasonable.

Caveat: I cannot test the Windows stuff now: my last PC here with Windows (what is the plural of Windows?) let the magic smoke out, need a visit to Ebay before going on.

PS: I am aware of my TODO list, it's not forgotten!

sjaeckel · 2024-03-15T08:24:33Z

what is the plural of Windows?

My SO just explained to me that the plural only works with the complete name! Microsoft Windows -> Microsofts Windows

my TODO list

No need to hurry, we have time.

test the Windows stuff

I'll see what I can do by extending CI.

czurnieden · 2024-03-15T23:05:11Z

My SO just explained to me that the plural only works with the complete name! Microsoft Windows -> Microsofts Windows

Ah, ok, my thanks to your SO!

I'll see what I can do by extending CI.

That would be really nice!

No need to hurry, we have time.

You have, but I'm an overweight old fart already! ;-)

My main problem is the error I got with the base2->base10 conversion with a single round of NR to compute the next divisor at a very large (close to MP_MAX_DIGIT_COUNT with MP_64BIT ) number. Sadly a randomly generated large number and it hasn't been logged what it was (Full HD. Yes, I'm an idiot). Haven't been able to repeat the error and am trying to compute the accumulated error of the NR rounds now manually. Fiddly.

We need to be able to do at most 22 rounds with n_0 = 1000 and the individual absolute error is about 2^(-34). But it is not floating point, it is integer math, so what is the accumulated error at the end? Does it even accumulate?

Was the aformentioned single error just a random bitflip? An error elsewhere?

*hng*!

sjaeckel · 2024-03-27T08:44:00Z

Ah, ok, my thanks to your SO!

You're very welcome ;-)

I'll see what I can do by extending CI.

wow, what a ride ... In between I was not sure whether I'd simply disable the feature for MSVC ... but now it seems to work^TM ...

This adds an option to use a heap-buffer for the usually stack-based `MP_WARRAY`-sized temporary buffers. Per default it will reserve a single buffer, which can be modified * at compile-time via the `MP_WARRAY_NUM` define * at run-time by calling `mp_warray_init()` The internal structure can only be created once. If one wants to modify the maximum number of elements, the entire structure has to be free'd by calling `mp_warray_free()`. In case one wants to use this option with multiple threads, one shall use the `mp_warray_init()` function and pass appropriate locking functions. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

`s_warray_init()` and `s_warray_free()` are not safe and MUST NOT be called from multiple threads. This also removes `MP_WARRAY_NUM`, since automatic initialization will not be safe for more than one thread. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

To be able to support this for MSVC as well, we have to move this into a separate private API function. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Output gets garbeled a bit, but we only care for the result which is `Tests OK/NOP/FAIL: 50/0/0`. Add `-Wno-incomplete-setjmp-declaration` since `clang-10` shipping with Ubuntu 20.04 seems broken... and `-Wno-unknown-warning-option` since `clang-8` doesn't know about this warning... Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

... and fix some MSVC related (and other) things. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

sjaeckel · 2024-03-27T15:30:15Z

@jlaitine can you please check if this works for you?

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

sjaeckel · 2024-04-03T15:47:35Z

In between I was not sure whether I'd simply disable the feature for MSVC ... but now it seems to work^TM ...

aaaand in the end I disabled it for MSVC since IMO the most elegant solution is via TLS and MSVC seems to have a problem with TLS in DLLs. I'm not 100% sure what's going wrong, but since it was going fine with the static library [0] I suspect that we'd have to implement what's described in [1] and I'm simply not willing to do that, nor would I accept a PR that implements it.

If I'm mistaken there and someone with more MSVC experience wants to chime in and provide an easy fix, I'll happily accept that :)

[0] https://ci.appveyor.com/project/libtom/libtommath/builds/49507598
[1] https://learn.microsoft.com/en-us/windows/win32/dlls/using-thread-local-storage-in-a-dynamic-link-library

czurnieden

Bearhug?

czurnieden · 2024-04-03T21:03:29Z

https://learn.microsoft.com/en-us/windows/win32/dlls/using-thread-local-storage-in-a-dynamic-link-library

So that's the superlativ of "unnecessary complicated", isn't it?

jlaitine · 2024-04-08T07:19:00Z

@jlaitine can you please check if this works for you?

Hi sorry for the very long response time, I have been busy with some other things. I see that this already got merged, so I tested the development branch directly and this change works perfectly!

I still have a small compilation error, in PX4, which is compiled with "-Wall". Not related to this PR. I created anoter PR out of that: #578

Thanks again,
Jukka

sjaeckel · 2024-04-08T10:20:48Z

Thanks a lot for the feedback! Glad to hear that it works for you :)

sjaeckel force-pushed the small-stacksize branch 11 times, most recently from d577ff8 to 75d8a23 Compare October 28, 2022 12:38

sjaeckel marked this pull request as ready for review October 28, 2022 12:42

sjaeckel force-pushed the small-stacksize branch from 75d8a23 to aab1c1f Compare October 28, 2022 13:50

sjaeckel requested a review from czurnieden March 31, 2023 12:05

czurnieden requested changes Mar 31, 2023

View reviewed changes

sjaeckel self-assigned this Apr 18, 2023

sjaeckel force-pushed the small-stacksize branch 2 times, most recently from 838b736 to f18c997 Compare March 11, 2024 14:32

sjaeckel force-pushed the small-stacksize branch 9 times, most recently from 3e1a7ce to 887b799 Compare March 14, 2024 14:40

sjaeckel force-pushed the small-stacksize branch 2 times, most recently from 2723f09 to d6dca63 Compare March 27, 2024 07:58

sjaeckel force-pushed the small-stacksize branch from d6dca63 to 21207c4 Compare March 27, 2024 13:52

sjaeckel force-pushed the small-stacksize branch from 21207c4 to c9b07aa Compare March 27, 2024 15:16

sjaeckel added 5 commits March 27, 2024 16:25

Add s_mp_cmpexch_n()

795f7ba

To be able to support this for MSVC as well, we have to move this into a separate private API function. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Add tests for MSVC multi-threading

fae9aa5

... and fix some MSVC related (and other) things. Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Update makefiles

334465d

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

sjaeckel force-pushed the small-stacksize branch from c9b07aa to 334465d Compare March 27, 2024 15:28

sjaeckel added 5 commits March 29, 2024 10:57

Use Thread Local Storage for warray buffer

33a9d0d

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

regen

513d48d

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Use appveyor build matrix

4c2e177

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Run small-stack tests with valgrind

6c70ef1

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

Disable MP_SMALL_STACK_SIZE on MSVC

3570e12

Signed-off-by: Steffen Jaeckel <s@jaeckel.eu>

sjaeckel force-pushed the small-stacksize branch from 1ea2413 to 3570e12 Compare April 3, 2024 14:23

sjaeckel requested a review from czurnieden April 3, 2024 15:47

czurnieden approved these changes Apr 3, 2024

View reviewed changes

sjaeckel merged commit a9a11d2 into develop Apr 4, 2024
83 checks passed

sjaeckel deleted the small-stacksize branch April 4, 2024 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `MP_SMALL_STACK_SIZE` option #538

add `MP_SMALL_STACK_SIZE` option #538

sjaeckel commented Oct 27, 2022 •

edited

Loading

sjaeckel commented Nov 12, 2022

czurnieden left a comment

czurnieden commented Jun 21, 2023

sjaeckel commented Mar 11, 2024

czurnieden commented Mar 15, 2024

sjaeckel commented Mar 15, 2024

czurnieden commented Mar 15, 2024

sjaeckel commented Mar 27, 2024

sjaeckel commented Mar 27, 2024

sjaeckel commented Apr 3, 2024

czurnieden left a comment

czurnieden commented Apr 3, 2024

jlaitine commented Apr 8, 2024

sjaeckel commented Apr 8, 2024

add MP_SMALL_STACK_SIZE option #538

add MP_SMALL_STACK_SIZE option #538

Conversation

sjaeckel commented Oct 27, 2022 • edited Loading

Timings

sjaeckel commented Nov 12, 2022

czurnieden left a comment

Choose a reason for hiding this comment

czurnieden commented Jun 21, 2023

sjaeckel commented Mar 11, 2024

czurnieden commented Mar 15, 2024

sjaeckel commented Mar 15, 2024

czurnieden commented Mar 15, 2024

sjaeckel commented Mar 27, 2024

sjaeckel commented Mar 27, 2024

sjaeckel commented Apr 3, 2024

czurnieden left a comment

Choose a reason for hiding this comment

czurnieden commented Apr 3, 2024

jlaitine commented Apr 8, 2024

sjaeckel commented Apr 8, 2024

add `MP_SMALL_STACK_SIZE` option #538

add `MP_SMALL_STACK_SIZE` option #538

sjaeckel commented Oct 27, 2022 •

edited

Loading