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

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

> 	* sysdeps/unix/sysv/linux/i386/nptl/libpthread.abilist: Add
> 	GLIBC_2.18
> 	(GLIBC_2.18): Add pthread_getattr_default_np and
> 	pthread_setattr_default_np.

The .abilist files are (at least theoretically) mechanically generated.
A log entry of just "Update." or "Update for foo and bar additions."
is sufficient.

> 	* pthread_create.c (__pthread_create_2_1): Allow default flags
> 	to be used.

This is too vague.

> 	* Makefile (libpthread-routines): Add
> 	pthread_setattr_default_np and pthread_getattr_default_np.
> 	(tests): Add tst-default-attr.
> 	* Versions (libpthread): Add GLIBC_2.18.
> 	(GLIBC_2.18): Add pthread_setattr_default_np and
> 	pthread_getattr_default_np.
> 	* nptl-init.c (__pthread_initialize_minimal_internal):
> 	Initialize default guardsize.
> 	* pthread_getattr_default_np.c: New file.
> 	* pthread_setattr_default_np.c: New file.
> 	* sysdeps/pthread/pthread.h (pthread_getattr_default_np,
> 	pthread_setattr_default_np): Declare.

Should have [__USE_GNU] or whatever condition the decls are inside.

> 	* tst-default-attr.c: New test case.

It's usual practice to put the line for a new file before any other line
that refers to that new file.  So here, Makefile last.

> --- a/nptl/allocatestack.c
> +++ b/nptl/allocatestack.c

There is no log entry for this file.

> +  if (attr->stacksize)
> +    size = attr->stacksize;

No implicit Boolean coercion, please.

> +  else
> +    {
> +      lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> +      size = __default_pthread_attr.stacksize;
> +      lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
> +    }

The lock isn't really needed for a single word fetch that doesn't need to
be synchronized with anything else.

> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h

There is no log entry for this file.

> --- a/nptl/pthread_attr_getstacksize.c
> +++ b/nptl/pthread_attr_getstacksize.c
> @@ -32,7 +32,14 @@ __pthread_attr_getstacksize (attr, stacksize)
>  
>    /* If the user has not set a stack size we return what the system
>       will use as the default.  */
> -  *stacksize = iattr->stacksize ?: __default_pthread_attr.stacksize;
> +  if (iattr->stacksize)

No implicit Boolean coercion, please.

> +    *stacksize = iattr->stacksize;
> +  else
> +    {
> +      lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> +      *stacksize = __default_pthread_attr.stacksize;
> +      lll_unlock (__default_pthread_attr_lock, LLL_PRIVATE);
> +    }

In some places (that I'm aware of, mostly in Hurd code, but anyway), we use
the style of fetching into a local in the locked region and then storing to
the user-supplied pointer only after releasing the lock.  It doesn't really
matter since a user passing a bad pointer deserves what he gets anyway, but
crashing without holding a lock is a bit nicer and makes more kinds of
recovery possible.

> +    {
> +      /* Is this the best idea?  On NUMA machines this could mean
> +         accessing far-away memory.  */
> +      iattr = &__default_pthread_attr;
> +      lll_lock (__default_pthread_attr_lock, LLL_PRIVATE);
> +    }

Taking the lock around so much code is very scary.  Even if you have no
bugs leading to error paths that fail to unlock, it is unnecessary
serialization.  Except for the cpuset pointer, it would be trivial just to
make a local stack copy of the global while locked, and then use that.

> +  struct sched_param *param = &real_in->schedparam;
> +
> +  if (param->sched_priority > 0)

I'd drop this blank line.

> +      ret = check_sched_priority_attr (param->sched_priority, policy);
> +
> +      if (ret)
> +        return ret;

And this one.


I have to run now and I haven't reviewed the test case yet.


Thanks,
Roland