This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v3.1] New functions pthread_[sg]etattr_default_np for default thread attributes


> 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]