cathugger/mkp224o

Make -B the default and remove the scary beta tag

Raymo111 opened this issue · 13 comments

Batch mode.

Will it miss certain valid matches, generate invalid onions, or crash the system?

nah. pretty sure it's okay now. was marked experimental initially because it was new codepath and I'm a bit paranoid about new stuff, and also it initially didn't support password seed mode.
now that it's been around for a while, and password seed stuff has been properly done, I guess it can be enabled by default for next release.

Gotcha. I'm running using -B for a 9-letter prefix so I'll let you know how it goes off you want; no issues so far :)

@cathugger everything works! I got the domain and it was super fast :)

I actually get a segfault in batch mode. The key difference is probably that I'm using musl.

After a brief investigation: this is because musl by default only provides a 128 KiB stack, which is not enough for the batched worker. I see that there is defined-out code that sets stack size. I would propose to re-enable that, since it seems that the code does implicitly rely on a larger-than-usual stack (in my testing 512 KiB works for default batch size). Is there a reason doing so was undesirable (thus defined out)?

I actually get a segfault in batch mode.

ooh right this one. I was sort-of aware of it after testing on alpine, but kinda forgot it at some point...

this is because musl by default only provides a 128 KiB stack, which is not enough for the batched worker.

correct.

Is there a reason doing so was undesirable (thus defined out)?

I actually did that code quite a bit before batch mode, to minimize stack usage, as glibc defaults are unnecessarily wasteful, but then realized that too low values crash stuff when pcre is used, and that benefit isn't enough to bother.
Adjusting values to be higher should mitigate this, though I'm not sure what value will work in all situations and whether taking into account batch size is worth it or just setting it high enough for reasonable scenarios will be good enough.
I'll do some testing I guess.

Alternatively, I could just switch to malloc for batch workers.

current master contains stuff to set thread size and some other tweaks.
right now it will set 256KiB + whatever batch arrays take up, aligned to 64KiB, and results in something like 600K with default settings iirc.
testing would be appreciated.
hopefully it'll be enough.

Thanks! Tested batch/nobatch x regex/noregex on musl, everything seems to work. The only change that was necessary is actually applying tattrp to the created threads, otherwise everything just segfaults:

index 22f9cfe..67710e0 100644
--- a/main.c
+++ b/main.c
@@ -543,7 +543,7 @@ int main(int argc,char **argv)
                tp = &VEC_BUF(stats,i);
 #endif
                tret = pthread_create(
-                       &VEC_BUF(threads,i),0,
+                       &VEC_BUF(threads,i),tattrp,
 #ifdef PASSPHRASE
                                deterministic
                                        ? (wt == WT_BATCH

hah thanks for catching that. pushed fix.

note that this has been sorta fixed (-B is now the default). will probably close when release new ver. or submitter may close if they desire idk..

Cool! Glad to see it :)

I'll close this now then, I trust you to put it in the new ver (release notes?)