~alpine/aports

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
7 2

[alpine-aports] [PATCH] testing/mongodb: fix for #5117

Marc Vertes
Details
Message ID
<1456247469-958-1-git-send-email-marc.vertes@ugrid.net>
Sender timestamp
1456247469
DKIM signature
missing
Download raw message
Patch: +27 -17
---
 testing/mongodb/APKBUILD                      |  6 ++---
 testing/mongodb/musl-process-stack-size.patch | 38 +++++++++++++++++----------
 2 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/testing/mongodb/APKBUILD b/testing/mongodb/APKBUILD
index a177e91..aa51200 100644
--- a/testing/mongodb/APKBUILD
+++ b/testing/mongodb/APKBUILD
@@ -96,7 +96,7 @@ md5sums="a9f150f8bead9e8afd3ab28492167073  mongodb-src-r3.2.1.tar.gz
 04a348397be8ca7471d404056d8a1490  40-fix-elf-native-class.patch
 86a988b5d4402227d177b8a1167008e8  backtrace.patch
 cd0833592e3a23e729ebd71eb756318c  fix-asio-strerror_r.patch
-4a0b1b35ee0c6a5e5c35aee0097f4a07  musl-process-stack-size.patch
+40ccc56b2332347f200c22140867c1df  musl-process-stack-size.patch
 1df24dc2aa6b8f4c6da22f097a921ebb  boost160.patch
 7d2f94bed7bfacd32fcd52dfd931f077  mongodb.confd
 792a0b53b3e843cf14176c5beb8cdfe1  mongodb.initd
@@ -108,7 +108,7 @@ sha256sums="50431a3ba5ab68bd0bed4a157a8528ca27753a63cf101f13135255e4e9d42f15  mo
 3a863660113d29728d7a852b3fba73926697b496848f8ccc4d8e73e6614cfdfc  40-fix-elf-native-class.patch
 300d9f6b819730de54018d4b418eb7f921ba9c83fad4958a040544b076160848  backtrace.patch
 ec6d404221f02706ef2e52e00e414e98666dcc3606e78c9b3d33dfbd42a1eae7  fix-asio-strerror_r.patch
-a266b1b2397f069fedb2427a1989a30ed9f63a300ba8726ec50609e93920f76d  musl-process-stack-size.patch
+f8f6e9016b387defa2ec925f983149a37dc8f77ed5fe7f3cbd17de537758eddc  musl-process-stack-size.patch
 0e9da35f4303e53daf51e78961c517895f2d12f4fa49298f01e1665e15246d73  boost160.patch
 a4ca29c577428c02cd0b0a8b46756df5f53a05519c9d13c270533cf99b9b819d  mongodb.confd
 7e39fbd4dc18dba21c8767931683f4795e58e0e91b9f9a5842539923ded453c9  mongodb.initd
@@ -120,7 +120,7 @@ b9fdacb273d5a4e1e60735846b262287f84ca6cbded9393d182f69158d3162a50cae5d834f76860d
 56db8f43afc94713ac65b174189e2dd903b5e1eff0b65f1bdac159e52ad4de6606c449865d73bd47bffad6a8fc339777e2b11395596e9a476867d94a219c7925  40-fix-elf-native-class.patch
 7d097f497cb910c9cb81086cd8c222e43456d1a6de4adfe3e97a4d99add454279350fdeb7305dab84b3deca04afd824036d4065ee0fb8cdd8c03e96d98ee86af  backtrace.patch
 f829b1ad542db3ee776d444243b8b47ab4e48a7386d9b199d7b1adafd31556cf173a5683b78ee735d6a69999ad9af5ad152fde955bbe8865f7910718991ce97c  fix-asio-strerror_r.patch
-e8f41ea4df62b6588cdea64e1f57553e04c3c0b603dac68b49b222bc9fa8c7b6f6bc1654b5033318ad3ec73632848dfa0f03d33b3c8d8b523ef0145e461364b2  musl-process-stack-size.patch
+c4880c3a97ea1c668350a18b4f9ff4cec0a9e589a4c2079be0a49a7566703b8b816362a1cedd7a2e66b038d05fcc1871aed214097f0a89ef99d4ff19fb482041  musl-process-stack-size.patch
 385c82875174caae433a3b381eb10f98a6fed0c8943788ddefff1de80a898e480bdbbf094a7783285cf2ae11ce3fc6878e57d58183d05be2f0837b206aaa4da6  boost160.patch
 9bcd870742c31bf25f34188ddc3c414de1103e9860dea9f54eee276b89bc2cf1226abab1749c5cda6a6fb0880e541373754e5e83d63cc7189d4b9c274fd555c3  mongodb.confd
 2cb295ac0eb44acb4533c86d6d0988d5f760361a43cff7845713f3e2cf0e37023d23790a2926c613bf2b0668060c0b68d59000db52daaacd8af10e701a8b4192  mongodb.initd
diff --git a/testing/mongodb/musl-process-stack-size.patch b/testing/mongodb/musl-process-stack-size.patch
index dd45abb..df31abf 100644
--- a/testing/mongodb/musl-process-stack-size.patch
+++ b/testing/mongodb/musl-process-stack-size.patch
@@ -1,22 +1,32 @@
---- mongodb-src-r3.2.1.orig/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
-+++ mongodb-src-r3.2.1/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
-@@ -31,6 +31,7 @@
- #include "mongo/platform/stack_locator.h"
+--- a/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
+@@ -32,6 +32,12 @@
  
  #include <pthread.h>
-+#include <sys/resource.h>
  
++#if defined(__linux__) && !defined(__GLIBC__) /* Assume Musl libc */
++#include <sys/resource.h>
++#include <sys/syscall.h>
++#include <unistd.h>
++#endif
++
  #include "mongo/util/assert_util.h"
  #include "mongo/util/scopeguard.h"
-@@ -52,6 +53,11 @@
-     invariant(result == 0);
-     invariant(base != nullptr);
-     invariant(size != 0);
-+
-+    struct rlimit rl;
+ 
+@@ -48,6 +54,16 @@
+     size_t size = 0;
+ 
+     auto result = pthread_attr_getstack(&selfAttrs, &base, &size);
 +
-+    invariant(getrlimit(RLIMIT_STACK, &rl) == 0);
++#if defined(__linux__) && !defined(__GLIBC__) /* Assume Musl libc */
++    struct rlimit rl = { 0 };
++    base += size;
++	if (syscall(SYS_gettid) == getpid()) {	/* In the main thread */
++		invariant(getrlimit(RLIMIT_STACK, &rl) == 0);
++	}
 +    size = rl.rlim_cur ? : 2 * 1024 * 1024;
++    base -= size;
++#endif
  
-     // TODO: Assumes a downward growing stack. Note here that
-     // getstack returns the stack *base*, being the bottom of the
+     invariant(result == 0);
+     invariant(base != nullptr);
-- 
2.7.1



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Marc Vertes
Details
Message ID
<6B28E7B4-454F-4391-AA4E-498D73DD811A@ugrid.net>
In-Reply-To
<20160223191612.6f25f25b@vostro> (view parent)
Sender timestamp
1456247908
DKIM signature
missing
Download raw message
> 
> 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
---
Marc Vertes
Details
Message ID
<C0783129-1A80-4D3B-8D2F-6D76A733095E@ugrid.net>
In-Reply-To
<6B28E7B4-454F-4391-AA4E-498D73DD811A@ugrid.net> (view parent)
Sender timestamp
1456248372
DKIM signature
missing
Download raw message
> 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
---
Timo Teras
Details
Message ID
<20160223191612.6f25f25b@vostro>
In-Reply-To
<1456247469-958-1-git-send-email-marc.vertes@ugrid.net> (view parent)
Sender timestamp
1456247772
DKIM signature
missing
Download raw message
On Tue, 23 Feb 2016 17:11:09 +0000
Marc Vertes <marc.vertes@ugrid.net> wrote:

> mongodb-src-r3.2.1.orig/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
> -+++
> mongodb-src-r3.2.1/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
> -@@ -31,6 +31,7 @@
> - #include "mongo/platform/stack_locator.h"
> +--- a/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
> ++++ b/src/mongo/platform/stack_locator_pthread_getattr_np.cpp
> +@@ -32,6 +32,12 @@
>   
>   #include <pthread.h>
> -+#include <sys/resource.h>
>   
> ++#if defined(__linux__) && !defined(__GLIBC__) /* Assume Musl libc */
> ++#include <sys/resource.h>
> ++#include <sys/syscall.h>
> ++#include <unistd.h>
> ++#endif
> ++
>   #include "mongo/util/assert_util.h"
>   #include "mongo/util/scopeguard.h"
> -@@ -52,6 +53,11 @@
> -     invariant(result == 0);
> -     invariant(base != nullptr);
> -     invariant(size != 0);
> -+
> -+    struct rlimit rl;
> + 
> +@@ -48,6 +54,16 @@
> +     size_t size = 0;
> + 
> +     auto result = pthread_attr_getstack(&selfAttrs, &base, &size);
>  +
> -+    invariant(getrlimit(RLIMIT_STACK, &rl) == 0);
> ++#if defined(__linux__) && !defined(__GLIBC__) /* Assume Musl libc */
> ++    struct rlimit rl = { 0 };
> ++    base += size;
> ++	if (syscall(SYS_gettid) == getpid()) {	/* In the
> main thread */ ++		invariant(getrlimit(RLIMIT_STACK,
> &rl) == 0); ++	}
>  +    size = rl.rlim_cur ? : 2 * 1024 * 1024;
> ++    base -= size;
> ++#endif

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?


>   
> -     // TODO: Assumes a downward growing stack. Note here that
> -     // getstack returns the stack *base*, being the bottom of the
> +     invariant(result == 0);
> +     invariant(base != nullptr);



---
Unsubscribe:  alpine-aports+unsubscribe@lists.alpinelinux.org
Help:         alpine-aports+help@lists.alpinelinux.org
---
Timo Teras
Details
Message ID
<20160223194440.3fc44b9a@vostro>
In-Reply-To
<C0783129-1A80-4D3B-8D2F-6D76A733095E@ugrid.net> (view parent)
Sender timestamp
1456249480
DKIM signature
missing
Download raw message
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
---
Marc Vertes
Details
Message ID
<2309A9D8-2394-4092-8BF5-BA4E3C13870F@ugrid.net>
In-Reply-To
<20160223194440.3fc44b9a@vostro> (view parent)
Sender timestamp
1456267599
DKIM signature
missing
Download raw message
> 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
---
Timo Teras
Details
Message ID
<20160224084437.2b11cbe1@vostro.util.wtbts.net>
In-Reply-To
<2309A9D8-2394-4092-8BF5-BA4E3C13870F@ugrid.net> (view parent)
Sender timestamp
1456296277
DKIM signature
missing
Download raw message
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
---
Marc Vertes
Details
Message ID
<7AA15BC7-7B5F-4973-ADEC-6ADCADECD11F@ugrid.net>
In-Reply-To
<20160224084437.2b11cbe1@vostro.util.wtbts.net> (view parent)
Sender timestamp
1456306379
DKIM signature
missing
Download raw message
> 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
---