[LTP] [PATCH] netstress: explicitly set a thread stack size

[LTP] [PATCH] netstress: explicitly set a thread stack size

[j.nixdorf]

Musl libc uses a relatively small thread stack size (128k [1]). This
gets used up on 2 local buffers sized max_msg_len (64k by default),
which causes a segfault due to a stack overflow in the error reporting
path.

Set the stack size to 128kB + 2*max_msg_len instead, which is enough for
both buffers with an additional allowance for the remaining stack usage
by netstress and called libc or ltp helper functions.

[1]: https://wiki.musl-libc.org/functional-differences-from-glibc.html#Thread_stack_size

Signed-off-by: Johannes Nixdorf <j.nixdorf@avm.de>
---
 testcases/network/netstress/netstress.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index e79e64220..bcd3cd049 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -480,7 +480,7 @@ static void client_init(void)
 	clock_gettime(CLOCK_MONOTONIC_RAW, &tv_client_start);
 	intptr_t i;
 	for (i = 0; i < clients_num; ++i)
-		SAFE_PTHREAD_CREATE(&thread_ids[i], 0, client_fn, (void *)i);
+		SAFE_PTHREAD_CREATE(&thread_ids[i], &attr, client_fn, (void *)i);
 }
 
 static void client_run(void)
@@ -747,8 +747,6 @@ static void server_run(void)
 	struct sockaddr_in6 addr6;
 	socklen_t addr_size = sizeof(addr6);
 
-	pthread_attr_init(&attr);
-
 	/*
 	 * detaching threads allow to reclaim thread's resources
 	 * once a thread finishes its work.
@@ -980,6 +978,14 @@ static void setup(void)
 	break;
 	}
 
+	errno = pthread_attr_init(&attr);
+	if (errno != 0)
+		tst_brk(TBROK | TERRNO, "pthread_attr_init failed");
+
+	errno = pthread_attr_setstacksize(&attr, 128*1024 + 2*max_msg_len);
+	if (errno != 0)
+		tst_brk(TBROK | TERRNO, "pthread_attr_setstacksize failed");
+
 	net.init();
 }
 
-- 
2.17.1

[LTP] [PATCH] netstress: explicitly set a thread stack size

[Alexey Kodanev]

On 25.11.2020 16:06, Johannes Nixdorf via ltp wrote:
> Musl libc uses a relatively small thread stack size (128k [1]). This
> gets used up on 2 local buffers sized max_msg_len (64k by default),
> which causes a segfault due to a stack overflow in the error reporting
> path.
> 
> Set the stack size to 128kB + 2*max_msg_len instead, which is enough for
> both buffers with an additional allowance for the remaining stack usage
> by netstress and called libc or ltp helper functions.
> 
> [1]: https://urldefense.com/v3/__https://wiki.musl-libc.org/functional-differences-from-glibc.html*Thread_stack_size__;Iw!!GqivPVa7Brio!JiKwkv8uMgJNgh9ARntUpseI9zZW6h6u3Pm6AeF_sgqVyDcsHIQ_MyJWVVKbh_rVk-8u$ 
> 

Hi Johannes,

> +	errno = pthread_attr_init(&attr);
> +	if (errno != 0)

if (errno = pthread_attr_init(&attr))

> +		tst_brk(TBROK | TERRNO, "pthread_attr_init failed");
> +
> +	errno = pthread_attr_setstacksize(&attr, 128*1024 + 2*max_msg_len);

Since max_msg_len is 65535, the result won't be even 4 bytes aligned,
perhaps using just 256 * 1024?


> +	if (errno != 0)
> +		tst_brk(TBROK | TERRNO, "pthread_attr_setstacksize failed");

In case of error, it would be good to print the used stack size.

[LTP] [PATCH] netstress: explicitly set a thread stack size

[j.nixdorf]

Hi Alexey,

On Thu, Nov 26, 2020 at 05:05:14PM +0300, Alexey Kodanev wrote:
> > +		tst_brk(TBROK | TERRNO, "pthread_attr_init failed");
> > +
> > +	errno = pthread_attr_setstacksize(&attr, 128*1024 + 2*max_msg_len);
> 
> Since max_msg_len is 65535, the result won't be even 4 bytes aligned,
> perhaps using just 256 * 1024?

The function pthread_attr_setstacksize does not have any alignment
requirements specified and only sets the minimum stack size. This means
the libc is required to over-allocate and suitably align the stack to
match platform requirements. Is this broken on any libraries the LTP
project cares about?

Note that this is different from pthread_attr_setstack, as there the
memory region is provided by the caller and the libc can't change the
alignment later on.

I'm reluctant to use a static value here as max_msg_len may be modified
by command line arguments.

Regards,
Johannes

[LTP] [PATCH] netstress: explicitly set a thread stack size

[Cyril Hrubis]

Hi!
> I'm reluctant to use a static value here as max_msg_len may be modified
> by command line arguments.

Wouldn't it be easier to use malloc() instead of putting large data
structures on the stack and working around that by increasing the
limits?

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] netstress: explicitly set a thread stack size

[Alexey Kodanev]

On 26.11.2020 18:03, Cyril Hrubis wrote:
> Hi!
>> I'm reluctant to use a static value here as max_msg_len may be modified
>> by command line arguments.
> 
> Wouldn't it be easier to use malloc() instead of putting large data
> structures on the stack and working around that by increasing the
> limits?

In the current server implementation it would hurt performance, so
I assume it would also require to make a memory/thread-pool...

[LTP] [PATCH] netstress: explicitly set a thread stack size

[Cyril Hrubis]

Hi!
> >> I'm reluctant to use a static value here as max_msg_len may be modified
> >> by command line arguments.
> > 
> > Wouldn't it be easier to use malloc() instead of putting large data
> > structures on the stack and working around that by increasing the
> > limits?
> 
> In the current server implementation it would hurt performance, so
> I assume it would also require to make a memory/thread-pool...

Okay, then increasing limits sound like a reasonable workaround for the
time being.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] netstress: explicitly set a thread stack size

[Alexey Kodanev]

On 26.11.2020 17:52, Johannes Nixdorf via ltp wrote:
> Hi Alexey,
> 
> On Thu, Nov 26, 2020 at 05:05:14PM +0300, Alexey Kodanev wrote:
>>> +		tst_brk(TBROK | TERRNO, "pthread_attr_init failed");
>>> +
>>> +	errno = pthread_attr_setstacksize(&attr, 128*1024 + 2*max_msg_len);
>>
>> Since max_msg_len is 65535, the result won't be even 4 bytes aligned,
>> perhaps using just 256 * 1024?
> 
> The function pthread_attr_setstacksize does not have any alignment
> requirements specified and only sets the minimum stack size. This means
> the libc is required to over-allocate and suitably align the stack to
> match platform requirements. Is this broken on any libraries the LTP
> project cares about?

It doesn't mean you should intentionally pass unaligned size.

And on it's man-page [1], in errors section, there is a note that it might
return EINVAL for some systems if the stack size not multiple of page size.

[1] https://man7.org/linux/man-pages/man3/pthread_attr_setstacksize.3.html

> 
> Note that this is different from pthread_attr_setstack, as there the
> memory region is provided by the caller and the libc can't change the
> alignment later on.
> 
> I'm reluctant to use a static value here as max_msg_len may be modified
> by command line arguments.

max_msg_len is const.

[LTP] [PATCH] netstress: explicitly set a thread stack size

[j.nixdorf]

-----"Alexey Kodanev" <alexey.kodanev@oracle.com> wrote: -----
>On 26.11.2020 17:52, Johannes Nixdorf via ltp wrote:
>It doesn't mean you should intentionally pass unaligned size.
>
>And on it's man-page [1], in errors section, there is a note that it
>might
>return EINVAL for some systems if the stack size not multiple of page
>size.

Ok. I was going by the POSIX man page [1], which doesn't mention
that practice.

[1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_attr_setstacksize.html

>max_msg_len is const.

It seems like I was misreading [2] to mean it sets max_msg_len
instead of using it as a limit.

[2]: https://github.com/linux-test-project/ltp/blob/master/testcases/network/netstress/netstress.c#L854

Thanks for the review, I'll change it to go with your proposed
static limit.

Regards,
Johannes

[LTP] [PATCH v2] netstress: explicitly set a thread stack size

[j.nixdorf]

Musl libc uses a relatively small thread stack size (128k [1]). This
gets used up on 2 local buffers sized max_msg_len (64k by default),
which causes a segfault due to a stack overflow in the error reporting
path.

Set the stack size to 256k instead, which is enough for both buffers
with an additional allowance for the remaining stack usage by netstress
and called libc or ltp helper functions.

[1]: https://wiki.musl-libc.org/functional-differences-from-glibc.html#Thread_stack_size

Signed-off-by: Johannes Nixdorf <j.nixdorf@avm.de>

---

v2:
  - Use a static limit of 256k.
  - Document the requested stack size in the error message.
  - Coding style fixup.

 testcases/network/netstress/netstress.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/testcases/network/netstress/netstress.c b/testcases/network/netstress/netstress.c
index e79e64220..48c0c23dc 100644
--- a/testcases/network/netstress/netstress.c
+++ b/testcases/network/netstress/netstress.c
@@ -480,7 +480,7 @@ static void client_init(void)
 	clock_gettime(CLOCK_MONOTONIC_RAW, &tv_client_start);
 	intptr_t i;
 	for (i = 0; i < clients_num; ++i)
-		SAFE_PTHREAD_CREATE(&thread_ids[i], 0, client_fn, (void *)i);
+		SAFE_PTHREAD_CREATE(&thread_ids[i], &attr, client_fn, (void *)i);
 }
 
 static void client_run(void)
@@ -747,8 +747,6 @@ static void server_run(void)
 	struct sockaddr_in6 addr6;
 	socklen_t addr_size = sizeof(addr6);
 
-	pthread_attr_init(&attr);
-
 	/*
 	 * detaching threads allow to reclaim thread's resources
 	 * once a thread finishes its work.
@@ -980,6 +978,12 @@ static void setup(void)
 	break;
 	}
 
+	if (errno = pthread_attr_init(&attr))
+		tst_brk(TBROK | TERRNO, "pthread_attr_init failed");
+
+	if (errno = pthread_attr_setstacksize(&attr, 256*1024))
+		tst_brk(TBROK | TERRNO, "pthread_attr_setstacksize(256*1024) failed");
+
 	net.init();
 }
 
-- 
2.17.1

[LTP] [PATCH v2] netstress: explicitly set a thread stack size

[Alexey Kodanev]

On 27.11.2020 14:38, Johannes Nixdorf via ltp wrote:
> Musl libc uses a relatively small thread stack size (128k [1]). This
> gets used up on 2 local buffers sized max_msg_len (64k by default),
> which causes a segfault due to a stack overflow in the error reporting
> path.
> 
> Set the stack size to 256k instead, which is enough for both buffers
> with an additional allowance for the remaining stack usage by netstress
> and called libc or ltp helper functions.
> 
> [1]: https://urldefense.com/v3/__https://wiki.musl-libc.org/functional-differences-from-glibc.html*Thread_stack_size__;Iw!!GqivPVa7Brio!Nat9ZPBR2P2QxOM2IsBWR2WdfOQ1ZNM4IJzjNy5a_bTsHUko9bULz88kJAVK8yQ7Kk8-$ 
> 
> Signed-off-by: Johannes Nixdorf <j.nixdorf@avm.de>
> 
> ---
> 
> v2:
>   - Use a static limit of 256k.
>   - Document the requested stack size in the error message.
>   - Coding style fixup.
> 

Added extra parenthesis to silence possible compiler warnings
and applied, thanks Johannes!