This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3.1] New functions pthread_[sg]etattr_default_np for default thread attributes
- From: Roland McGrath <roland at hack dot frob dot com>
- To: Siddhesh Poyarekar <siddhesh at redhat dot com>
- Cc: libc-alpha at sourceware dot org
- Date: Wed, 12 Jun 2013 16:17:57 -0700 (PDT)
- Subject: Re: [PATCH v3.1] New functions pthread_[sg]etattr_default_np for default thread attributes
- References: <20130314122003 dot GY22471 at spoyarek dot pnq dot redhat dot com> <20130314154905 dot ADDDD2C09C at topped-with-meat dot com> <20130315043154 dot GG22471 at spoyarek dot pnq dot redhat dot com> <20130318222239 dot 7A2712C084 at topped-with-meat dot com> <CAAHN_R13bRF0UY_XZ7Rj6tSeSgq8c_0j4bbEH6m9BbGD32EycQ at mail dot gmail dot com> <20130528220730 dot 33C262C06F at topped-with-meat dot com> <20130529065138 dot GF2145 at spoyarek dot pnq dot redhat dot com> <20130529224222 dot 8A87F2C07E at topped-with-meat dot com> <20130606131212 dot GZ13968 at spoyarek dot pnq dot redhat dot com> <20130612000601 dot 54C9F2C06E at topped-with-meat dot com> <20130612101128 dot GB19582 at spoyarek dot pnq dot redhat dot com>
> ChangeLog.tile
>
> * sysdeps/unix/sysv/linux/tile/tilegx/tilegx32/nptl/libpthread.abilist:
> Update.
> pthread_setattr_default_np.
Stray line.
> diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
> index b78bd95..efd4f90 100644
> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -448,19 +448,39 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg)
> {
> STACK_VARIABLES;
>
> - const struct pthread_attr *iattr = (struct pthread_attr *) attr;
> + struct pthread_attr *iattr = (struct pthread_attr *) attr;
> if (iattr == NULL)
> - /* Is this the best idea? On NUMA machines this could mean
> - accessing far-away memory. */
> - iattr = &__default_pthread_attr;
> + {
> + iattr = alloca (sizeof (*iattr));
An alloca of a fixed size that you do at most once is always worse than
just using a normal local. i.e., put 'struct pthread_attr default_attr;'
in function scope, and just:
iattr = &default_attr;
here.
> + lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> + *iattr = __default_pthread_attr;
Seems less strange to make that:
default_attr = __default_pthread_attr;
and put the "iattr = &default_attr;" line at the end of the if's then block
(it does not need to be inside the lock).
> + size_t cpusetsize = iattr->cpusetsize;
> + if (cpusetsize > 0)
> + {
> + cpu_set_t *cpuset = malloc (cpusetsize);
I'd just use __alloca here. But to be paranoid, do the __libc_use_alloca
dance. In probably all real situations, malloc will never actually be
required.
> +int
> +pthread_setattr_default_np (const pthread_attr_t *in)
> +{
> + struct pthread_attr *real_in, attrs;
REAL_IN should be a const pointer.
> + /* Catch invalid values. */
> + int policy = real_in->schedpolicy;
> +
> + ret = check_sched_policy_attr (policy);
Drop that blank line.
> + /* stacksize == 0 is fine. It means that we don't change the current
> + value. */
> + if (real_in->stacksize != 0)
> + {
> + ret = check_stacksize_attr (real_in->stacksize);
> + if (ret)
> + return ret;
> + }
> + else
> + real_in->stacksize = __default_pthread_attr.stacksize;
Is it kosher to fetch without the lock?
It's definitely not kosher to touch *REAL_IN here!
> + attrs = *real_in;
> +
> + /* Mantain stacksize as a non-zero value. This is a computed fallback that
> + we get on library initialization, so we don't want to overwrite it unless
> + there is a valid value to replace it. */
> + if (real_in->stacksize > 0)
> + attrs.stacksize = real_in->stacksize;
I don't really follow the rationale for this.
I think all you want is:
if (attrs.stacksize == 0)
/* Not changing the default, so preserve the old value. */
attrs.stacksize = __default_pthread_attr.stacksize;
inside the locked region.
> + if (attrs.cpuset == NULL || attrs.cpusetsize == 0)
> + free (__default_pthread_attr.cpuset);
Don't leave the stale pointer even if we won't use it. Reset it to NULL.
Since you have this test anyway, might as well move the memcpy into its
else branch so you never bother with a zero-length memcpy.
> + void *newp = (cpu_set_t *) realloc (__default_pthread_attr.cpuset,
> + attrs.cpusetsize);
Drop the cast and make the variable cpu_set_t *.
You could skip the realloc if the sizes match, which will be the common
case (the second most common, after zero).
> +static void *
> +thr (void *unused)
Use __attribute__ ((unused)) on the parameter.
> + /* To verify that the pthread_setattr_default_np worked. */
> + if ((ret = pthread_getattr_default_np (&attr)) != 0)
> + {
> + printf ("pthread_getattr_default_np failed: %s\n", strerror (ret));
> + goto out;
> + }
This doesn't really need to be done in the thread function. It could be
checked just right after the pthread_setattr_default_np call. But I
suppose it doesn't hurt to do it this way.
> + if ((ret = verify_result (&attr)) != 0)
> + goto out;
Use (*verify_result) (&attr) so it's clear that it's a function pointer.
> + pthread_mutex_lock (&m);
> + running++;
> + pthread_mutex_unlock (&m);
Tabify the middle line.
> + puts ("Thread failed\n");
puts already adds the \n.
> + /* Stay in sync for detached threads and get their status. */
> + while (!do_join)
> + {
> + pthread_mutex_lock (&m);
> + if (running == 0)
> + {
> + pthread_mutex_unlock (&m);
Tabify the { line.
> + printf ("pthread_attr_set_default failed to set detach state\n");
[...]
> + printf ("pthread_attr_set_default failed to set cpu affinity\n");
[...]
> + printf ("pthread_attr_set_default failed to set EXPLICIT_SCHED\n");
[...]
> + printf ("pthread_attr_set_default failed to set SCHED_RR\n");
[...]
> + printf ("pthread_attr_set_default failed to set sched_priority\n");
Use puts. Or for some, it could print the actual value it got, e.g.:
printf ("pthread_attr_set_default failed to set SCHED_RR (%d != %d)\n",
policy, SCHED_RR);
> + if (ret == EPERM)
> + {
> + fprintf (stderr, "Skipping CPU Affinity test: %s\n", strerror (ret));
[...]
> + fprintf (stderr, "Skipping Scheduler Attributes test: %s\n",
Should probably be on stdout like the rest of the output.
> + printf ("pthread_attr_set_default failed for guardsize (%zu, %zu)\n",
In this and all the messages, either use the actual name of the function,
or don't use a function name at all.
> + long pagesize = sysconf (_SC_PAGESIZE);
(repeated in two places)
'long int', never just 'long'.
It would have been easier to review the test if it had some comments
describing its design, or perhaps just if the functions were in a different
order. But after I'd read it through twice, it was clear enough so I won't
give you a real hard time about the comments.
Thanks,
Roland