Increase PTHREAD_KEYS_MAX

[Emmanuel Dreyfus]

Hello

pthread_key_create() will refuse to create to create more than
PTHREAD_KEYS_MAX pthread_key_t. I encountered situations
where some Apache setup with various modules could not work
reliabily on NetBSD because the value is too low. From time
to time the server enters a state where it will not be able
to serve requests.

POSIX mandates PTHREAD_KEYS_MAX to be at least _POSIX_THREAD_KEYS_MAX,
which should be 128 according to POSIX:
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html
Oddly it is 256 in NetBSD's <limit.h>

The values are:
NetBSD 256
Linux: 1024
MacOS X: 512

Obviously we lag behind and software will suit NetBSD less and less over the
time. Since it is often the result of shared usage by modules from different
developers, we cannot blame anyone for overflowing _POSIX_THREAD_KEYS_MAX
usage as a requirement.

I suggest for netbsd-7
- fix _POSIX_THREAD_KEYS_MAX to be 128 instad of 256
- increase PTHREAD_KEYS_MAX to 2048 so that we lead again for some time
this would increase memory footprint of programs linked with -lpthread
of 14 kB on ILP32 systems, and 28 kB on LP64 systems.

And for later:
- Think about a dynamic allocation. For now we have an array, could we
have a queue?

[Joerg Sonnenberger]

Post by Emmanuel Dreyfus
Obviously we lag behind and software will suit NetBSD less and less over the
time. Since it is often the result of shared usage by modules from different
developers, we cannot blame anyone for overflowing _POSIX_THREAD_KEYS_MAX
usage as a requirement.

Well, they could be using TLS as well, which doesn't have that problem.

Post by Emmanuel Dreyfus
I suggest for netbsd-7
- fix _POSIX_THREAD_KEYS_MAX to be 128 instad of 256

This doesn't make sense. It should reflect the actual limit.

Post by Emmanuel Dreyfus
- increase PTHREAD_KEYS_MAX to 2048 so that we lead again for some time
this would increase memory footprint of programs linked with -lpthread
of 14 kB on ILP32 systems, and 28 kB on LP64 systems.

I am against raising it that much. The overhead is *per thread*, so it
sums up easily.

Post by Emmanuel Dreyfus
- Think about a dynamic allocation. For now we have an array, could we
have a queue?

A queue doesn't make sense, O(1) is really required here. On-demand
growing of the array shouldn't be too hard to implement though.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Martin Husemann]

Post by Joerg Sonnenberger
I am against raising it that much. The overhead is *per thread*, so it
sums up easily.

For -7 we should do nothing, way too late in the game.

Post by Joerg Sonnenberger
A queue doesn't make sense, O(1) is really required here. On-demand
growing of the array shouldn't be too hard to implement though.

That would be my vote too.

Martin

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
- fix _POSIX_THREAD_KEYS_MAX to be 128 instad of 256

This doesn't make sense. It should reflect the actual limit.

The actual limit is PTHREAD_KEYS_MAX. _POSIX_THREAD_KEYS_MAX
is the constant defined in the standard.

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
- increase PTHREAD_KEYS_MAX to 2048 so that we lead again for some time
this would increase memory footprint of programs linked with -lpthread
of 14 kB on ILP32 systems, and 28 kB on LP64 systems.

I am against raising it that much. The overhead is *per thread*, so it
sums up easily.

This is not my understanding of both the code and the documentation.
pthread_key_create(3) saus:
The pthread_key_create() function creates a thread-specific data key
visible to all threads in the process.

The data attached to a pthread_key_t is thread-specific. The array is
not.

Post by Joerg Sonnenberger
A queue doesn't make sense, O(1) is really required here. On-demand
growing of the array shouldn't be too hard to implement though.

I am not sure if we can use realloc() there. Even if we can, it means
we have to call malloc() on first usage.

[Paul Goyette]

Post by Emmanuel Dreyfus

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
- fix _POSIX_THREAD_KEYS_MAX to be 128 instad of 256

This doesn't make sense. It should reflect the actual limit.

The actual limit is PTHREAD_KEYS_MAX. _POSIX_THREAD_KEYS_MAX
is the constant defined in the standard.

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
- increase PTHREAD_KEYS_MAX to 2048 so that we lead again for some time
this would increase memory footprint of programs linked with -lpthread
of 14 kB on ILP32 systems, and 28 kB on LP64 systems.

I am against raising it that much. The overhead is *per thread*, so it
sums up easily.

This is not my understanding of both the code and the documentation.
The pthread_key_create() function creates a thread-specific data key
visible to all threads in the process.
The data attached to a pthread_key_t is thread-specific. The array is
not.

Post by Joerg Sonnenberger
A queue doesn't make sense, O(1) is really required here. On-demand
growing of the array shouldn't be too hard to implement though.

I am not sure if we can use realloc() there. Even if we can, it means
we have to call malloc() on first usage.

I would guess that you wouldn't have to reallocate very often. Could
you maybe do something like what the SYSV* code does? Allocate one big
chunk of memory, and if you have to grow it, you allocate a bigger
chunk, migrate all the old stuff to the new chunk (one entry at a time),
and then free the old chunk?

Yup, it's not elegant, but it would not be a frequent occurrence. You
simply need to use an "appropriate" increment.



-------------------------------------------------------------------------
| Paul Goyette | PGP Key fingerprint: | E-mail addresses: |
| (Retired) | FA29 0E3B 35AF E8AE 6651 | paul at whooppee.com |
| Kernel Developer | 0786 F758 55DE 53BA 7731 | pgoyette at netbsd.org |
-------------------------------------------------------------------------

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

I would guess that you wouldn't have to reallocate very often. Could you
maybe do something like what the SYSV* code does? Allocate one big chunk of
memory, and if you have to grow it, you allocate a bigger chunk, migrate all
the old stuff to the new chunk (one entry at a time), and then free the old
chunk?

Isn't this exactly what realloc(3) does?

[Christos Zoulas]

Post by Emmanuel Dreyfus
Hello
pthread_key_create() will refuse to create to create more than
PTHREAD_KEYS_MAX pthread_key_t. I encountered situations
where some Apache setup with various modules could not work
reliabily on NetBSD because the value is too low. From time
to time the server enters a state where it will not be able
to serve requests.
POSIX mandates PTHREAD_KEYS_MAX to be at least _POSIX_THREAD_KEYS_MAX,
http://pubs.opengroup.org/onlinepubs/009695399/basedefs/limits.h.html
Oddly it is 256 in NetBSD's <limit.h>
NetBSD 256
Linux: 1024
MacOS X: 512
Obviously we lag behind and software will suit NetBSD less and less over the
time. Since it is often the result of shared usage by modules from different
developers, we cannot blame anyone for overflowing _POSIX_THREAD_KEYS_MAX
usage as a requirement.
I suggest for netbsd-7
- fix _POSIX_THREAD_KEYS_MAX to be 128 instad of 256
- increase PTHREAD_KEYS_MAX to 2048 so that we lead again for some time
this would increase memory footprint of programs linked with -lpthread
of 14 kB on ILP32 systems, and 28 kB on LP64 systems.
- Think about a dynamic allocation. For now we have an array, could we
have a queue?

Just make it an array that is dynamically allocated based on an environment
variable; this is what others do (it seems).

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Post by Christos Zoulas
Just make it an array that is dynamically allocated based on an environment
variable; this is what others do (it seems).

What name for the variable?

[Emmanuel Dreyfus]

Post by Christos Zoulas
Just make it an array that is dynamically allocated based on an environment
variable; this is what others do (it seems).

This is quickly getting ugly: malloc() calls pthread_key_create(),
which means we cannot allocate though the standard way until we have
allocated.

[Emmanuel Dreyfus]

Post by Emmanuel Dreyfus
This is quickly getting ugly: malloc() calls pthread_key_create(),

There is a way out, though. Is it reasonable?

Index: pthread.c
===================================================================
RCS file: /cvsroot/src/lib/libpthread/pthread.c,v
retrieving revision 1.145
diff -U 8 -r1.145 pthread.c
--- pthread.c 16 Dec 2014 20:05:54 -0000 1.145
+++ pthread.c 20 May 2015 09:40:12 -0000
@@ -160,16 +160,26 @@
void
pthread__init(void)
{
pthread_t first;
char *p;
int i;
extern int __isthreaded;

+ /*
+ * Allocate pthread_keys descriptors before
+ * reseting __uselibcstub because otherwise
+ * malloc() will call pthread_keys_create()
+ * while pthread_keys descriptors are not
+ * yet allocated.
+ */
+ if (pthread_tsd_init() != 0)
+ err(1, "Cannot allocate pthread_keys descriptors");
+
__uselibcstub = 0;

pthread__pagesize = (size_t)sysconf(_SC_PAGESIZE);
pthread__concurrency = (int)sysconf(_SC_NPROCESSORS_CONF);

/* Initialize locks first; they're needed elsewhere. */
pthread__lockprim_init();
for (i = 0; i < NHASHLOCK; i++) {

[Emmanuel Dreyfus]

I think it is reasonable.

Here is the complete patch.
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch

I restored the standard mandated value for _POSIX_THREAD_KEYS_MAX,
while defining PTHREAD_KEYS_MAX as the backward-compatible value
we have been uing for a while.

[Joerg Sonnenberger]

Post by Emmanuel Dreyfus

I think it is reasonable.

Here is the complete patch.
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch
I restored the standard mandated value for _POSIX_THREAD_KEYS_MAX,
while defining PTHREAD_KEYS_MAX as the backward-compatible value
we have been uing for a while.

This will just overflow the pthread structure? Besides, use strtoi,
calloc etc.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Post by Joerg Sonnenberger
This will just overflow the pthread structure?

What do you mean?

[Emmanuel Dreyfus]

Post by Christos Zoulas
Look for PTHREAD_KEYS_MAX in pthread_int.h.

That one will be harsh, as mallocs are not be possible anymore there.

[Christos Zoulas]

Look for PTHREAD_KEYS_MAX in pthread_int.h.

christos

Post by Emmanuel Dreyfus

Post by Joerg Sonnenberger
This will just overflow the pthread structure?

What do you mean?
--
Emmanuel Dreyfus

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch

This will just overflow the pthread structure? Besides, use strtoi,
calloc etc.

Here is an updated version using strtoll, calloc and taking care
of struct __pthread_st integrity:
http://ftp.espci.fr/shadow/manu/pthread_keys_max3.patch

[Christos Zoulas]

Post by Emmanuel Dreyfus

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch

This will just overflow the pthread structure? Besides, use strtoi,
calloc etc.

Here is an updated version using strtoll, calloc and taking care
http://ftp.espci.fr/shadow/manu/pthread_keys_max3.patch

Looks pretty good, except:

pthread_keys_max = strtol(pkm, (char **)NULL, 10);

Why cast NULL, why limit to decimal, this should be unsigned...?
Perhaps also limit it to a reasonable value.

if (pthread__tsd_list)
free(pthread__tsd_list);

No need to check for NULL before free.

pthread_keys_max = 0;

Why default to 0 on error? I would default to the current default.

long pthread_keys_max = 0;

Why initialize it, why long?

err:

Traditionally we call the bail label out, err is confusing because
it is also a function name.

Thanks,

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Post by Emmanuel Dreyfus

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch

This will just overflow the pthread structure? Besides, use strtoi,
calloc etc.

Here is an updated version using strtoll, calloc and taking care
http://ftp.espci.fr/shadow/manu/pthread_keys_max3.patch

Arrays of size 0 are invalid in C. You likely mean [].

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Hi

Nobody commented on this patch. Is it fine to commit?

Post by Emmanuel Dreyfus

Post by Joerg Sonnenberger

Post by Emmanuel Dreyfus
http://ftp.espci.fr/shadow/manu/pthread_keys_max.patch

This will just overflow the pthread structure? Besides, use strtoi,
calloc etc.

Here is an updated version using strtoll, calloc and taking care
http://ftp.espci.fr/shadow/manu/pthread_keys_max3.patch
--
Emmanuel Dreyfus

[Christos Zoulas]

On May 28, 9:59am, ***@netbsd.org (Emmanuel Dreyfus) wrote:
-- Subject: Re: Increase PTHREAD_KEYS_MAX (round 2)

| Hi
|
| Nobody commented on this patch. Is it fine to commit?

Both joerg and I commented on it...
Here's some more in addition to the previous ones:

1. in err use EXIT_FAILURE instead of 1.
2. the declaration char* pkm -> char *pkm

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Emmanuel Dreyfus]

Post by Christos Zoulas
Both joerg and I commented on it...

Oh sorry, I missed the replies.

Here is latest iterration that address your comments.
http://ftp.espci.fr/shadow/manu/pthread_keys_max4.patch

In pthread_tsd_init I do not reset pthread_keys_max to
a default value since it would not match allocated arrays.
We could call err() immediatly instead of retunring, though.

[Christos Zoulas]

On May 28, 12:49pm, ***@netbsd.org (Emmanuel Dreyfus) wrote:
-- Subject: Re: Increase PTHREAD_KEYS_MAX (round 3)

| On Thu, May 28, 2015 at 07:43:34AM -0400, Christos Zoulas wrote:
| > Both joerg and I commented on it...
|
| Oh sorry, I missed the replies.

NP :-)

| Here is latest iterration that address your comments.
| http://ftp.espci.fr/shadow/manu/pthread_keys_max4.patch
|
| In pthread_tsd_init I do not reset pthread_keys_max to
| a default value since it would not match allocated arrays.
| We could call err() immediatly instead of retunring, though.

This looks fine to me. I'd keep it as it is.
Don't you get a warning for the strtol() assignment, or that's only lint?

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de