Skip to content

Conversation

@hjelmn
Copy link
Member

@hjelmn hjelmn commented May 18, 2016

This commit changes the behavior of bml/r2 from conditionally
registering btl progress functions to always registering progress
functions. Any progress function beloning to a btl that is not yet in
use is registered as low-priority. As soon as a proc is added that
will make use of the btl is is re-registered normally.

This works around an issue with some btls. In order to progress a
first message from an unknown peer both ugni and openib need to have
their progress functions called. If either btl is not in use after the
first call to add_procs the callback was never happening. This commit
ensures the btl progress function is called at some point but the
number of progress callbacks is reduced from normal to ensure lower
overhead when a btl is not used. The current ratio is 1 low priority
progress callback for every 8 calls to opal_progress().

Fixes #1676

Signed-off-by: Nathan Hjelm [email protected]

This commit changes the behavior of bml/r2 from conditionally
registering btl progress functions to always registering progress
functions. Any progress function beloning to a btl that is not yet in
use is registered as low-priority. As soon as a proc is added that
will make use of the btl is is re-registered normally.

This works around an issue with some btls. In order to progress a
first message from an unknown peer both ugni and openib need to have
their progress functions called. If either btl is not in use after the
first call to add_procs the callback was never happening. This commit
ensures the btl progress function is called at some point but the
number of progress callbacks is reduced from normal to ensure lower
overhead when a btl is not used. The current ratio is 1 low priority
progress callback for every 8 calls to opal_progress().

Fixes open-mpi#1676

Signed-off-by: Nathan Hjelm <[email protected]>
@hjelmn
Copy link
Member Author

hjelmn commented May 18, 2016

@larrystevenwise Please test.

@jsquyres
Copy link
Member

@bosilca Please also review -- this touches opal_progress().

@jsquyres
Copy link
Member

@bharatpotnuri Please test (this is a possible solution for #1664).

@hjelmn
Copy link
Member Author

hjelmn commented May 19, 2016

Huh, I referenced the wrong bug #. Will rebase it with the correct one later.

@bharatpotnuri
Copy link
Contributor

@jsquyres commit a679cc072b9458957671886062c643f6a40b0351 bml/r2: always add btl progress function #1677 is working fine with iWARP and fixes issue #1664.
Below is the output of osu_bw test without stall.

/usr/mpi/gcc/openmpi-2.0.0/bin/mpirun -np 2 --host peer1,peer2 --allow-run-as-root --mca btl openib,sm,self /usr/mpi/gcc/openmpi-2.0.0/tests/OSU/osu_bw

# OSU MPI Bandwidth Test v5.0
# Size      Bandwidth (MB/s)
1                       0.52
2                       1.04
4                       2.07
8                       3.95
16                      7.74
32                     16.12
64                     31.21
128                    60.22
256                   118.03
512                   222.06
1024                  403.43
2048                  678.97
4096                  961.80
8192                 1260.08
16384                1542.48
32768                2268.59
65536                2836.43
131072               3181.33
262144               3307.34
524288               3349.31
1048576              3363.01
2097152              3368.89
4194304              3370.98

Thanks @hjelmn .

@jsquyres
Copy link
Member

@bosilca please review. Thanks.

@ibm-ompi
Copy link

Test passed.

@hjelmn
Copy link
Member Author

hjelmn commented May 21, 2016

Needed for 2.0.0. Merging and opening 2.0.0 PR.

@hjelmn hjelmn merged commit 6195ec0 into open-mpi:master May 21, 2016
@bosilca
Copy link
Member

bosilca commented May 21, 2016

Have someone evaluated the impact of this change on the performance ? We should stop adding atomic operations and polluting the cache in our critical point.

@hjelmn
Copy link
Member Author

hjelmn commented May 21, 2016

Ran some simple tests and it looked ok. I can do it without the atomics if that is preferred. Don't really care if the low-priority calls are made exactly 1/8 times.

@hjelmn
Copy link
Member Author

hjelmn commented May 21, 2016

@bosilca Your request changes will make it so only one thread is ever in opal_progress() correct? If that is the case we should plan to remove all atomics from opal_progress().

@bosilca
Copy link
Member

bosilca commented May 21, 2016

The current version ensure that only one thread is active in opal_progress. However, we might want to be more liberal with the resources, allowing multiple threads to concurrently drain the networks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants