Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Jakub Jelinek]

On Thu, Sep 03, 2015 at 01:36:35PM +0200, Sebastian Huber wrote:
> On 03/09/15 13:10, Jakub Jelinek wrote:
> >On Thu, Sep 03, 2015 at 01:09:23PM +0200, Sebastian Huber wrote:
> >We have only thread attributes in this function: mutable_attr and attr. 
> >The
> >attr is initialized with _thread_attr and gomp_thread_attr is 
> >supposed
> >to be read-only by this function. Under certain conditions we have to 
> >modify
> >the initial attributes. Since gomp_thread_attr is read-only, we have to 
> >copy
> >it and then modify the copy. For this we need some storage: mutable_attr.
> >>>So use local_thread_attr if you want to stress it, but IMHO thread_attr
> >>>just just fine.  I really don't like mutable_attr.
> >>Ok, if I don't rename thread_attr, is the patch ok?
> >Yes.
> 
> Thanks a lot for your kind review.
> 
> I committed the patches as:
> 
> https://gcc.gnu.org/viewcvs/gcc?view=revision=227439
> https://gcc.gnu.org/viewcvs/gcc?view=revision=227440
> https://gcc.gnu.org/viewcvs/gcc?view=revision=227441
> https://gcc.gnu.org/viewcvs/gcc?view=revision=227442

Unfortunately it broke stuff, here is a fix I've committed:

2015-09-03  Jakub Jelinek  

* configure.tgt: Add missing ;; in between nvptx and rtems
snippets.

--- libgomp/configure.tgt   (revision 227456)
+++ libgomp/configure.tgt   (working copy)
@@ -153,6 +153,7 @@ case "${target}" in
 
   nvptx*-*-*)
config_path="nvptx"
+   ;;
 
   *-*-rtems*)
# Use self-contained synchronization objects if provided by Newlib


Jakub

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Sebastian Huber]

On 03/09/15 12:19, Jakub Jelinek wrote:

@@ -292,7 +292,7 @@ gomp_team_start (void (*fn) (void *), void *data, unsigned 
nthreads,
>bool nested;
>struct gomp_thread_pool *pool;
>unsigned i, n, old_threads_used = 0;
>-  pthread_attr_t thread_attr, *attr;
>+  pthread_attr_t mutable_attr, *attr;

Just wonder why have you renamed this variable.  It is a thread attribute
after all, even after your changes.  mutable_attr doesn't make much sense to
me.


We have only thread attributes in this function: mutable_attr and attr. 
The attr is initialized with _thread_attr and gomp_thread_attr is 
supposed to be read-only by this function. Under certain conditions we 
have to modify the initial attributes. Since gomp_thread_attr is 
read-only, we have to copy it and then modify the copy. For this we need 
some storage: mutable_attr.


--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Jakub Jelinek]

On Tue, Jul 28, 2015 at 01:04:59PM +0200, Sebastian Huber wrote:
> libgomp/ChangeLog
> 2015-07-28  Sebastian Huber  
> 
>   * config/posix/pool.h (gomp_adjust_thread_attr): New.
>   * config/rtems/pool.h (gomp_adjust_thread_attr): Likewise.
>   (gomp_thread_pool_reservoir): Add priority member.
>   * confi/rtems/proc.c (allocate_thread_pool_reservoir): Add
>   priority.
>   (parse_thread_pools): Likewise.
>   * team.c (gomp_team_start): Rename thread_attr to mutable_attr.
>   Call configuration provided gomp_adjust_thread_attr(). Destroy
>   mutable attributes if necessary.
>   * libgomp.texi: Document GOMP_RTEMS_THREAD_POOLS.

Wonder if the RTEMS specific bits in libgomp.texi shouldn't be guarded
with
@ifset RTEMS
...
@end ifset
and --texinfo='@set RTEMS' be used when compiling it.
But perhaps it can be done incrementally, so the patch is ok.

> @@ -292,7 +292,7 @@ gomp_team_start (void (*fn) (void *), void *data, 
> unsigned nthreads,
>bool nested;
>struct gomp_thread_pool *pool;
>unsigned i, n, old_threads_used = 0;
> -  pthread_attr_t thread_attr, *attr;
> +  pthread_attr_t mutable_attr, *attr;

Just wonder why have you renamed this variable.  It is a thread attribute
after all, even after your changes.  mutable_attr doesn't make much sense to
me.

Jakub

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Jakub Jelinek]

On Thu, Sep 03, 2015 at 12:57:53PM +0200, Sebastian Huber wrote:
> On 03/09/15 12:19, Jakub Jelinek wrote:
> >>@@ -292,7 +292,7 @@ gomp_team_start (void (*fn) (void *), void *data, 
> >>unsigned nthreads,
> >>>bool nested;
> >>>struct gomp_thread_pool *pool;
> >>>unsigned i, n, old_threads_used = 0;
> >>>-  pthread_attr_t thread_attr, *attr;
> >>>+  pthread_attr_t mutable_attr, *attr;
> >Just wonder why have you renamed this variable.  It is a thread attribute
> >after all, even after your changes.  mutable_attr doesn't make much sense to
> >me.
> 
> We have only thread attributes in this function: mutable_attr and attr. The
> attr is initialized with _thread_attr and gomp_thread_attr is supposed
> to be read-only by this function. Under certain conditions we have to modify
> the initial attributes. Since gomp_thread_attr is read-only, we have to copy
> it and then modify the copy. For this we need some storage: mutable_attr.

So use local_thread_attr if you want to stress it, but IMHO thread_attr
just just fine.  I really don't like mutable_attr.

Jakub

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Jakub Jelinek]

On Thu, Sep 03, 2015 at 01:09:23PM +0200, Sebastian Huber wrote:
> >>>We have only thread attributes in this function: mutable_attr and attr. The
> >>>attr is initialized with _thread_attr and gomp_thread_attr is supposed
> >>>to be read-only by this function. Under certain conditions we have to 
> >>>modify
> >>>the initial attributes. Since gomp_thread_attr is read-only, we have to 
> >>>copy
> >>>it and then modify the copy. For this we need some storage: mutable_attr.
> >So use local_thread_attr if you want to stress it, but IMHO thread_attr
> >just just fine.  I really don't like mutable_attr.
> 
> Ok, if I don't rename thread_attr, is the patch ok?

Yes.

Jakub

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Sebastian Huber]

On 03/09/15 13:05, Jakub Jelinek wrote:

On Thu, Sep 03, 2015 at 12:57:53PM +0200, Sebastian Huber wrote:

>On 03/09/15 12:19, Jakub Jelinek wrote:

> >>@@ -292,7 +292,7 @@ gomp_team_start (void (*fn) (void *), void *data, 
unsigned nthreads,

> >>>bool nested;
> >>>struct gomp_thread_pool *pool;
> >>>unsigned i, n, old_threads_used = 0;
> >>>-  pthread_attr_t thread_attr, *attr;
> >>>+  pthread_attr_t mutable_attr, *attr;

> >Just wonder why have you renamed this variable.  It is a thread attribute
> >after all, even after your changes.  mutable_attr doesn't make much sense to
> >me.

>
>We have only thread attributes in this function: mutable_attr and attr. The
>attr is initialized with _thread_attr and gomp_thread_attr is supposed
>to be read-only by this function. Under certain conditions we have to modify
>the initial attributes. Since gomp_thread_attr is read-only, we have to copy
>it and then modify the copy. For this we need some storage: mutable_attr.

So use local_thread_attr if you want to stress it, but IMHO thread_attr
just just fine.  I really don't like mutable_attr.


Ok, if I don't rename thread_attr, is the patch ok?

--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.

Re: [PATCH 3/3] [gomp] Add thread attribute customization

[Sebastian Huber]

On 03/09/15 13:10, Jakub Jelinek wrote:

On Thu, Sep 03, 2015 at 01:09:23PM +0200, Sebastian Huber wrote:

We have only thread attributes in this function: mutable_attr and attr. The
attr is initialized with _thread_attr and gomp_thread_attr is supposed
to be read-only by this function. Under certain conditions we have to modify
the initial attributes. Since gomp_thread_attr is read-only, we have to copy
it and then modify the copy. For this we need some storage: mutable_attr.

So use local_thread_attr if you want to stress it, but IMHO thread_attr
just just fine.  I really don't like mutable_attr.

Ok, if I don't rename thread_attr, is the patch ok?

Yes.


Thanks a lot for your kind review.

I committed the patches as:

https://gcc.gnu.org/viewcvs/gcc?view=revision=227439
https://gcc.gnu.org/viewcvs/gcc?view=revision=227440
https://gcc.gnu.org/viewcvs/gcc?view=revision=227441
https://gcc.gnu.org/viewcvs/gcc?view=revision=227442

--
Sebastian Huber, embedded brains GmbH

Address : Dornierstr. 4, D-82178 Puchheim, Germany
Phone   : +49 89 189 47 41-16
Fax : +49 89 189 47 41-09
E-Mail  : sebastian.hu...@embedded-brains.de
PGP : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.