This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] New functions pthread_attr_[sg]et_default_np for defaultthread attributes
On Thu, Mar 14, 2013 at 08:49:05AM -0700, Roland McGrath wrote:
> > 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.
It should be safe to use whatever we get from
pthread_attr_get_default_np. It's currently used directly in
pthread_create except for a couple of cases (that I've fixed in this
patch and the earlier move of default stack size).
> > --- 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?
I think not. The stack size initialization may have been [SHARED]
only earlier, but it clearly doesn't seem that way now. I'll remove
the initializers from the definition and check if it regresses any
tests.
> 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.
OK.
> > + /* 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).
OK, I'll check them all again for this.
> > --- 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.
If I set PTHREAD_EXPLICIT_SCHED in the default attributes, it gets
ignored because attr == NULL implies that the default attribute is in
force.
> 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.
I figured they would be expensive because I thought I would have to
synchronize access among all users of default_attr, which includes
pthread_create. But I guess it would be expensive only for those who
are nutty enough to keep changing the default while they're spawning
threads and they probably ought to be punished anyway. I'll add the
locks.
Siddhesh