oshmem: get rid of oshmem_proc_t #2026
Conversation
|
@jladd-mlnx can you please review this PR (or have it reviewed) ? @jsquyres FYI |
|
👍 |
oshmem/proc/proc.c
Outdated
| OBJ_CONSTRUCT(&oshmem_proc_lock, opal_mutex_t); | ||
|
|
||
| assert(sizeof(ompi_proc_t) >= sizeof(oshmem_proc_t)); | ||
| assert(sizeof(ompi_proc_t) >= sizeof(ompi_proc_t)); |
There was a problem hiding this comment.
logical tautology? Instead you might want to check that you have enough room in the padding for what you want to store inside.
There was a problem hiding this comment.
thanks @bosilca
I ran sed, and tested it without thoroughly checking the new code ...
will review it all tomorrow
|
👍 👍 👍 |
ggouaillardet
force-pushed
the
topic/oshmem_proc_t
branch
2 times, most recently
from
e629994 to
dac5c0a
Compare
|
i made the requested changes, can you please review them ? |
oshmem/proc/proc.h
Outdated
| int is_member; /* true if my_pe is part of the group, participate in collectives */ | ||
| struct oshmem_proc_t **proc_array; /**< list of pointers to ompi_proc_t structures | ||
| struct ompi_proc_t **proc_array; /**< list of pointers to ompi_proc_t structures | ||
| for each process in the group */ |
There was a problem hiding this comment.
i do not get it.
a pointer (of pointer of some struct) was replaced by a pointer (of pointer of some struct) e.g.
sizeof(struct oshmem_proc_t **) == sizeof (struct ompi_proc_t **)
so how can this break alignment ?
or are you saying that the padding array of an ompi_proc_t might not be aligned, and hence the oshmem_proc_t will not be aligned too ?
i configured with and without --enable-debug, and i could not see any difference.
shall i simply declare the padding of ompi_proc_t as
void *padding[OMPI_PROC_PADDING_SIZE/sizeof(void *);
instead of
char padding[OMPI_PROC_PADDING_SIZE;
in order to guarantee it will be aligned ?
There was a problem hiding this comment.
I mean coding style issue (could you align proc_array field with others). Sorry for confusion.
There was a problem hiding this comment.
I see now, will do
(one more drawback of running sed blindly ...)
|
I agree with @igor-ivanov's points. Once all these minor things have been cleaned up, 👍 |
ggouaillardet
force-pushed
the
topic/oshmem_proc_t
branch
from
dac5c0a to
a216cf3
Compare
|
i made the requested changes. note i also added an an other option is to redefine
yet an other option is to do nothing for now and start enforcing that if/when problem start occuring on alignment sensitive arch (such as sparc) thoughts anyone ? |
|
|
||
| #define OSHMEM_PE_INVALID (-1) | ||
|
|
||
| struct oshmem_proc_data_t { |
There was a problem hiding this comment.
@ggouaillardet could you add comment for this struct that says that this struct should meet padding size in ompi_proc_t as I asked. Thanks.
There was a problem hiding this comment.
@igor-ivanov i did the change but i lost it before committing.
here is the comment
/* This struct will be copied into the padding field of an ompi_proc_t
* so the size of oshmem_proc_data_t must be less or equal than
* OMPI_PROC_PADDING_SIZE */
struct oshmem_proc_data_t {
char * transport_ids;
int num_transports;
};
ggouaillardet
force-pushed
the
topic/oshmem_proc_t
branch
from
a216cf3 to
d4231d5
Compare
|
Build Failed with GNU compiler! Please review the log, and get in touch if you have questions. Gist: https://gist.github.com/a3d6ae30de97955422bc165cf0f4bb5f |
|
Build Failed with XL compiler! Please review the log, and get in touch if you have questions. Gist: https://gist.github.com/2147be152bccf7ff487db01432eadc16 |
|
👍 |
|
bot:ibm:retest |
|
Build Failed with GNU compiler! Please review the log, and get in touch if you have questions. Gist: https://gist.github.com/7fbfa41567971d4e0611b85f94d104a0 |
|
👍 |
|
IBM CI system seems to have hit an intermittent Jenkins and system failure - it looks like it should be resolved now. |
store oshmem related per proc data in an oshmem_proc_data_t struct, that is stored in the padding section of an ompi_proc_t this data can be accessed via the OSHMEM_PROC_DATA(proc) macro Fixes open-mpi#2023
previously, the definition was
struct oshmem_proc_data_t {
int num_transports;
char * transport_ids;
};
so in 64 bits arch, the compiler would very likely insert a 4 bytes
padding before the two fields in order to have transport_ids aligned
ggouaillardet
force-pushed
the
topic/oshmem_proc_t
branch
from
d4231d5 to
184d53a
Compare
9 participants
and use these macros to access oshmem related per proc data :
Fixes #2023