> > Technically the getrlimit() is perfectly ok to do on all platforms. But> we know glibc returns rlimit anway, so it can be skipped there.> > I would have put all of the base/size twiddling inside the if(). Then> struct rlimit does not need initialization either. But thats a minor> thingy. You care to fix that, or should I just apply this?>
Ok, let met fix, test and resubmit.
Thanks,
Marc
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---
> Le 23 févr. 2016 à 18:18, Marc Vertes <marc.vertes@ugrid.net> a écrit :> > >> >> Technically the getrlimit() is perfectly ok to do on all platforms. But>> we know glibc returns rlimit anway, so it can be skipped there.>> >> I would have put all of the base/size twiddling inside the if(). Then>> struct rlimit does not need initialization either. But thats a minor>> thingy. You care to fix that, or should I just apply this?>> > Ok, let met fix, test and resubmit.
Adjusting the base/size outside of if() is necessary if init is not
performed by main thread, which is the case for mongo shell. If you
agree with that, you can commit as it is.
Thanks
--
Marc
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---
On Tue, 23 Feb 2016 18:26:12 +0100
Marc Vertes <marc.vertes@ugrid.net> wrote:
> > Le 23 févr. 2016 à 18:18, Marc Vertes <marc.vertes@ugrid.net> a> > écrit :> > > > > >> > >> Technically the getrlimit() is perfectly ok to do on all> >> platforms. But we know glibc returns rlimit anway, so it can be> >> skipped there.> >> > >> I would have put all of the base/size twiddling inside the if().> >> Then struct rlimit does not need initialization either. But thats> >> a minor thingy. You care to fix that, or should I just apply this?> >> > > Ok, let met fix, test and resubmit. > > Adjusting the base/size outside of if() is necessary if init is not> performed by main thread, which is the case for mongo shell. If you> agree with that, you can commit as it is.
No. And that's actually wrong. If you adjust with 2mb for non-main
threads, you end up returning wrong info.
When adjustment is not done, base is just += size -= size, which is
no-op.
So the current patch is wrong. pthread_getattr_np returns valid and
correct data for non-main threads. Only main thread should get the
"treatment".
Thanks.
Also, sorry for pushing my earlier broken patch. I forgot to reset my
git tree before pushing the other patches out.
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---
> Le 23 févr. 2016 à 18:44, Timo Teras <timo.teras@iki.fi> a écrit :> > So the current patch is wrong. pthread_getattr_np returns valid and> correct data for non-main threads. Only main thread should get the> "treatment".>
Ok, I agree, there is no need to patch the reporting of stack base / size which are correct (obtained by pthread_getattr_np in a non-main thread).
So the real problem comes from the small default stack size (80k) imposed by musl.
I saw that reducing the quota from 64k to 32k in StackLocator.available() which checks remaining stack space, was enough to avoid the initial crash.
So the normal thing to do would be increase a bit the stack space for created threads.
I can't use pthread_attr_setstacksize before pthread_create, because of the use of C++ threads (what a pile of crap!). I tried to force a non-zero stacksize in PR_CreateThread(), but it seems to do nothing.
The 2 last remaining options: test if g++ -fsplit-stack would work, or reduce quota as mentioned before.
What do you think ?
Marc
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---
On Tue, 23 Feb 2016 23:46:39 +0100
Marc Vertes <marc.vertes@ugrid.net> wrote:
> > Le 23 févr. 2016 à 18:44, Timo Teras <timo.teras@iki.fi> a écrit :> > > > So the current patch is wrong. pthread_getattr_np returns valid and> > correct data for non-main threads. Only main thread should get the> > "treatment".> > > Ok, I agree, there is no need to patch the reporting of stack base /> size which are correct (obtained by pthread_getattr_np in a non-main> thread).> > So the real problem comes from the small default stack size (80k)> imposed by musl.> > I saw that reducing the quota from 64k to 32k in> StackLocator.available() which checks remaining stack space, was> enough to avoid the initial crash.
Should we start by applying that one? At least to make mongdb compile
again as my change broke the build (bad checksums).
> So the normal thing to do would be increase a bit the stack space for> created threads.> > I can't use pthread_attr_setstacksize before pthread_create, because> of the use of C++ threads (what a pile of crap!). I tried to force a> non-zero stacksize in PR_CreateThread(), but it seems to do nothing.
Urgh. Yeah, that looks abhorrent. I'll ping #musl what could be done.
> The 2 last remaining options: test if g++ -fsplit-stack would work,> or reduce quota as mentioned before.> > What do you think ?
split-stack has it's own problems too. IIRC, it breaks other stuff. And
requires gold linker to even work (we have it, but do not use it be
default).
glibc ships pthread_setattr_default_np() that can be used to change the
default stack size. I wonder if some similar mechanism could be added
to musl.
But as said, maybe start with changing the marginal from 64->32kB?
In musl the stack size does not contain the guard page (which is in
agreement with posix's definition of thread attribute stack base/size).
The comment did say the check is not done often enough though, so that
might not be desirable long term solution. And that should probably be
explained in the patch.
Thanks,
Timo
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---
> Le 24 févr. 2016 à 07:44, Timo Teras <timo.teras@iki.fi> a écrit :> > On Tue, 23 Feb 2016 23:46:39 +0100> Marc Vertes <marc.vertes@ugrid.net> wrote:> >>> Le 23 févr. 2016 à 18:44, Timo Teras <timo.teras@iki.fi> a écrit :>>> >>> So the current patch is wrong. pthread_getattr_np returns valid and>>> correct data for non-main threads. Only main thread should get the>>> "treatment".>>> >> Ok, I agree, there is no need to patch the reporting of stack base />> size which are correct (obtained by pthread_getattr_np in a non-main>> thread).>> >> So the real problem comes from the small default stack size (80k)>> imposed by musl.>> >> I saw that reducing the quota from 64k to 32k in>> StackLocator.available() which checks remaining stack space, was>> enough to avoid the initial crash.> > Should we start by applying that one? At least to make mongdb compile> again as my change broke the build (bad checksums).
I'm preparing a new patch, removing musl-process-stack-size.patch and
adjusting the check of minimum available stack space from 64k to 32k.
> >> So the normal thing to do would be increase a bit the stack space for>> created threads.>> >> I can't use pthread_attr_setstacksize before pthread_create, because>> of the use of C++ threads (what a pile of crap!). I tried to force a>> non-zero stacksize in PR_CreateThread(), but it seems to do nothing.> > Urgh. Yeah, that looks abhorrent. I'll ping #musl what could be done.> >> The 2 last remaining options: test if g++ -fsplit-stack would work,>> or reduce quota as mentioned before.>> >> What do you think ?> > split-stack has it's own problems too. IIRC, it breaks other stuff. And> requires gold linker to even work (we have it, but do not use it be> default).>
Ok, I won't proceed further on that.
> glibc ships pthread_setattr_default_np() that can be used to change the> default stack size. I wonder if some similar mechanism could be added> to musl.>
It would be certainly useful to ease porting multi-threaded C++ stuff needing
big stack space for their threads. It's not because alpine and musl
are concise and elegant that they should not support big bloated craps.
> But as said, maybe start with changing the marginal from 64->32kB?> In musl the stack size does not contain the guard page (which is in> agreement with posix's definition of thread attribute stack base/size).> The comment did say the check is not done often enough though, so that> might not be desirable long term solution. And that should probably be> explained in the patch.>
Yes. Will do that.
Thanks,
Marc
---
Unsubscribe: alpine-aports+unsubscribe@lists.alpinelinux.org
Help: alpine-aports+help@lists.alpinelinux.org
---