FileFIXME Note
src/mpi/coll/iscan.cjust use roots_comm->rank instead
src/mpi/coll/iallgather.csaving an MPI_Aint into an int
src/mpi/coll/ired_scat_block.cdoes this algorithm actually work correctly for noncommutative ops?
If so, relax restriction in assert and comments...
src/mpi/coll/ired_scat_block.cthis version only works for power of 2 procs
src/mpi/coll/red_scat_block.cshould we be checking the op_errno here?
src/mpi/coll/red_scat_block.cthis version only works for power of 2 procs
src/mpi/coll/ired_scat.cdoes this algorithm actually work correctly for noncommutative ops?
If so, relax restriction in assert and comments...
src/mpi/coll/ired_scat.cthis version only works for power of 2 procs
src/mpi/coll/iscatter.chow can this be right? shouldn't (sendcount*mask)
be the amount sent and curr_cnt be reduced by that? Or
is it always true the (curr_cnt/2==sendcount*mask)?
src/mpi/coll/helper_fns.cFor the brief-global and finer-grain control, we must ensure that
the global lock is *not* held when this routine is called. (unless we change
progress_start/end to grab the lock, in which case we must *still* make
sure that the lock is not held when this routine is called).
src/mpi/coll/helper_fns.cshould we cancel the pending (possibly completed) receive
request or wait for it to complete?
src/mpi/coll/igather.cwhy are there two copies here?
src/mpi/coll/allgather.csaving an MPI_Aint into an int
src/mpi/coll/allgather.csaving an MPI_Aint into an int
src/mpi/coll/bcast.cit would be nice if we could refactor things to minimize
duplication between this and MPIR_Scatter_intra and friends. We can't use
MPIR_Scatter_intra as is without inducing an extra copy in the noncontig case.
src/mpi/coll/bcast.cThis function uses some heuristsics based off of some testing on a
cluster at Argonne. We need a better system for detrmining and controlling
the cutoff points for these algorithms. If I've done this right, you should
be able to make changes along these lines almost exclusively in this function
and some new functions. [goodell@ 2008/01/07]
src/mpi/coll/bcast.cbinomial may not be the best algorithm for on-node
bcast. We need a more comprehensive system for selecting the
right algorithms here.
src/mpi/coll/bcast.cbinomial may not be the best algorithm for on-node
bcast. We need a more comprehensive system for selecting the
right algorithms here.
src/mpi/coll/bcast.cIt would be good to have an SMP-aware version of this
algorithm that (at least approximately) minimized internode
communication.
src/mpi/coll/red_scat.cthis version only works for power of 2 procs
src/mpi/coll/reduce.cdoes this need to be checked after each uop invocation for
predefined operators?
src/mpi/coll/alltoall.cThis converts the Alltoall to a set of blocking phases.
Two alternatives should be considered:
1) the choice of communication pattern could try to avoid
contending routes in each phase
2) rather than wait for all communication to finish (waitall),
we could maintain constant queue size by using waitsome
and posting new isend/irecv as others complete. This avoids
synchronization delays at the end of each block (when
there are only a few isend/irecvs left)
src/mpi/info/info_dup.cmultithreaded
src/mpi/pt2pt/sendrecv_rep.cshould we cancel the pending (possibly completed) receive request or wait for it to complete?
src/mpi/pt2pt/bsend.cCheck for MPID_WOULD_BLOCK?
src/mpi/pt2pt/iprobe.cIs this correct for intercomms?
src/mpi/pt2pt/mpir_request.cWhat is this routine for?

[BRT] it is used by testall, although looking at testall now, I think the
algorithm can be change slightly and eliminate the need for this routine
src/mpi/pt2pt/sendrecv.cPerformance for small messages might be better if MPID_Send() were used here instead of MPID_Isend()
src/mpi/pt2pt/sendrecv.cshould we cancel the pending (possibly completed) receive request or wait for it to complete?
src/mpi/pt2pt/waitall.care Ibsend requests added to the send queue?
src/mpi/pt2pt/bsendutil.cProcess pending bsend requests in detach
src/mpi/pt2pt/bsendutil.cUnimplemented retry of pending bsend operations
src/mpi/pt2pt/ibsend.cThere should be no unreferenced args!
src/mpi/pt2pt/ibsend.cuse the memory management macros
src/mpi/pt2pt/ibsend.cshould we be setting the request at all in the case of an error?
src/mpi/pt2pt/greq_start.cMT this function is not thread safe when using fine-grained threading
src/mpi/pt2pt/cancel.cwhy do we directly access the partner request's
cc field? shouldn't our cc_ptr be set to the address
of the partner req's cc field?
src/mpi/spawn/lookup_name.cchange **fail to something more meaningful
src/mpi/spawn/lookup_name.cchange **fail to something more meaningful
src/mpi/spawn/open_port.cIf info_ptr is non-null, we should validate it
src/mpi/spawn/comm_disconnect.cMT should we be checking this?
src/mpi/datatype/typeutil.cdoes the order of this list need to correspond to anything in
particular? There are several lists of predefined types sprinkled throughout
the codebase and it's unclear which (if any) of them must match exactly.
[goodell@ 2009-03-17]
src/mpi/datatype/typeutil.cThis is actually an error, since this routine
should only be called once
src/mpi/datatype/typeutil.cThis needs to be make thread safe
src/mpi/datatype/looputil.cIs this routine complete? Why is it needed? If it is needed, it
must have a comment that describes why it is needed and the arguments
must have ATTRIBUTE((unused))
src/mpi/datatype/type_create_resized.c???
src/mpi/datatype/register_datarep.cUNIMPLEMENTED
src/mpi/datatype/looputil.h!!! TODO!!! FOO!!! DO THIS!!! DETECT ME!!!

Consider using MPIU_INT64_T etc. types instead of the
EIGHT_BYTE_BASIC_TYPE stuff, or put #defines at the top of this file
assigning them in a simple manner.

Doing that might require that we create MPIU_UINT64_T types (etc),
because it looks like we really want to have unsigned types for the
various convert functions below.
src/mpi/datatype/looputil.hThis is necessary with some compilers or compiler settings
src/mpi/datatype/looputil.hWho defines __BYTE_ORDER or __BIG_ENDIAN? They aren't part of C
src/mpi/datatype/looputil.hThe else test assumes that the byte order is little endian, whereas
it may simply have been undetermined. This should instead use either
a configure-time test (for which there are macros) or a runtime test
and not use this non-portable check
src/mpi/datatype/looputil.h"BLENDIAN" is a non-conforming name - it could conflict with some
other definition in a non-mpich header file
src/mpi/datatype/type_match_size.cThis routine does not require the global critical section
src/mpi/datatype/dataloop/segment_count.cThe blkidx and index functions are not used since they
optimize the count by coalescing contiguous segments, while
functions using the count do not optimize in the same way
(e.g., flatten code)
src/mpi/topo/dist_gr_neighb_count.cWhy does this routine require a CS?
src/mpi/topo/cart_create.cnranks could be zero (?)
src/mpi/topo/topoutil.cthread safe code needs a thread lock here, followed
by another test on the keyval to see if a different thread
got there first
src/mpi/topo/topoutil.cfree the attribute data structure
src/mpi/topo/dist_gr_neighb.cWhy does this routine need a CS
src/mpi/topo/dims_create.cThe error checking should really be part of MPI_Dims_create,
not part of MPIR_Dims_create_impl. That slightly changes the
semantics of Dims_create provided by the device, but only by
removing the need to check for errors
src/mpi/topo/dist_gr_create.cWhy not - there is only one allocated at a time. Is it only
that there is no defined macro to pop and free an item?
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/topo/dist_gr_create.cbuf is never freed on error!
src/mpi/errhan/add_error_code.cverify that errorclass is a dynamic class
src/mpi/errhan/file_set_errhandler.ccheck for a valid file handle (fh) before converting to
a pointer
src/mpi/errhan/file_set_errhandler.cIs this obsolete now?
src/mpi/errhan/file_set_errhandler.cWe need an error return
src/mpi/errhan/file_set_errhandler.cWe need an error return
src/mpi/errhan/file_call_errhandler.ccheck for a valid file handle (fh) before converting to a
pointer
src/mpi/errhan/errutil.cNot internationalized. Since we are using MPIR_Err_get_string,
we are assuming that the code is still able to execute a full
MPICH error code to message conversion.
src/mpi/errhan/errutil.cNow that error codes are chained, this does not produce a valid
error code since there is no valid ring index corresponding to this code
src/mpi/errhan/errutil.cNo one uses the return value
src/mpi/errhan/errutil.cThe error stack comment only applies to MSG_ALL
src/mpi/errhan/errutil.cWe probably want to set this to MPI_ERR_UNKNOWN
and discard the rest of the bits
src/mpi/errhan/errutil.cThis routine isn't quite right yet
src/mpi/errhan/errutil.cnot internationalized
src/mpi/errhan/errutil.cWhy print the class message first? The instance
message is supposed to be complete by itself.
src/mpi/errhan/errutil.cReplace with function to add instance string or
error code string
src/mpi/errhan/errutil.cNot internationalized
src/mpi/errhan/errutil.cThis needs to be made consistent with the different thread levels,
since in the "global" thread level, an extra thread mutex is not required.
src/mpi/errhan/errutil.cERR_OTHER is overloaded; this may mean "OTHER" or it may
mean "No additional error, just routine stack info"
src/mpi/errhan/errutil.cShouldn't str be const char * ? - no, but you don't know that without
some documentation
src/mpi/errhan/errutil.cDon't use Snprint to append a string !
src/mpi/errhan/errutil.cThe following code is broken as described above (if the errcode
is not valid, then this code is just going to cause more problems)
src/mpi/errhan/errutil.c(Here and elsewhere) Make sure any string is
non-null before you use it
src/mpi/errhan/errutil.cNot internationalized
src/mpi/errhan/errutil.cThese functions are not thread safe
src/mpi/errhan/errutil.cdefault is not thread safe
src/mpi/errhan/errutil.cA check for MPI_IN_PLACE should only be used
where that is valid
src/mpi/errhan/errutil.cWe may want to use 0x%p for systems that
(including Windows) that don't prefix %p with 0x.
This must be done with a capability, not a test on
particular OS or header files
src/mpi/errhan/errutil.cHow do we determine that we failed to unwind the stack?
src/mpi/errhan/dynerrutil.cDoes this need a thread-safe init?
src/mpi/errhan/dynerrutil.cUnallocated error code?
src/mpi/errhan/dynerrutil.cUnallocated error code?
src/mpi/errhan/dynerrutil.cFor robustness, we should make sure that the associated string
is initialized to null
src/mpi/errhan/add_error_class.cwhy isn't this done in MPIR_Err_add_class?
src/mpi/errhan/file_get_errhandler.ccheck for a valid file handle (fh) before converting to a pointer
src/mpi/errhan/file_get_errhandler.cIs this obsolete now?
src/mpi/debugger/tvtest.cMove these into dbgstub.c
src/mpi/debugger/dbginit.cIn MPICH, the executables may not have the information
on the other processes; this is part of the Process Manager Interface
(PMI). We need another way to provide this information to
a debugger
src/mpi/debugger/dll_mpich.cinclude the mpichconf.h file?
src/mpi/debugger/dll_mpich.chandle size properly (declared as 64 in mpi_interface.h)
src/mpi/comm/contextid.cDo we need to do an GLOBAL yield here?
When we do a collective operation, we anyway yield
for other others
src/mpi/comm/contextid.cDo we need to do an GLOBAL yield here?
When we do a collective operation, we anyway yield
for other others
src/mpi/comm/contextid.cDo we need to do an GLOBAL yield here? When we
do a collective operation, we anyway yield for other
others
src/mpi/comm/contextid.cI'm not sure if there can be an overflows for this tag
src/mpi/comm/contextid.cwe need an internal tag or
communication channel. Can we use a different
context instead?. Or can we use the tag
provided in the intercomm routine? (not on a dup,
but in that case it can use the collective context)
src/mpi/comm/comm_dup_with_info.cWe just ignore the info argument for now and just call
Comm_dup
src/mpi/comm/comm_create.cBUBBLE SORT
src/mpi/comm/commutil.cReusing context ids can lead to a race condition if (as is desirable)
MPI_Comm_free does not include a barrier. Consider the following:
Process A frees the communicator.
Process A creates a new communicator, reusing the just released id
Process B sends a message to A on the old communicator.
Process A receives the message, and believes that it belongs to the
new communicator.
Process B then cancels the message, and frees the communicator.

The likelyhood of this happening can be reduced by introducing a gap
between when a context id is released and when it is reused. An alternative
is to use an explicit message (in the implementation of MPI_Comm_free)
to indicate that a communicator is being freed; this will often require
less communication than a barrier in MPI_Comm_free, and will ensure that
no messages are later sent to the same communicator (we may also want to
have a similar check when building fault-tolerant versions of MPI).
src/mpi/comm/commutil.cthis is an alternative constructor that doesn't use MPIR_Comm_create!
src/mpi/comm/commutil.cNo coll_fns functions for the collectives
src/mpi/comm/commutil.cNo local functions for the topology routines
src/mpi/comm/commutil.cMT what protects access to this structure and ic_default_collops?
src/mpi/comm/commutil.cdo we want to copy coll_fns here?
src/mpi/comm/commutil.cdo we want to copy coll_fns here?
src/mpi/comm/comm_split_type.cthis error code is wrong, it's the error code for
regular MPI_Comm_split
src/mpi/comm/comm_group.cAdd a sanity check that the size of the group is the same as
the size of the communicator. This helps catch corrupted
communicators
src/mpi/rma/winutil.cMove this into wincreate (having a separate file is unneccessary,
and it leads to a false "missing coverage" report)
src/mpi/init/initthread.cMove to os-dependent interface?
src/mpi/init/initthread.c(Windows) This severly degrades performance but fixes alignment
issues with the datatype code.
src/mpi/init/initthread.cCurrently this info object is empty, we need to add data to this
as defined by the standard.
src/mpi/init/initthread.cDefine these in the interface. Does Timer init belong here?
src/mpi/init/initthread.cThis is a hack to handle the common case of two worlds.
If the parent comm is not NULL, we always give the world number
as "1" (false).
src/mpi/init/initthread.cDoes this need to come before the call to MPID_InitComplete?
For some debugger support, MPII_Wait_for_debugger may want to use
MPI communication routines to collect information for the debugger
src/mpi/init/abort.c100 is arbitrary and may not be long enough
src/mpi/init/abort.cIt isn't clear that Abort should wait for a critical section,
since that could result in the Abort hanging if another routine is
hung holding the critical section. Also note the "not thread-safe"
comment in the description of MPI_Abort above.
src/mpi/init/abort.cThis is not internationalized
src/mpi/init/finalize.cWhy is this not one of the finalize callbacks?. Do we need
pre and post MPID_Finalize callbacks?
src/mpi/init/finalize.cShould this also be a finalize callback?
src/mpi/init/finalize.cMany of these debugging items could/should be callbacks,
added to the finalize callback list
src/mpi/init/finalize.cthe memory tracing code block should be a finalize callback
src/mpi/init/finalize.cWe'd like to arrange for the mem dump output to
go to separate files or to be sorted by rank (note that
the rank is at the head of the line)
src/mpi/init/async.cWe assume that waiting on some request forces progress
on all requests. With fine-grained threads, will this still
work as expected? We can imagine an approach where a request on
a non-conflicting communicator would not touch the remaining
requests to avoid locking issues. Once the fine-grained threads
code is fully functional, we need to revisit this and, if
appropriate, either change what we do in this thread, or delete
this comment.
src/mpi/attr/win_set_attr.ccommunicator of window?
src/mpi/attr/attrutil.cThis is not correct in some cases (size(MPI_Aint)>
sizeof(intptr_t))
src/mpi/attr/attrutil.cMissing routine description
src/mpi/attr/comm_set_attr.cThis code is incorrect in some cases, particularly
in the case where intptr_t is different from MPI_Aint,
since in that case, the Fortran 9x interface will provide
more bytes in the attribute_val than this allows. The
dual casts are a sign that this is faulty. This will
need to be fixed in the type/win set_attr routines as
well.
src/mpi/attr/comm_set_attr.cSee the comment above on this dual cast.
src/mpi/attr/comm_get_attr.cWe could initialize some of these here; only tag_ub is
used in the error checking.
src/mpi/attr/comm_get_attr.cThis code is broken. The MPIR_ATTR_INT is for Fortran
MPI_Fint types, not int, and MPIR_ATTR_AINT is for Fortran
INTEGER(KIND=MPI_ADDRESS_KIND), which is probably an MPI_Aint,
and intptr_t is for exactly the case where MPI_Aint is not
the same as intptr_t.
This code needs to be fixed in every place that it occurs
(i.e., see the win and type get_attr routines).
src/mpi/attr/comm_get_attr.cThere is no reason to have this routine since it unnecessarily
duplicates the MPII_Comm_get_attr interface.
src/mpi/attr/comm_get_attr.cAttributes must be visable from all languages
src/mpi/romio/adio/common/ad_iwrite_coll.cneed an implementation of ADIOI_IOIstridedColl
src/mpi/romio/adio/common/req_malloc.cInsert error here
src/mpi/romio/adio/common/lock.cThis is a temporary hack until we use flock64 where
available. It also doesn't fix the broken Solaris header sys/types.h
header file, which declars off_t as a UNION ! Configure tests to
see if the off64_t is a union if large file support is requested;
if so, it does not select large file support.
src/mpi/romio/adio/common/lock.cThis should use the error message system,
especially for MPICH
src/mpi/romio/adio/common/ad_iread_coll.cneed an implementation of ADIOI_IOIstridedColl
if (fd->hints->cb_pfr != ADIOI_HINT_DISABLE) {
ADIOI_IOIstridedColl(fd, buf, count, ADIOI_READ, datatype,
file_ptr_type, offset, request, error_code);
return;
}
src/mpi/romio/mpl/src/mem/mpl_trmem.cWe should use generalized parameter handling here
to allow use of the command line as well as environment
variables
src/mpi/romio/mpl/src/mem/mpl_trmem.cwhy do we skip the first few ints? [goodell@]
src/mpi/romio/mpl/src/mem/mpl_trmem.cNote head is no longer valid after MPL_trfree above
src/mpi/romio/mpl/src/dbg/mpl_dbg.cNeed to know how many process groups are known
src/mpi/romio/mpl/src/env/mpl_env.cWe need to provide a way to signal this error
src/mpi/romio/mpl/src/shm/mpl_shm_mmap.cPass (void **)shm_addr_ptr instead of (char **) shm_addr_ptr
since a util func should be generic
Currently not passing (void **) to reduce warning in nemesis
code which passes (char **) ptrs to be attached to a seg
src/mpi/romio/mpl/src/shm/mpl_shm_mmap.cClose local handle only when closing the shm handle
src/mpi/romio/mpl/src/thread/mpl_thread_posix.cfaster allocation, or avoid it all together?
src/mpi/romio/mpl/src/thread/mpl_thread_posix.cconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/src/thread/mpl_thread_solaris.cfaster allocation, or avoid it all together?
src/mpi/romio/mpl/src/thread/mpl_thread_solaris.cconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/src/str/mpl_str.cAssumes ASCII
src/mpi/romio/mpl/include/mpl_iov.hHow is IOV_LIMIT chosen?
src/mpi/romio/mpl/include/mpl_thread_posix.hmutex creation and destruction should be implemented as routines
because there is no reason to use macros (these are not on the performance
critical path). Making these macros requires that any code that might use
these must load all of the pthread.h (or other thread library) support.
src/mpi/romio/mpl/include/mpl_thread_posix.husing constant initializer if available
src/mpi/romio/mpl/include/mpl_thread_posix.hconvert errors to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/mpi/romio/mpl/include/mpl_thread.hDefine other error codes. For now, any non-zero value is an error.
src/mpi/romio/mpl/include/mpl_shm.hWhat if val is a non-null terminated string ?
src/mpi/romio/mpl/include/mpl_trmem.hConsider an option of specifying __attribute__((malloc)) for
gcc - this lets gcc-style compilers know that the returned pointer
does not alias any pointer prior to the call.
src/mpi/romio/mpi-io/write_alle.cwe should really ensure that the split_datatype remains
valid by incrementing the ref count in the write_allb.c routine
and decrement it here after setting the bytes
src/mpi/romio/mpi-io/write_ordb.cCheck for error code from WriteStridedColl?
src/mpi/romio/mpi-io/get_extent.chandle other file data representations
src/mpi/romio/mpi-io/write_ord.cCheck for error code from WriteStridedColl?
src/mpi/romio/mpi-io/iotestall.cTHIS IS NOT CORRECT (see above). But most applications won't
care
src/mpi/romio/mpi-io/seek_sh.cexplain why the barrier is necessary
src/mpi/romio/mpi-io/read_ord.cCheck for error code from ReadStridedColl?
src/mpi/romio/mpi-io/get_view.cIt is wrong to use MPI_Type_contiguous; the user could choose to
re-implement MPI_Type_contiguous in an unexpected way. Either use
MPID_Barrier as in MPICH or PMPI_Type_contiguous
src/mpi/romio/mpi-io/get_view.cDitto for MPI_Type_commit - use NMPI or PMPI
src/mpi/romio/mpi-io/get_view.cDitto for MPI_Type_xxx - use NMPI or PMPI
src/mpi/romio/mpi-io/glue/mpich/mpio_err.cThis is a hack in case no error handler was set
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.hshould plfd really be const const? Some handlers may need to change the plfd entry.
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.hbc actually contains port_name info
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.ctrace/log all the state transitions
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cneed to handle error condition better
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cZ1
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cZ1
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cis it needed ?
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cZ1
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cZ1
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cZ1
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cWe need to set addr and port using bc.
If a process is dynamically spawned, vc->pg is NULL.
In that case, same procedure is done
in MPID_nem_tcp_connect_to_root()
src/mpid/ch3/channels/nemesis/netmod/tcp/socksm.cXXX DJG do we need to do anything here to ensure that the final
close(TRUE) packet has made it into a writev call? The code might have a
race for queued messages.
src/mpid/ch3/channels/nemesis/netmod/tcp/tcp_getip.cThis should use the standard debug/logging routines and macros
src/mpid/ch3/channels/nemesis/netmod/llc/llc_poll.cvc is NULL when rank is MPI_ANY_SOURCE
src/mpid/ch3/channels/nemesis/netmod/llc/llc_poll.cvc is NULL when rank is MPI_ANY_SOURCE
src/mpid/ch3/channels/nemesis/netmod/llc/llc_poll.cshould 0 ?
src/mpid/ch3/channels/nemesis/netmod/llc/llc_poll.cHow to call a cancel_recv function
src/mpid/ch3/channels/nemesis/netmod/portals4/rptl.cturn this into a CVAR, or fraction of the event limit from
rptl_init
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_init.cturn ORIGIN_EVENTS into a CVAR
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_probe.cthis should use a custom handler that throws the data away inline
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_recv.cFor now, allocate a single large buffer to hold entire message
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_recv.cThis function is void, so we can't return an error. This function
cannot return an error because the queue functions (where the posted_recv
hooks are called) return no error code.
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_recv.cThis function is does not return an error because the queue
functions (where the posted_recv hooks are called) return no error
code. See also comment on cancel_recv.
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_recv.cThis function is does not return an error because the queue
functions (where the posted_recv hooks are called) return no error
code.
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_recv.cFor now, allocate a single large buffer to hold entire message
src/mpid/ch3/channels/nemesis/netmod/portals4/ptl_nm.cthis search/delete not be necessary if we set PTL_ME_UNEXPECTED_HDR_DISABLE
on the overflow list entries. However, doing so leads to PTL_IN_USE errors
during finalize in rare cases, even though all messages are handled.
src/mpid/ch3/channels/nemesis/include/mpidi_ch3_impl.hWe should use a pool allocator here
src/mpid/ch3/channels/nemesis/include/mpid_nem_datatypes.hWe are using this as an interprocess lock in the queue code, although
it's not strictly guaranteed to work for this scenario. These should really
use the "process locks" code, but it's in such terrible shape that it doesn't
really work for us right now. Also, because it has some inline assembly it's
not really a fair comparison for studying the impact of atomic instructions.
[goodell@ 2009-01-16]
src/mpid/ch3/channels/nemesis/include/mpid_nem_defs.hThis definition should be gotten from mpidi_ch3_impl.h
src/mpid/ch3/channels/nemesis/include/mpid_nem_queue.his a barrier needed here because of the control-only dependency?
src/mpid/ch3/channels/nemesis/include/mpid_nem_queue.hWe shouldn't really be using the MPID_Thread_mutex_* code but the
MPIDU_Process_locks code is a total mess right now. In the long term we need
to resolve this, but in the short run it should be safe on most (all?)
platforms to use these instead. Usually they will both boil down to a
pthread_mutex_t and and associated functions.
src/mpid/ch3/channels/nemesis/include/mpid_nem_post.hDo not export these definitions all the way into mpiimpl.h (which
will happen if this is included in mpidpost.h or mpidpre.h)
src/mpid/ch3/channels/nemesis/include/mpidi_ch3_pre.hused for debugging
src/mpid/ch3/channels/nemesis/include/mpidi_ch3_pre.hch3 assumes there is a field called sendq_head in the ch
portion of the vc. This is unused in nemesis and should be set
to NULL
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt.cwhere does this request get freed?
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt.cwhere does this request get freed?
src/mpid/ch3/channels/nemesis/src/mpid_nem_init.cDARIUS set these to default for now
src/mpid/ch3/channels/nemesis/src/mpid_nem_init.cDARIUS -- enable this assert once these functions are implemented
src/mpid/ch3/channels/nemesis/src/mpid_nem_init.cch3 assumes there is a field called sendq_head in the ch
portion of the vc. This is unused in nemesis and should be set
to NULL
src/mpid/ch3/channels/nemesis/src/ch3_win_fns.cget this from OS
src/mpid/ch3/channels/nemesis/src/ch3_win_fns.cThis needs to be fixed for heterogeneous systems
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_dma.crandomly chosen
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_dma.cThe knem module iovec is potentially different from the system
iovec. This causes all sorts of fun if you don't realize it and use the
system iovec directly instead. Eventually we need to either unify them
or avoid this extra copy.
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_dma.cThe knem module iovec is potentially different from the system
iovec. This causes all sorts of fun if you don't realize it and use the
system iovec directly instead. Eventually we need to either unify them
or avoid this extra copy.
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_dma.cwe should write our own function that isn't dependent on
the in-request iov array. This will let us use IOVs that are
larger than MPL_IOV_LIMIT.
src/mpid/ch3/channels/nemesis/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/nemesis/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/nemesis/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/nemesis/src/ch3_init.cCircular dependency. Before calling MPIDI_CH3_Init,
MPID_Init calls InitPG which calls MPIDI_PG_Create which calls
MPIDI_CH3_VC_Init. But MPIDI_CH3_VC_Init needs nemesis to be
initialized. We can't call MPIDI_CH3_Init before initializing
the PG, because it needs the PG.

There is a hook called MPIDI_CH3_PG_Init which is called from
MPIDI_PG_Create before MPIDI_CH3_VC_Init, but we don't have
the pg_rank in that function.

Maybe this shouldn't really be a FIXME, since I believe that
this issue will be moot once nemesis is a device.

So what we do now, is do nothing if MPIDI_CH3_VC_Init is
called before MPIDI_CH3_Init, and call MPIDI_CH3_VC_Init from
inside MPIDI_CH3_Init after initializing nemesis
src/mpid/ch3/channels/nemesis/src/ch3_init.cwhere does this vc get freed?
ANSWER (goodell@) - ch3u_port.c FreeNewVC
(but the VC_Destroy is in this file)
src/mpid/ch3/channels/nemesis/src/ch3_istartmsg.cwe are sometimes called from within the progress engine, we
shouldn't be calling the progress engine again
src/mpid/ch3/channels/nemesis/src/ch3i_comm.cThis has a serious design bug. It assumes that context ids are
globally unique (i.e., that if two processes have communicators with the
same context id, they're the same communicator), but this is not true.
This may result in two different communicators using the same
barier_vars. This code is being left in for now as an example of how to
override collective operations.
src/mpid/ch3/channels/nemesis/src/ch3i_comm.cneed a better solution here. e.g., allocate
some barrier_vars on the first barrier for the life of
the communicator (as is the case now), others must be
allocated for each barrier, then released. If we run
out of barrier_vars after that, then use msg_barrier.
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_vmsplice.cwe should write our own function that isn't dependent on
the in-request iov array. This will let us use IOVs that are
larger than MPL_IOV_LIMIT.
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_vmsplice.cset the error status of the req and complete it, rather than POP
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_vmsplice.cset the error status of the req and complete it, rather than POP
src/mpid/ch3/channels/nemesis/src/mpid_nem_lmt_vmsplice.cneed to handle the case where a VC is terminated due to
a process failure. We need to remove any outstanding LMT ops
for this VC.
src/mpid/ch3/channels/nemesis/src/mpid_nem_ckpt.cwe're only handling processes in our pg, so no dynamic connections
src/mpid/ch3/channels/sock/include/mpidu_sock.hThis is not the right way to add error values to an MPICH module.
Note that (a) the last class values are not respected by the error handling
code, (b) the entire point of codes and classes is to provide a
natural grouping of codes to a class, (c) this approach can only be used
by one module and hence breaks any component design, and (d) this is
what the MPI dynamic error codes and classes was designed for.
src/mpid/ch3/channels/sock/include/mpidu_socki.h!!!
src/mpid/ch3/channels/sock/include/mpidi_ch3_impl.hAny of these used in the ch3->channel interface should be
defined in a header file in ch3/include that defines the
channel interface
src/mpid/ch3/channels/sock/include/mpidi_ch3_pre.hThese should be removed
src/mpid/ch3/channels/sock/include/mpidi_ch3_pre.hAre the following packet extensions? Can the socket connect/accept
packets be made part of the util/sock support?
src/mpid/ch3/channels/sock/include/mpidi_ch3_pre.hExplain these; why is this separate from the VC state?
src/mpid/ch3/channels/sock/include/mpidi_ch3_post.hWe need to document all of these parameters. There should be no
ifdef that is not documented, other than those set by configure
src/mpid/ch3/channels/sock/src/ch3_isend.cShouldn't the vc->state also change?
src/mpid/ch3/channels/sock/src/ch3_progress.cMove thread stuff into some set of abstractions in order to remove
ifdefs
src/mpid/ch3/channels/sock/src/ch3_progress.cWas (USE_THREAD_IMPL == MPICH_THREAD_IMPL_NOT_IMPLEMENTED),
which really meant not-using-global-mutex-thread model . This
was true for the single threaded case, but was probably not intended
for that case
src/mpid/ch3/channels/sock/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/sock/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/sock/src/ch3_progress.cthe following should be handled by the close
protocol
src/mpid/ch3/channels/sock/src/ch3_progress.cTest for runtime thread level (here or where used)
src/mpid/ch3/channels/sock/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/sock/src/ch3_progress.cTest for runtime thread level
src/mpid/ch3/channels/sock/src/ch3_progress.cshould be appropriately abstracted somehow
src/mpid/ch3/channels/sock/src/ch3_progress.c(a) what does this do and where is it used and (b)
we could replace it with a #define for the single-method case
src/mpid/ch3/channels/sock/src/ch3_progress.cThis function also used in ch3u_connect_sock.c
src/mpid/ch3/channels/sock/src/ch3_progress.cIs dequeue/get next the operation we really want?
src/mpid/ch3/channels/sock/src/ch3_progress.cWhat is this routine for?
src/mpid/ch3/channels/sock/src/sock.cWhat do these mean? Why is 32 a good size (e.g., is it because
32*32 = 1024 if these are bits in a 4-byte int? In that case, should
these be related to a maximum processor count or an OS-defined fd limit?
src/mpid/ch3/channels/sock/src/sock.cWhy aren't these static
src/mpid/ch3/channels/sock/src/sock.cThese need to separate the operations from the thread-related
synchronization to ensure that the code that is independent of
threads is always the same. Also, the thread-level check needs
to be identical to all others, and there should be an option,
possibly embedded within special thread macros, to allow
runtime control of the thread level
src/mpid/ch3/channels/sock/src/sock.cDoes this need a runtime check on whether threads are in use?
src/mpid/ch3/channels/sock/src/sock.cLow usage operations like this should be a function for
better readability, modularity, and code size
src/mpid/ch3/channels/sock/src/sock.cAre these really optional? Based on their definitions, it looks
like they should only be used when debugging the code.
src/mpid/ch3/channels/sock/src/sock.cShould this use the CHKPMEM macros (perm malloc)?
src/mpid/ch3/channels/sock/src/sock.cWe need an abstraction for the thread sync operations
src/mpid/ch3/channels/sock/src/sock.cmove last element into current position and update sock associated with last element.
src/mpid/ch3/channels/sock/src/sock.cShouldn't this be an mpi error code?
src/mpid/ch3/channels/sock/src/sock.cWho allocates eventq tables? Should there be a check that these
tables are empty first?
src/mpid/ch3/channels/sock/src/sock.cIs this the name that we want to use (this was chosen
to match the original, undocumented name)
src/mpid/ch3/channels/sock/src/sock.cThe usual missing documentation (what are these routines for?
preconditions? who calls? post conditions?
src/mpid/ch3/channels/sock/src/sock.cWho calls? When? Should this be a finalize handler instead?
src/mpid/ch3/channels/sock/src/sock.cMove the thread-specific operations into thread-specific
routines (to allow for alternative thread sync models and
for runtime control of thread level)
src/mpid/ch3/channels/sock/src/sock.cIt would be better to include a special formatting
clue for system error messages (e.g., %dSE; in the recommended
revision for error reporting (that is, value (errno) is an int,
but should be interpreted as an System Error string)
src/mpid/ch3/channels/sock/src/sock.cWhat does this routine do? Why does it take a host description
instead of an interface name or address?
src/mpid/ch3/channels/sock/src/sock.cstrtok may change the contents of host_description. Shouldn't
the host description be a const char [] and not modified by this
routine?
src/mpid/ch3/channels/sock/src/sock.cFor ipv6, we should use getaddrinfo
src/mpid/ch3/channels/sock/src/sock.cSet error
src/mpid/ch3/channels/sock/src/sock.cWhat does this function do?
src/mpid/ch3/channels/sock/src/sock.cWhy is this the _immed file (what does immed stand for?)
src/mpid/ch3/channels/sock/src/sock.cWhat do any of these routines do? What are the arguments?
Special conditions (see the FIXME on len = SSIZE_MAX)? preconditions?
postconditions?
src/mpid/ch3/channels/sock/src/sock.cWhat does this function do? What are its arguments?
It appears to execute a nonblocking accept call
src/mpid/ch3/channels/sock/src/sock.cEither use the syscall macro or correctly wrap this in a
test for EINTR
src/mpid/ch3/channels/sock/src/sock.cThere should be a simpler macro for reporting errno messages
src/mpid/ch3/channels/sock/src/sock.cWho sets the socket buffer size? Why isn't the test
made at that time?
src/mpid/ch3/channels/sock/src/sock.cThere's normally no need to check that the socket buffer
size was set to the requested size. This should only be part of
some more verbose diagnostic output, not a general action
src/mpid/ch3/channels/sock/src/sock.cThere's normally no need to check that the socket buffer
size was set to the requested size. This should only be part of
some more verbose diagnostic output, not a general action
src/mpid/ch3/channels/sock/src/sock.cCut and paste code is a disaster waiting to happen.
Particularly in any non-performance critical section,
create a separate routine instead of using cut and paste.
src/mpid/ch3/channels/sock/src/sock.cmultiple passes should be made if
len > SSIZE_MAX and nb == SSIZE_MAX
src/mpid/ch3/channels/sock/src/sock.cThis is a scary test/assignment. It needs an explanation
(presumably that this routine will be called again if len is
shortened. However, in that case, the description of the routine
(which is also missing!!!!) needs to be very clear about this
requirement.
src/mpid/ch3/channels/sock/src/sock.cmultiple passes should be made if len > SSIZE_MAX and nb == SSIZE_MAX
src/mpid/ch3/channels/sock/src/sock.cWe need (1) a standardized test for including multithreaded
code and (2) include support for user requests for a lower-level
of thread safety. Finally, things like this should probably
be implemented as an abstraction (e.g., wakeup_progress_threads?)
rather than this specific code.
src/mpid/ch3/channels/sock/src/sock.cThis routine is misnamed; it is really get_interface_name (in the
case where there are several networks available to the calling process,
this picks one but even in the current code can pick a different
interface if a particular environment variable is set) .
src/mpid/ch3/channels/sock/src/sock.cIs this documented? How does it work if the process manager
cannot give each process a different value for an environment
name? What if a different interface is needed?
src/mpid/ch3/channels/sock/src/sock.cThis function violates the internationalization design by
using English language strings rather than the error translation mechanism.
This unnecessarily breaks the goal of allowing internationalization.
Read the design documentation and if there is a problem, raise it rather
than ignoring it.
src/mpid/ch3/channels/sock/src/sock.cIt appears that this function was used instead of making use of the
existing MPI-2 features to extend MPI error classes and code, of to export
messages to non-MPI application
src/mpid/ch3/channels/sock/src/sock.creturn code? If an error occurs do we return it
instead of the error specified in the event?
src/mpid/ch3/channels/sock/src/ch3_isendv.cthe current code only agressively writes the first IOV.
Eventually it should be changed to agressively write
as much as possible. Ideally, the code would be shared between
the send routines and the progress engine.
src/mpid/ch3/channels/sock/src/ch3_isendv.cShouldn't the vc->state also change?
src/mpid/ch3/src/ch3u_rma_progress.cwe should optimize the issuing pattern here.
src/mpid/ch3/src/mpid_port.cWe should instead ask the mpid_pg routines to give us
a connection string. There may need to be a separate step to
restrict us to a connection information that is only valid for
connections between processes that are started separately (e.g.,
may not use shared memory). We may need a channel-specific
function to create an exportable connection string.
src/mpid/ch3/src/mpid_recv.cin the common case, we want to simply complete the message
and make as few updates as possible.
Note in addition that this routine is used only by MPI_Recv (a
blocking routine; the intent of the interface (which returns
a request) was to simplify the handling of the case where the
message was not found in the unexpected queue.
src/mpid/ch3/src/mpid_recv.cWe do not need to add a datatype reference if
the request is blocking. This is currently added because
of the actions that are taken when a request is freed.
(specifically, the datatype and comm both have their refs
decremented, and are freed if the refs are zero)
src/mpid/ch3/src/ch3u_request.cRMA ops shouldn't need to be set except when creating a
request for RMA operations
src/mpid/ch3/src/ch3u_request.cwe should drain the data off the pipe here, but we
don't have a buffer to drain it into. should this be
a fatal error?
src/mpid/ch3/src/ch3u_comm.cWe should not unconditionally disable multicast.
Test to make sure it's available before choosing to
enable or disable it.
src/mpid/ch3/src/ch3u_comm.cThis algorithm assumes that the number of processes in group is
very small (like 1). So doing a linear search for them in comm is better
than sorting the procs in comm and group then doing a binary search
src/mpid/ch3/src/ch3u_comm.cThis won't work for dynamic procs
src/mpid/ch3/src/mpidi_isend_self.cExplain this function
src/mpid/ch3/src/mpidi_isend_self.cshould there be a simpler version of this for short messages,
particularly contiguous ones? See also the FIXME about buffering
short messages
src/mpid/ch3/src/mpidi_isend_self.cInsert code here to buffer small sends in a temporary buffer?
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cPlace all of this within the mpid_comm_spawn_multiple file
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cWe can avoid these two routines if we define PMI as using
MPI info values
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cinfo may be needed for port name
src/mpid/ch3/src/ch3u_comm_spawn_multiple.ccheating on constness
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cThis is *really* awkward. We should either
Fix on MPI-style info data structures for PMI (avoid unnecessary
duplication) or add an MPIU_Info_getall(...) that creates
the necessary arrays of key/value pairs
src/mpid/ch3/src/ch3u_comm_spawn_multiple.ctranslate the pmi error codes here
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cWhat does this function do? Who calls it? Can we assume that
it is called only dynamic process operations (specifically spawn)
are supported? Do we need the concept of a port? For example,
could a channel that supported only shared memory call this (it doesn't
look like it right now, so this could go into util/sock, perhaps?

It might make more sense to have this function provided as a function
pointer as part of the channel init setup, particularly since this
function appears to access channel-specific storage (MPIDI_CH3_Process)
src/mpid/ch3/src/ch3u_comm_spawn_multiple.cDARIUS
src/mpid/ch3/src/mpid_comm_spawn_multiple.cCorrect description of function
src/mpid/ch3/src/ch3u_rma_sync.cdo we need to wait for remote completion?
src/mpid/ch3/src/ch3u_win_fns.cThis needs to be fixed for heterogeneous systems
src/mpid/ch3/src/ch3u_win_fns.cIf we wanted to validate the transfer as within range at the
origin, we'd also need the window size.
src/mpid/ch3/src/ch3u_handle_recv_req.cTemp to avoid SEGV when memory tracing
src/mpid/ch3/src/ch3u_handle_recv_req.cthis function is same with PktHandler_FOP(), should
do code refactoring on both of them.
src/mpid/ch3/src/ch3u_handle_recv_req.cMT: Must be done atomically
src/mpid/ch3/src/ch3u_handle_recv_req.cMT: The setting of the lock type must be done atomically
src/mpid/ch3/src/ch3u_handle_recv_req.cMT: All queue accesses need to be made atomic
src/mpid/ch3/src/mpid_send.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/mpid_send.cflow control: limit number of outstanding eager messages
containing data and need to be buffered by the receiver
src/mpid/ch3/src/ch3u_eager.cWhy do we set the message type?
src/mpid/ch3/src/ch3u_eager.cWe want to set the OnDataAvail to the appropriate
function, which depends on whether this is an RMA
request or a pt-to-pt request.
src/mpid/ch3/src/ch3u_eager.cThe MPICH tests do not exercise this branch
src/mpid/ch3/src/ch3u_eager.cthe AEU branch gives (rreq->cc==1) but the complete at the
bottom of this function will decr it. Is everything going to be
cool in this case? No upper layer has a pointer to rreq yet
(it's unexpected and freshly allocated)
src/mpid/ch3/src/ch3u_eager.cWhen eagershort is enabled, provide a preallocated
space for short messages (which is used even if eager short
is not used), since we don't want to have a separate check
to figure out which buffer we're using (or perhaps we should
have a free-buffer-pointer, which can be null if it isn't
a buffer that we've allocated).
src/mpid/ch3/src/ch3u_eager.cThis is not optimized for short messages, which
should have the data in the same packet when the data is
particularly short (e.g., one 8 byte long word)
src/mpid/ch3/src/ch3u_eager.can error packet should be sent back to the sender
indicating that the ready-send failed. On the send
side, the error handler for the communicator can be invoked
even if the ready-send request has already
completed.
src/mpid/ch3/src/ch3u_buffer.cExplain the contents of this file
src/mpid/ch3/src/ch3u_buffer.cExplain this routine .
Used indirectly by mpid_irecv, mpid_recv (through MPIDI_CH3_RecvFromSelf) and
in mpidi_isend_self.c
src/mpid/ch3/src/ch3u_rndv.cfill temporary IOV or pack temporary buffer after send to hide
some latency. This requires synchronization
because the CTS packet could arrive and be processed before the above
iStartmsg completes (depending on the progress
engine, threads, etc.).
src/mpid/ch3/src/ch3u_rndv.cWhat if the receive user buffer is not big enough to
hold the data about to be cleared for sending?
src/mpid/ch3/src/ch3u_rndv.cIdeally we could specify that a req not be returned.
This would avoid our having to decrement the
reference count on a req we don't want/need.
src/mpid/ch3/src/mpid_comm_disconnect.cDescribe what more might be required
src/mpid/ch3/src/mpidi_pg.cThese routines need a description. What is their purpose? Who
calls them and why? What does each one do?
src/mpid/ch3/src/mpidi_pg.cstraighten out the use of PMI_Finalize - no use after
PG_Finalize
src/mpid/ch3/src/mpidi_pg.cThis routine needs to make it clear that the pg_id, for example
is saved; thus, if the pg_id is a string, then that string is not
copied and must be freed by a PG_Destroy routine
src/mpid/ch3/src/mpidi_pg.cit would be good if we could make this assertion.
Unfortunately, either:
1) We're not being disciplined and some caller of this
function doesn't bother to manage all the refcounts
because he thinks he knows better. Annoying, but not
strictly a bug.
(wdg - actually, that is a bug - managing the ref
counts IS required and missing one is a bug.)
2) There is a real bug lurking out there somewhere and we
just haven't hit it in the tests yet.
src/mpid/ch3/src/mpidi_pg.cWhat does DEV_IMPLEMENTS_KVS mean? Why is it used? Who uses
PG_To_string and why?
src/mpid/ch3/src/mpidi_pg.cThis is a temporary hack for devices that do not define
MPIDI_DEV_IMPLEMENTS_KVS
FIXME: MPIDI_DEV_IMPLEMENTS_KVS should be removed
src/mpid/ch3/src/mpidi_pg.cThis is a hack to avoid including shared-memory
queue names in the business card that may be used
by processes that were not part of the same COMM_WORLD.
To fix this, the shared memory channels should look at the
returned connection info and decide whether to use
sockets or shared memory by determining whether the
process is in the same MPI_COMM_WORLD.
src/mpid/ch3/src/mpidi_pg.cThe more general problem is that the connection information
needs to include some information on the range of validity (e.g.,
all processes, same comm world, particular ranks), and that
representation needs to be scalable
src/mpid/ch3/src/mpidi_pg.cthis should be a failure to call this routine
src/mpid/ch3/src/mpidi_pg.cTurn this into a valid error code create/return
src/mpid/ch3/src/mpidi_pg.cThis is a hack, and it doesn't even work
src/mpid/ch3/src/mpidi_pg.cMT: This should be a fetch and increment for thread-safety
src/mpid/ch3/src/mpidi_pg.cThis routine should invoke a close method on the connection,
rather than have all of the code here
src/mpid/ch3/src/mpidi_pg.cIf the reference count for the vc is not 0,
something is wrong
src/mpid/ch3/src/mpidi_printf.cWhat are these routines for? Who uses them? Why are they different
from the src/util/dbg routines?
src/mpid/ch3/src/mpidi_printf.cIt would be much better if the routine to print packets used
routines defined by packet type, making it easier to modify the handling
of packet types (and allowing channels to customize the printing
of packets). For example, an array of function pointers, indexed by
packet type, could be used.
Also, these routines should not use MPL_DBG_MSG_* functions, instead they should
us a simple fprintf with a style allowance (so that the style checker
won't flag the use as a possible problem).

This should switch to using a table of functions

MPIDI_PktPrintFunctions[pkt->type](stdout,pkt);
src/mpid/ch3/src/mpidi_printf.cMove these RMA descriptions into the RMA code files
src/mpid/ch3/src/mpid_cancel_send.cThis should call a channel-provided routine to deliver the
cancel message, once the code decides that the request can still
be cancelled
src/mpid/ch3/src/mpid_cancel_send.cshould be a decr and assert, not a set
src/mpid/ch3/src/mpid_cancel_send.cshould be a decr and assert, not a set
src/mpid/ch3/src/mpid_cancel_send.cif send cancellation packets are allowed to arrive out-of-order
with respect to send packets, then we need to
timestamp send and cancel packets to insure that a cancellation request
does not bypass the send packet to be cancelled
and erroneously cancel a previously sent message with the same request
handle.
src/mpid/ch3/src/mpid_cancel_send.cA timestamp is more than is necessary; a message sequence number
should be adequate.
src/mpid/ch3/src/mpid_cancel_send.cNote that this routine is called from within the packet handler.
If the message queue mutex is different from the progress mutex, this
must be protected within a message-queue mutex
src/mpid/ch3/src/mpid_cancel_send.cThis is called within the packet handler
src/mpid/ch3/src/mpid_vc.cWhat should this do? See proc group and vc discussion
src/mpid/ch3/src/mpid_vc.cthis is still bogus, the VCRT may contain a mix of
dynamic and non-dynamic VCs, so the ref_count isn't
guaranteed to have started at 2. The best thing to do might
be to avoid overloading the reference counting this way and
use a separate check for dynamic VCs (another flag? compare
PGs?)
src/mpid/ch3/src/mpid_vc.cthe correct test is ACTIVE or REMOTE_CLOSE
src/mpid/ch3/src/mpid_vc.cThese routines belong in a different place
src/mpid/ch3/src/mpid_vc.ca quick check on the min/max values of the lpid
for this process group could help speed this search
src/mpid/ch3/src/mpid_vc.cFor testing, we just test in place
src/mpid/ch3/src/mpid_vc.cWe need a cleaner way to handle this case than using an ifdef.
We could have an empty version of MPID_PG_BCast in ch3u_port.c, but
that's a rather crude way of addressing this problem. Better is to
make the handling of local and remote PIDS for the dynamic process
case part of the dynamic process "module"; devices that don't support
dynamic processes (and hence have only COMM_WORLD) could optimize for
that case
src/mpid/ch3/src/mpid_vc.cWe need a better abstraction for initializing the thread state
for an object
src/mpid/ch3/src/ch3u_port.cIf dynamic processes are not supported, this file will contain
no code and some compilers may warn about an "empty translation unit"
src/mpid/ch3/src/ch3u_port.cpg_translation is used for ?
src/mpid/ch3/src/ch3u_port.cnot thread-safe
src/mpid/ch3/src/ch3u_port.cDescribe the algorithm used here, and what routine
is user on the other side of this connection
src/mpid/ch3/src/ch3u_port.cThis code is still broken for the following case:
If the same process opens connections to the multiple
processes, this context ID might get out of sync.
src/mpid/ch3/src/ch3u_port.cwe probably need a unique context_id.
src/mpid/ch3/src/ch3u_port.cExplain why
src/mpid/ch3/src/ch3u_port.cWhy do we do a dup here?
src/mpid/ch3/src/ch3u_port.cError, the pg_list is broken
src/mpid/ch3/src/ch3u_port.cWe should use the PG destroy function for this, and ensure that
the PG fields are valid for that function
src/mpid/ch3/src/ch3u_port.cwhy is this commented out?
src/mpid/ch3/src/ch3u_port.cHow much of this could/should be common with the
upper level (src/mpi/comm/ *.c) code? For best robustness,
this should use the same routine (not copy/paste code) as
in the upper level code.
src/mpid/ch3/src/ch3u_port.cThe free and the create routines should be in the same file
src/mpid/ch3/src/ch3u_port.cWhat is an Accept queue and who uses it?
Is this part of the connect/accept support?
These routines appear to be called by channel progress routines;
perhaps this belongs in util/sock (note the use of a port_name_tag in the
dequeue code, though this could be any string).

Are the locks required? If this is only called within the progress
engine, then the progress engine locks should be sufficient. If a
finer grain lock model is used, it needs to be very carefully
designed and documented.
src/mpid/ch3/src/mpid_isend.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/mpid_isend.cThe routines MPID_Isend, MPID_Issend, MPID_Irsend are nearly
identical. It would be better if these did roughly:

MPID_Irsend -> always eager send (with ready mode for error detection)
MPID_Issend -> always rendezvous send
MPID_Isend -> chose eager/rendezvous based on a threshold (and consider
making the threshold configurable at either compile time (for best
low-latency performance) or run-time (for application tuning).

Then the 3 routines share little code, particularly if the eager/rendezvous
implementations are in their own routine
src/mpid/ch3/src/mpid_isend.cflow control: limit number of outstanding eager messages
containing data and need to be buffered by the receiver
src/mpid/ch3/src/mpid_isend.cfill temporary IOV or pack temporary buffer after send to
hide some latency. This requires synchronization
because the CTS packet could arrive and be processed before the
above iStartmsg completes (depending on the progress
engine, threads, etc.).
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.cFor contig and very short operations, use a streamlined op
src/mpid/ch3/src/ch3u_rma_ops.ccurrently we only piggyback LOCK flag with op using predefined datatypes
for both origin and target data. We should extend this optimization to derived
datatypes as well.
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.cFor contig and very short operations, use a streamlined op
src/mpid/ch3/src/ch3u_rma_ops.ccurrently we only piggyback LOCK flag with op using predefined datatypes
for both origin and target data. We should extend this optimization to derived
datatypes as well.
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.ccurrently we only piggyback LOCK flag with op using predefined datatypes
for both origin and target data. We should extend this optimization to derived
datatypes as well.
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.ccurrently we only piggyback LOCK flag with op using predefined datatypes
for origin, target and result data. We should extend this optimization to derived
datatypes as well.
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.cFor shared memory windows, we should provide an implementation
that uses a processor atomic operation.
src/mpid/ch3/src/ch3u_rma_ops.cHere we decide whether to perform SHM operations by checking if origin and target are on
the same node. However, in ch3:sock, even if origin and target are on the same node, they do
not within the same SHM region. Here we filter out ch3:sock by checking shm_allocated flag first,
which is only set to TRUE when SHM region is allocated in nemesis.
In future we need to figure out a way to check if origin and target are in the same "SHM comm".
src/mpid/ch3/src/ch3u_rma_ops.cFor shared memory windows, we should provide an implementation
that uses a processor atomic operation.
src/mpid/ch3/src/mpid_issend.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/mpid_issend.cfill temporary IOV or pack temporary buffer after send to
hide some latency. This requires synchronization
because the CTS packet could arrive and be processed before the
above iStartmsg completes (depending on the progress
engine, threads, etc.).
src/mpid/ch3/src/mpid_mprobe.ccould this be replaced with a progress_wait?
src/mpid/ch3/src/mpid_mprobe.ccould this be replaced with a progress_wait?
src/mpid/ch3/src/ch3u_eagersync.cThis sometimes segfaults
src/mpid/ch3/src/ch3u_handle_recv_pkt.cWe can turn this into something like

MPIR_Assert(pkt->type <= MAX_PACKET_TYPE);
mpi_errno = MPIDI_CH3_ProgressFunctions[pkt->type](vc,pkt,rreqp);

in the progress engine itself. Then this routine is not necessary.
src/mpid/ch3/src/ch3u_handle_recv_pkt.cSet OnDataAvail to 0? If not, why not?
src/mpid/ch3/src/ch3u_handle_recv_pkt.cto improve performance, allocate temporary buffer from a
specialized buffer pool.
src/mpid/ch3/src/ch3u_handle_recv_pkt.cto avoid memory exhaustion, integrate buffer pool management
with flow control
src/mpid/ch3/src/ch3u_handle_recv_pkt.cWe want to set the OnDataAvail to the appropriate
function, which depends on whether this is an RMA
request or a pt-to-pt request.
src/mpid/ch3/src/ch3u_handle_recv_pkt.cto improve performance, allocate temporary buffer from a
specialized buffer pool.
src/mpid/ch3/src/ch3u_handle_recv_pkt.cto avoid memory exhaustion, integrate buffer pool management
with flow control
src/mpid/ch3/src/ch3u_handle_recv_pkt.cwe still need to implement flow control. As a reminder,
we don't mark these parameters as unused, because a full implementation
of this routine will need to make use of all 4 parameters
src/mpid/ch3/src/ch3u_handle_recv_pkt.cThis should be initialized by a separate, RMA routine.
That would allow different RMA implementations.
We could even do lazy initialization (make this part of win_create)
src/mpid/ch3/src/mpid_getpname.cMake thread safe
src/mpid/ch3/src/mpid_improbe.cIt would be helpful to know if the Progress_poke
operation causes any change in state; we could then avoid
a second test of the receive queue if we knew that nothing
had changed
src/mpid/ch3/src/mpid_irsend.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/ch3u_handle_connection.cDecrement the reference count? Who increments?
src/mpid/ch3/src/ch3u_handle_connection.cThe reference count is often already 0. But
not always
src/mpid/ch3/src/ch3u_handle_connection.cWho increments the reference count that
this is decrementing?
src/mpid/ch3/src/ch3u_handle_connection.cThis should be done when the reference
count of the vc is first decremented
src/mpid/ch3/src/ch3u_handle_connection.cThis won't work for dynamic procs
src/mpid/ch3/src/ch3u_handle_connection.cCurrently this only handles failed processes in
comm_world. We need to fix hydra to include the pgid along
with the rank, then we need to create the failed group from
something bigger than comm_world.
src/mpid/ch3/src/mpid_init.cThis does not belong here
src/mpid/ch3/src/mpid_init.cthe PMI init function should ONLY do the PMI operations, not the
process group or bc operations. These should be in a separate routine
src/mpid/ch3/src/mpid_init.cThis is a good place to check for environment variables
and command line options that may control the device
src/mpid/ch3/src/mpid_init.cWhy are pg_size and pg_rank handled differently?
src/mpid/ch3/src/mpid_init.cWhy do we add a ref to pg here?
src/mpid/ch3/src/mpid_init.cTo allow just the "root" process to
request the port and then use MPIR_Bcast_intra to
distribute it to the rest of the processes,
we need to perform the Bcast after MPI is
otherwise initialized. We could do this
by adding another MPID call that the MPI_Init(_thread)
routine would make after the rest of MPI is
initialized, but before MPI_Init returns.
In fact, such a routine could be used to
perform various checks, including parameter
consistency value (e.g., all processes have the
same environment variable values). Alternately,
we could allow a few routines to operate with
predefined parameter choices (e.g., bcast, allreduce)
for the purposes of initialization.
src/mpid/ch3/src/mpid_init.cCheck that this intercommunicator gets freed in MPI_Finalize
if not already freed.
src/mpid/ch3/src/mpid_init.cDocument this
src/mpid/ch3/src/mpid_init.cWe may want to allow the channel to ifdef out the use
of PMI calls, or ask the channel to provide stubs that
return errors if the routines are in fact used
src/mpid/ch3/src/mpid_init.cWe can allow the channels to tell the PG how to get
connection information by passing the pg to the channel init routine
src/mpid/ch3/src/mpid_init.cWho is this for and where does it belong?
src/mpid/ch3/src/mpid_init.chas_args and has_env need to come from PMI eventually...
src/mpid/ch3/src/mpid_init.cThe PG code should supply these, since it knows how the
pg_ids and other data are represented
src/mpid/ch3/src/mpid_abort.cWe should move this into a header file so that we don't
need the ifdef. Also, don't use exit (add to coding check) since
not safe in windows. To avoid confusion, define a RobustExit? or
MPL_exit?
src/mpid/ch3/src/mpid_abort.cThis routine *or* MPI_Abort should provide abort callbacks,
similar to the support in MPI_Finalize
src/mpid/ch3/src/mpid_abort.cDo we want the rank of the input communicator here
or the rank of comm world? The message gives the rank but not the
communicator, so using other than the rank in comm world does not
identify the process, as the message suggests
src/mpid/ch3/src/mpid_abort.cNot internationalized
src/mpid/ch3/src/mpid_abort.cNot internationalized
src/mpid/ch3/src/mpid_abort.cWhat is the scope for PMI_Abort? Shouldn't it be one or more
process groups? Shouldn't abort of a communicator abort either the
process groups of the communicator or only the current process?
Should PMI_Abort have a parameter for which of these two cases to
perform?
src/mpid/ch3/src/mpid_ssend.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/mpid_startall.cThis bsend header shouldn't be needed (the function prototype
should be in mpiimpl.h), to allow a devices MPID_Startall to use the
MPIR_Bsend_isend function
src/mpid/ch3/src/mpid_startall.cConsider using function pointers for invoking persistent requests;
if we made those part of the public request structure, the top-level routine
could implement startall unless the device wanted to study the requests
and reorder them
src/mpid/ch3/src/mpid_startall.cWhere is the memory registration call in the init routines,
in case the channel wishes to take special action (such as pinning for DMA)
the memory? This was part of the design.
src/mpid/ch3/src/mpid_startall.cThe odd 7th arg (match.context_id - comm->context_id)
is probably to get the context offset. Do we really need the
context offset? Is there any case where the offset isn't zero?
src/mpid/ch3/src/mpid_startall.cMove the routines that initialize the persistent requests into this file,
since startall must be used with all of them
src/mpid/ch3/src/mpid_finalize.cThis routine needs to be factored into finalize actions per module,
In addition, we should consider registering callbacks for those actions
rather than direct routine calls.
src/mpid/ch3/src/mpid_finalize.cThe correct action here is to begin a shutdown protocol
that lets other processes know that this process is now
in finalize.

Note that only requests that have been freed with MPI_Request_free
are valid at this point; other pending receives can be ignored
since a valid program should wait or test for them before entering
finalize.

The easist fix is to allow an MPI_Barrier over comm_world (and
any connected processes in the MPI-2 case). Once the barrier
completes, all processes are in finalize and any remaining
unmatched receives will never be matched (by a correct program;
a program with a send in a separate thread that continues after
some thread calls MPI_Finalize is erroneous).

Avoiding the barrier is hard. Consider this sequence of steps:
Send in-finalize message to all connected processes. Include
information on whether there are pending receives.
(Note that a posted receive with any source is a problem)
(If there are many connections, then this may take longer than
the barrier)
Allow connection requests from anyone who has not previously
connected only if there is an possible outstanding receive;
reject others with a failure (causing the source process to
fail).
Respond to an in-finalize message with the number of posted receives
remaining. If both processes have no remaining receives, they
can both close the connection.

Processes with no pending receives and no connections can exit,
calling PMI_Finalize to let the process manager know that they
are in a controlled exit.

Processes that still have open connections must then try to contact
the remaining processes.

August 2010:

The barrier has been removed so that finalize won't hang when
another processes has died. This allows processes to finalize
and exit while other processes are still executing. This has
the following consequences:

* If a process tries to send a message to a process that has
exited before completing the matching receive, it will now
get an error. This is an erroneous program.

* If a process finalizes before completing a nonblocking
send, the message might not be sent. Similarly, if it
finalizes before completing all receives, the sender may
get an error. These are erroneous programs.

* A process may isend to another process that has already
terminated, then cancel the send. The program is not
erroneous in this case, but this will result in an error.
This can be fixed by not returning an error until the app
completes the send request. If the app cancels the
request, we need to to search the pending send queue and
cancel it, in which case an error shouldn't be generated.
src/mpid/ch3/src/mpid_finalize.cThe close actions should use the same code as the other
connection close code
src/mpid/ch3/src/mpid_iprobe.cThe routine CH3U_Recvq_FU is used only by the probe functions;
it should atomically return the flag and status rather than create
a request. Note that in some cases it will be possible to
atomically query the unexpected receive list (which is what the
probe routines are for).
src/mpid/ch3/src/mpid_iprobe.cIt would be helpful to know if the Progress_poke
operation causes any change in state; we could then avoid
a second test of the receive queue if we knew that nothing
had changed
src/mpid/ch3/src/mpid_rsend.cHOMOGENEOUS SYSTEMS ONLY -- no data conversion is performed
src/mpid/ch3/src/mpid_rsend.cHow does this differ from eager send? It should differ in
only a few bits (e.g., indicate that the send is ready and should
fail if there is no matching receive)
src/mpid/ch3/src/ch3u_recvq.cRecvq_lock/unlock removed because it is not needed for the SINGLE_CS
approach and we might want a different, non-lock-based approach in
a finer-grained thread-sync version. For example,
some routines can be implemented in a lock-free
fashion (because the user is required to guarantee non-conflicting
accesses, such as doing a probe and a receive that matches in different
threads).

There are a lot of routines here. Do we really need them all?

The search criteria can be implemented in a single 64 bit compare on
systems with efficient 64-bit operations. The rank and contextid can also
in many cases be combined into a single 32-bit word for the comparison
(in which case the message info should be stored in the queue in a
naturally aligned, 64-bit word.

src/mpid/ch3/src/ch3u_recvq.cIf this routine is only used by probe/iprobe, then we don't need
to set the cancelled field in status (only set for nonblocking requests)
src/mpid/ch3/util/unordered/unordered.cThis should probably be *rreqp = NULL?
src/mpid/ch3/util/unordered/unordered.cprocessing send cancel requests requires that we be
aware of pkts in the reorder queue
src/mpid/ch3/util/sock/findinterfaces.cTHIS IS WRONG. INSTEAD, SPECIFY THE SPECIFIC FEATURE LEVEL
NEEDED, AND THEN ONLY IF A CONFIGURE TEST SHOWS THAT IT IS REQUIRED
THESE NEED TO BE SET FOR ALL COMPILATIONS TO AVOID HAVING DIFFERENT
FILES COMPILED WITH DIFFERENT AND INCOMPATIBLE HEADER FILES.

WHAT IS APPARENTLY NEEDED (SEE /usr/include/features.h ON A LINUX
SYSTEM) IS EITHER _BSD_SOURCE OR _SVID_SOURCE; THIS MUST BE SET
CONSISTENTLY FOR ALL FILES COMPILED AS PART OF MPICH
src/mpid/ch3/util/sock/ch3u_init_sock.cWhy are these unused?
src/mpid/ch3/util/sock/ch3u_init_sock.cGet the size from the process group
src/mpid/ch3/util/sock/ch3u_init_sock.cThis should probably be the same as MPIDI_VC_InitSock. If
not, why not?
src/mpid/ch3/util/sock/ch3u_init_sock.cNote that MPIDI_CH3_VC_Init sets state, sendq_head and tail.
so this should be MPIDI_CH3_VC_Init( &pg_p->vct[p] );
followed by MPIDI_VC_InitSock( ditto );
In fact, there should be a single VC_Init call here
src/mpid/ch3/util/sock/ch3u_init_sock.cWhy isn't this MPIDI_VC_Init( vc, NULL, 0 )?
src/mpid/ch3/util/sock/ch3u_init_sock.cThis doesn't belong here, since the pg is not created in
this routine
src/mpid/ch3/util/sock/ch3u_connect_sock.cWe need a little security here to avoid having a random port scan
crash the process. Perhaps a "secret" value for each process could be
published in the key-val space and subsequently sent in the open pkt.
src/mpid/ch3/util/sock/ch3u_connect_sock.cDescribe what these routines do
src/mpid/ch3/util/sock/ch3u_connect_sock.cClean up use of private packets (open/accept)
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis size is unchanging, so get it only once (at most);
we might prefer for connections to simply point at the single process
group to which the remote process belong
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhy does the name include "to_root"?
src/mpid/ch3/util/sock/ch3u_connect_sock.cDescribe the algorithm for the connection logic
src/mpid/ch3/util/sock/ch3u_connect_sock.cwhere does this vc get freed?
src/mpid/ch3/util/sock/ch3u_connect_sock.cThere may need to be an additional routine here, to ensure that the
channel is initialized for this pair of process groups (this process
and the remote process to which the vc will connect).
src/mpid/ch3/util/sock/ch3u_connect_sock.cTo avoid this global (MPIDI_CH3I_sock_set) which is
used only in ch3_progress.c and ch3_progress_connect.c in the channels,
this should be a call into the channel, asking it to setup the
socket for a connection and return the connection. That will
keep the socket set out of the general ch3 code, even if this
is the socket utility functions.
src/mpid/ch3/util/sock/ch3u_connect_sock.cThese are small routines; we may want to bring them together
into a more specific post-connection-for-sock
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis is a hack to allow Windows to continue to use
the host description string instead of the interface address
bytes when posting a socket connection. This should be fixed
by changing the Sock_post_connect to only accept interface
address. Note also that Windows does not have the inet_pton
routine; the Windows version of this routine will need to
be identified or written. See also channels/sock/ch3_progress.c
src/mpid/ch3/util/sock/ch3u_connect_sock.cWe should start switching to getaddrinfo instead of
gethostbyname
src/mpid/ch3/util/sock/ch3u_connect_sock.cWe don't make use of the ifname in Windows in order to
provide backward compatibility with the (undocumented) host
description string used by the socket connection routine
MPIDI_CH3I_Sock_post_connect. We need to change to an interface-address
(already resolved) based description for better scalability and
to eliminate reliance on fragile DNS services. Note that this is
also more scalable, since the DNS server may serialize address
requests. On most systems, asking for the host info of yourself
is resolved locally (i.e., perfectly parallel). Regrettably, not
all systems do this (e.g., some versions of FreeBSD).
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhat does the above comment mean?
src/mpid/ch3/util/sock/ch3u_connect_sock.cseparate out the accept and connect sides to make it easier
to follow the logic
src/mpid/ch3/util/sock/ch3u_connect_sock.cIs there an assumption about conn->state?
src/mpid/ch3/util/sock/ch3u_connect_sock.cwhere does this vc get freed?
src/mpid/ch3/util/sock/ch3u_connect_sock.cPossible ambiguous state (two ways to get to OPEN_LSEND)
src/mpid/ch3/util/sock/ch3u_connect_sock.cis this the correct assert?
src/mpid/ch3/util/sock/ch3u_connect_sock.cShould conn->vc be freed? Who allocated? Why not?
src/mpid/ch3/util/sock/ch3u_connect_sock.cShould probably reduce ref count on conn->vc
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhat happens to the state of the associated VC?
Why isn't it changed? Is there an assert here,
such as conn->vc->conn != conn (there is another connection
chosen for the vc)?
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhat does post close do here?
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis should really be combined with handle_conn_event
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis routine is called when? What is valid in conn?
src/mpid/ch3/util/sock/ch3u_connect_sock.cthe connect side of this sets conn->vc to NULL. Why is
this different? The code that checks CONN_STATE_CLOSING uses
conn == NULL to identify intentional close, which this
appears to be.
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhat does this do?
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis is a hack to allow Windows to continue to use
the host description string instead of the interface address
bytes when posting a socket connection. This should be fixed
by changing the Sock_post_connect to only accept interface
address.
src/mpid/ch3/util/sock/ch3u_connect_sock.cWhat does this do?
src/mpid/ch3/util/sock/ch3u_connect_sock.cThis function also used in channels/sock/src/ch3_progress.c
src/mpid/ch3/include/mpidimpl.hWe want to avoid even storing information about the builtins
if we can
src/mpid/ch3/include/mpidimpl.hXXX DJG for TLS hack
src/mpid/ch3/include/mpidimpl.hWhy does a send request need the match information?
Is that for debugging information? In case the initial envelope
cannot be sent? Ditto for the dev.user_buf, count, and datatype
fields when the data is sent eagerly.

The following fields needed to be set:
datatype_ptr
status.MPI_ERROR

Note that this macro requires that rank, tag, context_offset,
comm, buf, datatype, and count all be available with those names
(they are not arguments to the routine)
src/mpid/ch3/include/mpidimpl.hWe've moved to allow finer-grain critical sections...
src/mpid/ch3/include/mpidimpl.hDetermine which of these functions should be exported to all of
the MPICH routines and which are internal to the device implementation
src/mpid/ch3/include/mpidimpl.hThis duplicates a value in util/sock/ch3usock.h
src/mpid/ch3/include/mpidimpl.hSwitch this to use the common debug code
src/mpid/ch3/include/mpidimpl.hThis does not belong here
src/mpid/ch3/include/mpidimpl.hThese should be defined only when these particular utility
packages are used. Best would be to keep these prototypes in the
related util/xxx directories, and either copy them into an include
directory used only for builds or add (yet another) include path
src/mpid/ch3/include/mpidimpl.hWhere should this go?
src/mpid/ch3/include/mpidimpl.hThis is a macro!
src/mpid/ch3/include/mpidimpl.hMove these prototypes into header files in the appropriate
util directories
src/mpid/ch3/include/mpidimpl.hMake these part of the channel support headers
src/mpid/ch3/include/mpidpost.hmpidpost.h is included by mpiimpl.h . However, mpiimpl.h should
refer only to the ADI3 prototypes and should never include prototypes
specific to any particular device. Factor the include files to maintain
better modularity by providing mpiimpl.h with only the definitions that it
needs
src/mpid/ch3/include/mpidpre.hThis header should contain only the definitions exported to the
mpiimpl.h level
src/mpid/ch3/include/mpidpre.hInclude here?
src/mpid/ch3/include/mpidpre.hWho defines this name
src/mpid/ch3/include/mpidpre.hThis ifndef test is a temp until mpidpre is cleaned of
all items that do not belong (e.g., all items not needed by the
top layers of MPICH)
src/mpid/ch3/include/mpidpre.hThe progress routines will be made into ch3-common definitions, not
channel specific. Channels that need more will need to piggy back or
otherwise override
src/mpid/ch3/include/mpidpkt.hThis appears to assume that sizeof(int) == 4 (or at least >= 4)
src/mpid/ch3/include/mpidpkt.hHaving predefined names makes it harder to add new message types,
such as different RMA types.
src/mpid/ch3/include/mpidpkt.hno sync eager
src/mpid/ch3/include/mpidpkt.hshould be stream put
src/mpid/ch3/include/mpidpkt.hUnused
src/mpid/ch3/include/mpidpkt.hExperimental for now
src/mpid/ch3/include/mpid_rma_shm.hWe currently only optimize a few predefined datatypes, which
have a direct C datatype mapping.
src/mpid/ch3/include/mpid_rma_shm.hwe need a configure check to define HAVE_WCHAR_T before
this can be enabled
src/mpid/ch3/include/mpid_rma_shm.hwe need a configure check to define HAVE_C_BOOL before
this can be enabled
src/mpid/ch3/include/mpid_rma_shm.hwe need a configure check to define HAVE_C_COMPLEX before
this can be enabled
src/mpid/ch3/include/mpid_rma_shm.hwe need a configure check to define HAVE_C_DOUPLE_COMPLEX
before this can be enabled
src/mpid/ch3/include/mpid_rma_shm.hwe need a configure check to define
HAVE_C_LONG_DOUPLE_COMPLEX before this can be enabled
src/mpid/common/shm/mpidu_shm_barrier.cthis is not a scalable algorithm because everyone is polling on the same cacheline
src/mpid/common/sched/mpidu_sched.hopen questions:
- Should the schedule hold a pointer to the nbc request and the nbc request
hold a pointer to the schedule? This could cause MT issues.
src/mpid/common/sched/mpidu_sched.cis this right when comm/datatype GC is used?
src/mpid/common/hcoll/hcoll_rte.cThe hcoll library needs to be updated to return
error codes. The progress function pointer right now
expects that the function returns void.
src/util/cvar/mpir_cvars.cany MT issues here?
src/util/wrappers/mpiu_sock_wrappers.hReplace wrapper funcs implemented as complex
macros with static inline funcs
src/util/wrappers/mpiu_sock_wrappers.hAdd more comments
src/util/wrappers/mpiu_sock_wrappers.hEnsure this header is only loaded when needed
src/util/wrappers/mpiu_sock_wrappers.hThis is cheating... Expand dynamically - we need to take
care of updating sock Handles if we expand dynamically.
src/util/wrappers/mpiu_sock_wrappers.hCheating - Expand dynamically
src/util/wrappers/mpiu_sock_wrappers.hTry not to use MPIR_Assert(). Utils should not depend
on a device impl of assert
src/util/procmap/local_proc.cthis is including a ch3 include file!
Implement these functions at the device level.
src/util/procmap/local_proc.crealloc doesn't guarantee that the allocated area will be
shrunk - so using realloc is not an appropriate strategy.
src/util/procmap/local_proc.cthis could/should be a list of ranks on the local node, which
should take up much less space on a typical thin(ish)-node system.
src/util/nodemap/build_nodemap.hThe patch is hacky because it assumes that seeing a
start node ID of 0 means a wrap around. This is not
necessarily true. A user-defined node list can, in theory,
use the node ID 0 without actually creating a wrap around.
The reason this patch still works in this case is because
Hydra creates a new node list starting from node ID 0 for
user-specified nodes during MPI_Comm_spawn{_multiple}. If
a different process manager searches for allocated nodes in
the user-specified list, this patch will break.
src/util/nodemap/build_nodemap.hthis routine can't handle the dynamic process case at this
time. this will require more support from the process manager.
src/util/nodemap/build_nodemap.hneed a better algorithm -- this one does O(N^2) strncmp()s!
src/util/logging/rlog/printrlog.cAdd a structured comment for the man page generate to
create the basic documentation on this routine, particularly
since this routine only prints a subset of information by default
src/util/logging/rlog/printrlog.cThis should also check for the GNU-standard --help, --usage,
and -h options.
src/util/logging/rlog/printrlog.cWhat is the default behavior with just an rlogfile?
src/util/logging/rlog/rlogtime.cThis name needs to be changed to ensure no conflicts with
user-defined globals
src/util/logging/rlog/TraceInput/trace_input.cImprove failure reporting
src/pm/util/pmiport.csimple_pmi should *never* start mpiexec with a bogus
interface name
src/pm/util/pmiserv.cWe may need to record some information, such as the
curPMIGroup, in the pState or pmiprocess entry
src/pm/util/pmiserv.cWhat should be the defaults for the spawned env?
Should the default be the env ov the spawner?
src/pm/util/pmiserv.cOtherwise, we have a problem
src/pm/util/pmiserv.ccall user-specified info handler, if any.
Unspecified info keys are ignored
src/pm/util/pmiserv.cMake this an optional output
src/pm/util/pmiserv.hWe need to specify a minimum length for both key and value.
The "business cards" used by the ch3 channel implementations for
sockets and for sockets+shared memory can exceed 128 characters.
src/pm/util/pmiserv.hThis is a temporary declaration (non-conforming name). Who added
this, and why?
src/pm/util/simple_pmiutil2.cDecide what role PMIU_printf should have (if any) and
select the appropriate MPIU routine
src/pm/util/simple_pmiutil2.cthe read length should be the size of the buffer,
not the size of the pointer to buffer
src/pm/util/simple_pmiutil2.cMake this an optional output
src/pm/util/cmnargs.cGet values from the environment first. Command line options
override the environment
src/pm/util/cmnargs.cMove this routine else where; perhaps a pmutil.c?
src/pm/util/cmnargs.chandle sign, invalid input
src/pm/util/cmnargs.chandle negative increments, and e not s + k incr
src/pm/util/ioloop.cOccassionally, data from stdout has been lost when the job is
exiting. I don't know whether data is being lost because the writer
is discarding it or the reader (mpiexec) is failing to finish reading from
all of the sockets before exiting.
src/pm/util/ioloop.can EINTR may also mean that a process has exited
(SIGCHILD). If all processes have exited, we may want to
exit
src/pm/util/process.ckill children
src/pm/util/process.ckill children
src/pm/util/process.cShould initialize with the
number of created processes
src/pm/util/process.cIndicate whether all processes have exited. Then
mpiexec programs can decide (probably based on a debugging flag)
what to do if they have not all exited.
src/pm/util/process.cThis is an ugly hack to ensure that the macros to manipulate the
cpu_set_t are defined - without these, you can't use the affinity routines
src/pm/util/process.cHow do we bind the threads when we don't have direct access to
them?
src/pm/util/rm.cneed path and external designation of file names
src/pm/util/env.cThis should be getGenv (so allocation/init in one place)
src/pm/hydra/mpl/include/mpl_thread_posix.hmutex creation and destruction should be implemented as routines
because there is no reason to use macros (these are not on the performance
critical path). Making these macros requires that any code that might use
these must load all of the pthread.h (or other thread library) support.
src/pm/hydra/mpl/include/mpl_thread_posix.husing constant initializer if available
src/pm/hydra/mpl/include/mpl_thread_posix.hconvert errors to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_trmem.hConsider an option of specifying __attribute__((malloc)) for
gcc - this lets gcc-style compilers know that the returned pointer
does not alias any pointer prior to the call.
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_thread_solaris.hconvert error to a MPL_THREAD_ERR value
src/pm/hydra/mpl/include/mpl_shm.hWhat if val is a non-null terminated string ?
src/pm/hydra/mpl/include/mpl_thread.hDefine other error codes. For now, any non-zero value is an error.
src/pm/hydra/mpl/include/mpl_iov.hHow is IOV_LIMIT chosen?
src/pm/hydra/mpl/src/thread/mpl_thread_solaris.cfaster allocation, or avoid it all together?
src/pm/hydra/mpl/src/thread/mpl_thread_solaris.cconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/src/thread/mpl_thread_posix.cfaster allocation, or avoid it all together?
src/pm/hydra/mpl/src/thread/mpl_thread_posix.cconvert error to an MPL_THREAD_ERR value
src/pm/hydra/mpl/src/env/mpl_env.cWe need to provide a way to signal this error
src/pm/hydra/mpl/src/dbg/mpl_dbg.cNeed to know how many process groups are known
src/pm/hydra/mpl/src/mem/mpl_trmem.cWe should use generalized parameter handling here
to allow use of the command line as well as environment
variables
src/pm/hydra/mpl/src/mem/mpl_trmem.cwhy do we skip the first few ints? [goodell@]
src/pm/hydra/mpl/src/mem/mpl_trmem.cNote head is no longer valid after MPL_trfree above
src/pm/hydra/mpl/src/str/mpl_str.cAssumes ASCII
src/pm/hydra/mpl/src/shm/mpl_shm_mmap.cPass (void **)shm_addr_ptr instead of (char **) shm_addr_ptr
since a util func should be generic
Currently not passing (void **) to reduce warning in nemesis
code which passes (char **) ptrs to be attached to a seg
src/pm/hydra/mpl/src/shm/mpl_shm_mmap.cClose local handle only when closing the shm handle
src/pm/hydra/tools/topo/hwloc/hwloc/include/hwloc/helper.hagregate nbobjs from different levels?
src/pm/hydra/tools/topo/hwloc/hwloc/include/hwloc/inlines.hagregate nbobjs from different levels?
src/pm/hydra/tools/topo/hwloc/hwloc/include/private/debug.huse __hwloc_attribute_format from private/private.h but that header cannot be used in plugins
src/pm/hydra/tools/topo/hwloc/hwloc/utils/hwloc/hwloc-bind.ccheck whether Windows execvp() passes INHERIT_PARENT_AFFINITY to CreateProcess()
because we need to propagate processor group affinity. However process-wide affinity
isn't supported with processor groups so far.
src/pm/hydra/tools/topo/hwloc/hwloc/utils/lstopo/lstopo-color.creturn -1 on error? tell the caller to use the default?
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-solaris.cif errno = ESRCH because some NUMA nodes are unavailable, we could reduce the matrix instead of ignoring
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-linux.cfind a way to identify the corresponding NUMA node and attach these objects there.
but it means we need to parse DeviceLocation=DIMM_B4 but these vary significantly
with the vendor, and it's hard to be 100% sure 'B' is second socket.
Examples at http://sourceforge.net/p/edac-utils/code/HEAD/tree/trunk/src/etc/labels.db
or https://github.com/grondo/edac-utils/blob/master/src/etc/labels.db
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-linux.cget the right DMI info of each machine
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-darwin.cassuming that L1i and L1d are shared the same way. Darwin
does not yet provide a way to know.
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology.cwith upcoming multiple levels of NUMA, we may have to report INCLUDED or CONTAINED here
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-opencl.ccannot happen in PCI devices?
src/pm/hydra/tools/topo/hwloc/hwloc/src/bitmap.cadd hwloc_bitmap_alloc_size() + hwloc_bitmap_init_allocated() to avoid malloc?
src/pm/hydra/tools/topo/hwloc/hwloc/src/topology-windows.ccould be non-contigous, rather return a mask from dwActiveProcessorMask?
src/pm/hydra/tools/bootstrap/external/ssh.cThese are never getting freed
src/pm/hydra/tools/bootstrap/external/pbs_wait.cWe rely on gettimeofday here. This needs to detect the
timer type available and use that. Probably more of an MPL
functionality than Hydra's.
src/pm/hydra/tools/bootstrap/utils/bscu_wait.cWe rely on gettimeofday here. This needs to detect the
timer type available and use that. Probably more of an MPL
functionality than Hydra's.
src/pm/hydra/pm/pmiserv/pmip_pmi_v1.cWe cannot abort when we are not able to send data
downstream. The upper layer needs to handle this based on
whether we want to abort or not.
src/pm/hydra/pm/pmiserv/pmip_cb.cThis should really be a "FIXME" for the client, since
there's not much we can do on the server side.

We initialize to whatever PMI version we detect while reading
the PMI command, instead of relying on what the init command
gave us. This part of the code should not know anything about
PMI-1 vs. PMI-2. But the simple PMI client-side code in MPICH
is so hacked up, that commands can arrive out-of-order and this
is necessary. This was discussed in the group and we felt that
it is unsafe to change the order of the PMI command arrival in
the client code (even if we are really correcting it), since
other PMs might rely on the "incorrect order of commands".
src/pm/hydra/pm/pmiserv/pmip_cb.cThis dual memcpy is crazy and needs to be
fixed. Single memcpy should be possible, but we need to be
a bit careful not to corrupt the buffer.
src/pm/hydra/pm/pmiserv/pmip_cb.cThis code needs to change from sending the
SIGUSR1 signal to a PMI-2 notification message.
src/pm/hydra/pm/pmiserv/pmip_cb.cthese envvars should be set by MPICH instead. See #2360
src/pm/hydra/pm/pmiserv/pmip_cb.cThis code needs to change from sending the signal to
a PMI-2 notification message.
src/pm/hydra/pm/pmiserv/pmip_pmi_v2.cWe cannot abort when we are not able to send data
downstream. The upper layer needs to handle this based on
whether we want to abort or not.
src/pm/hydra/pm/pmiserv/pmiserv_cb.cIf the list of dead processes does not fit
inside a single value length, set it as a
multi-line value.
src/pm/hydra/pm/pmiserv/pmiserv_cb.cThis was changed from a sorted list where sequential
numbers could be compacted to an expanded list where they
couldn't. Obviously, this isn't sustainable on the PMI
side, but on the MPI side, it's necessary (see the
definition of MPI_COMM_FAILURE_ACK). In a future version of
PMI where we can pass around things other than strings,
this should improve.
src/pm/gforker/mpiexec.cThe following should be a single routine in pmiport
src/pm/gforker/mpiexec.cThis should be part of the PMI initialization in the clients
src/pm/remshell/mpiexec.cThe singleton code goes here
src/pm/remshell/mpiexec.cThis should be part of the PMI initialization in the clients
src/pmi/simple/simple_pmi.cWhy is setvbuf commented out?
src/pmi/simple/simple_pmi.cWhat if the output should be fully buffered (directed to file)?
unbuffered (user explicitly set?)
src/pmi/simple/simple_pmi.cWhy does this depend on their being a port???
src/pmi/simple/simple_pmi.cWhat is this for?
src/pmi/simple/simple_pmi.cThis should use a cmd/response rather than a expecting the
server to set a value in this and only this case
src/pmi/simple/simple_pmi.cAnd it most ceratainly should not happen *before* the
initialization handshake
src/pmi/simple/simple_pmi.cThis is something that the PM should tell the process,
rather than deliver it through the environment
src/pmi/simple/simple_pmi.cMy name should be cached rather than re-acquired, as it is
unchanging (after singleton init)
src/pmi/simple/simple_pmi.cWe need to support a distinct kvsname for each
process group
src/pmi/simple/simple_pmi.c(protocol design flaw): command line arguments
may contain both = and <space> (and even tab!).
src/pmi/simple/simple_pmi.cThis mixes init with get maxes
src/pmi/simple/simple_pmi.cThis is an example of an incorrect fix - writeline can change
the second argument in some cases, and that will break the const'ness
of request. Instead, writeline should take a const item and return
an error in the case in which it currently truncates the data.
src/pmi/simple/simple_pmi.cUse a valid hostname
src/pmi/simple/simple_pmi.cWe need to support a distinct kvsname for each
process group
src/pmi/simple/simple_pmi.cCheck for valid integer after :
src/pmi/simple/simple_pmiutil.cDecide what role PMIU_printf should have (if any) and
select the appropriate MPIU routine
src/pmi/simple/simple_pmiutil.cMake this an optional output
src/pmi/pmi2/simple/simple2pmi.cWhy is setvbuf commented out?
src/pmi/pmi2/simple/simple2pmi.cWhat if the output should be fully buffered (directed to file)?
unbuffered (user explicitly set?)
src/pmi/pmi2/simple/simple2pmi.coverall need a better interface for building commands!
Need to be able to append commands, and to easily accept integer
valued arguments. Memory management should stay completely out
of mind when writing a new PMI command impl like this!
src/pmi/pmi2/simple/simple2pmi.cwe are somehow still leaking some of this memory
src/pmi/pmi2/simple/simple2pmi.ccasts are here to suppress legitimate constness warnings
src/pmi/pmi2/simple/simple2pmi.cCheck for valid integer after :
src/pmi/pmi2/simple/simple_pmiutil.cDecide what role PMI2U_printf should have (if any) and
select the appropriate PMI2U routine
src/pmi/pmi2/simple/simple_pmiutil.cMake this an optional output
src/nameserv/file/file_nameserv.cDetermine if the directory exists before trying to create it
src/nameserv/file/file_nameserv.cAn error. Ignore most ?
For example, ignore EEXIST?
src/nameserv/file/file_nameserv.cThis should use internationalization calls