Re: [PATCH] New functions pthread_attr_[sg]et_default_np for defaultthread attributes

[Roland McGrath <roland at hack dot frob dot com>]

> pthread_attr_init (&attr);
> pthread_attr_setstacksize (&attr, 1024 * 1024);
> pthread_attr_set_default_np (&attr);

The basic API seems reasonable.  I wonder what the difference is or should
be between the above and:

	pthread_attr_get_default_np (&attr);
	... pthread_attr_set* calls ...
	pthread_attr_set_default_np (&attr);

e.g., after pthread_attr_get_default_np do you have a pthread_attr_t that
looks like it's fresh from pthread_attr_init, or do you have one from which
you can use pthread_attr_get* to query the defaults?  I can see reasons for
both ways.  We should give it some thought.

> --- a/nptl/nptl-init.c
> +++ b/nptl/nptl-init.c
> @@ -424,6 +424,7 @@ __pthread_initialize_minimal_internal (void)
>    /* Round the resource limit up to page size.  */
>    limit.rlim_cur = (limit.rlim_cur + pagesz - 1) & -pagesz;
>    default_attr.stacksize = limit.rlim_cur;
> +  default_attr.guardsize = __getpagesize ();

This looks like every nonzero field (i.e. just those two) in
__pthread_default_attr is initialized explicitly at runtime.
So the variable might as well be uninitialized bss (and thus
take up no space in libpthread.so on disk) beforehand.  That
would already seem to be so for __default_stacksize, but we
have an initializer for that under [!SHARED] and I don't see
why off hand.  Am I missing something?

> +  /* Catch invalid values.  */
> +  int policy = real_in->schedpolicy;
> +
> +  if (policy != SCHED_OTHER && policy != SCHED_FIFO && policy != SCHED_RR)
> +    return EINVAL;
> +
> +  struct sched_param *param = &real_in->schedparam;
> +
> +  if (param->sched_priority > 0)
> +    {
> +      int min = sched_get_priority_min (policy);
> +      int max = sched_get_priority_max (policy);
> +
> +      if (min == -1 || max == -1
> +	  || param->sched_priority > max || param->sched_priority < min)
> +	return EINVAL;
> +    }
> +
> +  /* stacksize == 0 is fine.  It means that we don't change the current
> +     value.  */
> +  if (real_in->stacksize != 0 && real_in->stacksize < PTHREAD_STACK_MIN)
> +    return EINVAL;

This repeats logic that exists elsewhere for each specific attribute,
either in pthread_attr_set* or in pthread_create.  We really ought to
have those consolidated into one piece of code for each attribute.

> +  /* Everything is fine, so the values.  */

Second clause no verb.

> +  default_attr.schedparam = real_in->schedparam;
> +  default_attr.schedpolicy = real_in->schedpolicy;
> +  default_attr.flags = real_in->flags;
> +  default_attr.guardsize = real_in->guardsize;
> +  default_attr.stackaddr = real_in->stackaddr;
> +  default_attr.cpuset = real_in->cpuset;
> +  default_attr.cpusetsize = real_in->cpusetsize;

Since there is only one field that has something other than copying,
perhaps it would be better to do this with struct assignment rather than
listing out each field in the source like this.  I think the ease of
maintenance matters more than microoptimization of this particular
interface, so something like:

	attrs = *real_in;
	if (attrs.stacksize == 0)
	  attrs.stacksize = default_attr.stacksize;
	default_attr = attrs;

would be fine.

OTOH, if we do something more generally formulaic about validating the
values when setting them then perhaps there is a natural way to roll that
in with the special handling of stacksize.

The stackaddr attribute never makes sense as a default, since you obviously
don't want multiple threads using the same value.  So it should be EINVAL
to call pthread_attr_set_default_np with a pthread_attr_t on which
pthread_attr_setstackaddr has been called.  I don't think there are other
attributes in this category, but I'm not positive off hand (and I don't
have all the source in front of me right now).

> --- a/nptl/pthread_create.c
> +++ b/nptl/pthread_create.c
> @@ -507,8 +507,7 @@ __pthread_create_2_1 (newthread, attr, start_routine, arg)
>  #endif
>  
>    /* Determine scheduling parameters for the thread.  */
> -  if (attr != NULL
> -      && __builtin_expect ((iattr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0, 0)
> +  if (__builtin_expect ((iattr->flags & ATTR_FLAG_NOTINHERITSCHED) != 0, 0)

Explain this change.

I haven't looked at the test case at all, will do in the next iteration.

Finally, it's implicit here that the new calls are not thread-safe.  If
that is to be the interface, it should be explicit in the pthread.h
comments.  It seems like it would be nutty to call these anywhere but at
startup time when there is still only one thread.  But since it is cheap
and easy enough just to make these calls thread-safe with an internal lock,
might as well.


Thanks,
Roland