fsalem
commented
4 years ago Thanks Jan @cuveland for your comments. I updated the code to consider most of your comments. Please find below my comment to each of your points:
-
MPI dependency: The MPI barrier was only used to detect the initial clock drift and to sync the start time of the first scheduler interval. We plan to replace the MPI with the corresponding Libfabric collective operations but they are not yet implemented for the OmniPath and IB Libfabric providers. In order to remove the MPI code, we replaced the barrier with a temporary send/recv Libfabric implementation. Once the collective operations are implemented, we will update this part to be more efficient. Done 👍
-
lib/fles_ipc/System.cpp: It gave me compilation error due to unknown 'errno'. We reverted the original code and added #include
instead . Hopefully, this is okay. -
"libfabric only" to the parameter description: Done 👍
-
drop-process-ts: The “drop-process-ts” param is the mapping of “drop_” variable in the TimesliceBuilder in both RDMA and Libfabric implementation. If this part of code is only for debugging purposes, please let me know to remove this param.
-
Logging params: When DFS logs are needed for debugging, this two params are to store the DFS log files (factor of GBs) in a specific directory. We could remove the directory flag at all and the log files will be generated in the same directory of the binary file. For now, I added the “scheduler-“ prefix for both params.
-
Console Output: All cout & printf statements are removed. Done 👍
Please let me know your comments about the latest changes.
cuveland
It all looks very neat and tidy. Thanks for the great job!
I have no comments on the vast majority of changes within lib/fles_libfabric. This falls within your area of responsibility, and I did not follow every line.
What is new to me is the dependence on MPI. I cannot yet foresee all the implications of this global change. I think it would make sense to continue to support systems without MPI at least to the same extent as before. I have nothing against it in principle, but can we make it optional in the build process for now? It could be coupled to something like CMake's USE_LIBFABRIC.
A few more questions about MPI: What is MPI actually used for? Only for the barrier? The use of MPI changes the life cycle of the processes significantly. What happens when a process ends? Can the system still shut down correctly, including the handling of shared memories? How can the system survive the failure of a computing node when using MPI?
In lib/fles_ipc/System.cpp, I think you might have introduced an error. As the documentation states (see https://www.man7.org/linux/man-pages/man2/gethostname.2.html) for gethostname: "On success, zero is returned. On error, -1 is returned, and errno is set appropriately." This is also true for getdomainname.
The additional flesnet parameters introduced are for the most part well structured.
The "scheduler-*" parameters obviously target the behaviour of the new scheduler. To prevent any confusion, I think it would make sense to add a remark like "libfabric only" to the parameter description.
In the case of "drop-process-ts" I am not sure how this is to be used. Is this also libfabric-only? If so, we should definitely label it as such. Or do you envision that this would also make sense to use in the other transports?
I'm afraid that the two parameters "log-directory" and "enable-logging" could lead to confusion. I would suggest leaving them out. We already have the global parameters "log-level" and "log-file", which refer to the global log system, which can also be redirected to syslog, for example, or later possibly to another monitoring system. If at all possible, I would suggest to use this existing system here as well. As an idea: instead of several files, one could, for example, simply differentiate using an additional column, which is then filtered on in the evaluation.
About logging and console output... slowly moving towards a scalable production system we have to be careful here. I think in general we should avoid using any console output that bypasses the log system.
In lib/fleslibfabric/Provider.cpp, fprintf(stdout, ...) is used extensively for a debug dump. The "printf" family of functions is not used anywhere else in the flesnet source code, and also std::cout/std::cerr is used only in rare circumstances. Could you use L(trace) or something similar instead here?
I think with these few changes, we should be able to merge the branches successfully in a very short time.