From fced2b623f0b801c3f8d8cfa346780811d859dfd Mon Sep 17 00:00:00 2001 From: Mark Abraham Date: Tue, 24 Oct 2017 21:59:45 +0200 Subject: [PATCH] Fix and update hw_info Stopped using typedef struct (so later we can put a vector into the struct). Managed the memory using a unique_ptr, and made the interface reflect that it is a file static, rather than something that is owned by e.g. the runner. Amended docs to clarify the sense of "global." Change-Id: I1ce9bc42e03668498051b59aaeeb9e50a9f6f762 --- src/gromacs/ewald/tests/testhardwarecontexts.cpp | 2 +- src/gromacs/hardware/CMakeLists.txt | 2 +- src/gromacs/hardware/detecthardware.cpp | 30 +++++++++++++----------- src/gromacs/hardware/detecthardware.h | 4 ++-- src/gromacs/hardware/gpu_hw_info.cpp | 2 +- src/gromacs/hardware/hw_info.h | 4 ++-- src/programs/mdrun/runner.cpp | 2 +- 7 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/gromacs/ewald/tests/testhardwarecontexts.cpp b/src/gromacs/ewald/tests/testhardwarecontexts.cpp index 08d6fa2bea..7bba127012 100644 --- a/src/gromacs/ewald/tests/testhardwarecontexts.cpp +++ b/src/gromacs/ewald/tests/testhardwarecontexts.cpp @@ -120,7 +120,7 @@ void PmeTestEnvironment::SetUp() void PmeTestEnvironment::TearDown() { - gmx_hardware_info_free(hardwareInfo_); + gmx_hardware_info_free(); } } diff --git a/src/gromacs/hardware/CMakeLists.txt b/src/gromacs/hardware/CMakeLists.txt index d825aaaaa6..c758f54374 100644 --- a/src/gromacs/hardware/CMakeLists.txt +++ b/src/gromacs/hardware/CMakeLists.txt @@ -35,8 +35,8 @@ gmx_add_libgromacs_sources( cpuinfo.cpp detecthardware.cpp - hardwaretopology.cpp gpu_hw_info.cpp + hardwaretopology.cpp printhardware.cpp ) diff --git a/src/gromacs/hardware/detecthardware.cpp b/src/gromacs/hardware/detecthardware.cpp index c63b73dba8..af9fd2edc6 100644 --- a/src/gromacs/hardware/detecthardware.cpp +++ b/src/gromacs/hardware/detecthardware.cpp @@ -44,12 +44,14 @@ #include #include +#include #include #include #include #include "thread_mpi/threads.h" +#include "gromacs/compat/make_unique.h" #include "gromacs/gpu_utils/gpu_utils.h" #include "gromacs/hardware/cpuinfo.h" #include "gromacs/hardware/hardwaretopology.h" @@ -91,12 +93,18 @@ namespace gmx //! Constant used to help minimize preprocessed code static const bool bGPUBinary = GMX_GPU != GMX_GPU_NONE; -//! The globally shared hwinfo structure -static gmx_hw_info_t *hwinfo_g; +/*! \brief The hwinfo structure (common to all threads in this process). + * + * \todo This should become a shared_ptr owned by e.g. Mdrunner::runner() + * that is shared across any threads as needed (e.g. for thread-MPI). That + * offers about the same run time performance as we get here, and avoids a + * lot of custom code. + */ +static std::unique_ptr hwinfo_g; //! A reference counter for the hwinfo structure -static int n_hwinfo = 0; +static int n_hwinfo = 0; //! A lock to protect the hwinfo structure -static tMPI_Thread_mutex_t hw_info_lock = TMPI_THREAD_MUTEX_INITIALIZER; +static tMPI_Thread_mutex_t hw_info_lock = TMPI_THREAD_MUTEX_INITIALIZER; //! Detect GPUs, if that makes sense to attempt. static void gmx_detect_gpus(const gmx::MDLogger &mdlog, const t_commrec *cr) @@ -451,7 +459,6 @@ hardwareTopologyDoubleCheckDetection(const gmx::MDLogger gmx_unused &mdl #endif } - gmx_hw_info_t *gmx_detect_hardware(const gmx::MDLogger &mdlog, const t_commrec *cr) { int ret; @@ -466,7 +473,7 @@ gmx_hw_info_t *gmx_detect_hardware(const gmx::MDLogger &mdlog, const t_commrec * /* only initialize the hwinfo structure if it is not already initalized */ if (n_hwinfo == 0) { - snew(hwinfo_g, 1); + hwinfo_g = compat::make_unique(); hwinfo_g->cpuInfo = new gmx::CpuInfo(gmx::CpuInfo::detect()); @@ -499,7 +506,7 @@ gmx_hw_info_t *gmx_detect_hardware(const gmx::MDLogger &mdlog, const t_commrec * gmx_fatal(FARGS, "Error unlocking hwinfo mutex: %s", strerror(errno)); } - return hwinfo_g; + return hwinfo_g.get(); } bool compatibleGpusFound(const gmx_gpu_info_t &gpu_info) @@ -507,7 +514,7 @@ bool compatibleGpusFound(const gmx_gpu_info_t &gpu_info) return gpu_info.n_dev_compatible > 0; } -void gmx_hardware_info_free(gmx_hw_info_t *hwinfo) +void gmx_hardware_info_free() { int ret; @@ -521,11 +528,6 @@ void gmx_hardware_info_free(gmx_hw_info_t *hwinfo) n_hwinfo--; - if (hwinfo != hwinfo_g) - { - gmx_incons("hwinfo < hwinfo_g"); - } - if (n_hwinfo < 0) { gmx_incons("n_hwinfo < 0"); @@ -536,7 +538,7 @@ void gmx_hardware_info_free(gmx_hw_info_t *hwinfo) delete hwinfo_g->cpuInfo; delete hwinfo_g->hardwareTopology; free_gpu_info(&hwinfo_g->gpu_info); - sfree(hwinfo_g); + hwinfo_g.reset(); } ret = tMPI_Thread_mutex_unlock(&hw_info_lock); diff --git a/src/gromacs/hardware/detecthardware.h b/src/gromacs/hardware/detecthardware.h index 3990ce78c7..331aa73bf1 100644 --- a/src/gromacs/hardware/detecthardware.h +++ b/src/gromacs/hardware/detecthardware.h @@ -57,13 +57,13 @@ class MDLogger; * it. It will run a preamble before executing cpu and hardware checks, and * then run consistency checks afterwards. The results will also be made * available on all nodes. - * Caller is responsible for freeing this pointer. + * Caller is responsible for calling gmx_hardware_info_free() when finished. */ gmx_hw_info_t *gmx_detect_hardware(const gmx::MDLogger &mdlog, const t_commrec *cr); /*! \brief Free the hwinfo structure */ -void gmx_hardware_info_free(gmx_hw_info_t *hwinfo); +void gmx_hardware_info_free(); //! Return whether compatible GPUs were found. bool compatibleGpusFound(const gmx_gpu_info_t &gpu_info); diff --git a/src/gromacs/hardware/gpu_hw_info.cpp b/src/gromacs/hardware/gpu_hw_info.cpp index 38749a93f0..6d25a62e67 100644 --- a/src/gromacs/hardware/gpu_hw_info.cpp +++ b/src/gromacs/hardware/gpu_hw_info.cpp @@ -47,5 +47,5 @@ /* Names of the GPU detection/check results (see e_gpu_detect_res_t in hw_info.h). */ const char * const gpu_detect_res_str[egpuNR] = { - "compatible", "inexistent", "incompatible", "insane" + "compatible", "nonexistent", "incompatible", "insane" }; diff --git a/src/gromacs/hardware/hw_info.h b/src/gromacs/hardware/hw_info.h index 401421b521..7e08e63f3f 100644 --- a/src/gromacs/hardware/hw_info.h +++ b/src/gromacs/hardware/hw_info.h @@ -50,7 +50,7 @@ class HardwareTopology; * It is initialized by gmx_detect_hardware(). * NOTE: this structure may only contain structures that are globally valid * (i.e. must be able to be shared among all threads) */ -typedef struct gmx_hw_info_t +struct gmx_hw_info_t { /* Data for our local physical node */ struct gmx_gpu_info_t gpu_info; /* Information about GPUs detected in the system */ @@ -78,7 +78,7 @@ typedef struct gmx_hw_info_t int simd_suggest_max; /* Highest SIMD instruction set supported by at least one rank */ gmx_bool bIdenticalGPUs; /* TRUE if all ranks have the same type(s) and order of GPUs */ -} gmx_hw_info_t; +}; /* The options for the thread affinity setting, default: auto */ diff --git a/src/programs/mdrun/runner.cpp b/src/programs/mdrun/runner.cpp index 7d617b97fd..940079ff28 100644 --- a/src/programs/mdrun/runner.cpp +++ b/src/programs/mdrun/runner.cpp @@ -1199,7 +1199,7 @@ int Mdrunner::mdrunner() free_membed(membed); } - gmx_hardware_info_free(hwinfo); + gmx_hardware_info_free(); /* Does what it says */ print_date_and_time(fplog, cr->nodeid, "Finished mdrun", gmx_gettime()); -- 2.11.4.GIT