From 227b3976bae5831c20d9f579f53708df6529492f Mon Sep 17 00:00:00 2001 From: Mattijs Janssens Date: Tue, 2 Nov 2021 14:26:30 +0000 Subject: [PATCH] BUG: collated: threaded writing accesses out-of-scope. Fixes #2257. --- etc/caseDicts/annotated/decomposeParDict | 8 ++------ etc/controlDict | 4 ++++ .../decomposedBlockData/decomposedBlockData.C | 3 ++- .../decomposedBlockData/decomposedBlockData.H | 2 +- .../decomposedBlockDataHeader.C | 6 +++--- src/OpenFOAM/db/IOstreams/Pstreams/UPstream.C | 8 ++++++++ src/OpenFOAM/global/argList/argList.C | 2 +- .../collatedFileOperation/OFstreamCollator.C | 12 ++++++------ .../collatedFileOperation/OFstreamCollator.H | 14 ++++++-------- .../collatedFileOperation/collatedFileOperation.C | 9 ++++++--- .../collatedFileOperation/collatedFileOperation.H | 9 +++++++-- .../threadedCollatedOFstream.C | 2 +- src/Pstream/mpi/UPstream.C | 10 +++++++++- .../cellCellStencil/inverseDistance/waveMethod.C | 4 ++-- .../implicitAMI/system/decomposeParDict | 12 ++++++++++++ 15 files changed, 70 insertions(+), 35 deletions(-) diff --git a/etc/caseDicts/annotated/decomposeParDict b/etc/caseDicts/annotated/decomposeParDict index 5fcea6439e..9a685736cc 100644 --- a/etc/caseDicts/annotated/decomposeParDict +++ b/etc/caseDicts/annotated/decomposeParDict @@ -208,9 +208,7 @@ constraints patches { //- Keep owner and neighbour on same processor for faces in patches - // (only makes sense for cyclic patches. Not suitable for e.g. - // cyclicAMI since these are not coupled on the patch level. Use - // singleProcessorFaceSets for those) + // (only makes sense for cyclic patches and cyclicAMI) type preservePatches; patches (".*"); enabled false; @@ -271,9 +269,7 @@ constraints // preserveFaceZones (heater solid1 solid3); //- Keep owner and neighbour on same processor for faces in patches: -// (makes sense only for cyclic patches. Not suitable for e.g. cyclicAMI -// since these are not coupled on the patch level. Use -// singleProcessorFaceSets for those) +// (only makes sense for cyclic patches and cyclicAMI) //preservePatches (cyclic_half0 cyclic_half1); //- Keep all of faceSet on a single processor. This puts all cells diff --git a/etc/controlDict b/etc/controlDict index f039ec1736..fa8474e573 100644 --- a/etc/controlDict +++ b/etc/controlDict @@ -106,6 +106,10 @@ OptimisationSwitches //- collated: thread buffer size for queued file writes. // If set to 0 or not sufficient for the file size, threading is not used. + // A special setting is a negative value which assumes the buffer + // (sized with magnitude of value) is large enough to hold all + // outstanding writes so will not try to initialise the Pstream with + // threading support. // Default: 1e9 maxThreadFileBufferSize 0; diff --git a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.C b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.C index ee795672bf..ac00d96cca 100644 --- a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.C +++ b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.C @@ -984,7 +984,8 @@ bool Foam::decomposedBlockData::writeData(Ostream& os) const io.headerClassName(), io.note(), masterLocation, - name() + name(), + dictionary() ); } diff --git a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.H b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.H index 6e1f8e5df8..247a7006f2 100644 --- a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.H +++ b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockData.H @@ -213,7 +213,7 @@ public: const string& note, const fileName& location, const word& objectName, - const dictionary* extraEntries = nullptr + const dictionary& extraEntries ); //- Helper: write FoamFile IOobject header diff --git a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockDataHeader.C b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockDataHeader.C index 51313bc972..e4bdbf4732 100644 --- a/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockDataHeader.C +++ b/src/OpenFOAM/db/IOobjects/decomposedBlockData/decomposedBlockDataHeader.C @@ -141,7 +141,7 @@ void Foam::decomposedBlockData::writeHeader const string& note, const fileName& location, const word& objectName, - const dictionary* extraEntries + const dictionary& extraEntries ) { if (IOobject::bannerEnabled()) @@ -161,9 +161,9 @@ void Foam::decomposedBlockData::writeHeader objectName ); - if (extraEntries) + if (!extraEntries.empty()) { - extraEntries->writeEntries(os); + extraEntries.writeEntries(os); } os.endBlock(); diff --git a/src/OpenFOAM/db/IOstreams/Pstreams/UPstream.C b/src/OpenFOAM/db/IOstreams/Pstreams/UPstream.C index b87f219426..936a50db55 100644 --- a/src/OpenFOAM/db/IOstreams/Pstreams/UPstream.C +++ b/src/OpenFOAM/db/IOstreams/Pstreams/UPstream.C @@ -93,6 +93,14 @@ void Foam::UPstream::setParRun(const label nProcs, const bool haveThreads) Pout.prefix() = '[' + name(myProcNo(comm)) + "] "; Perr.prefix() = '[' + name(myProcNo(comm)) + "] "; } + + if (debug) + { + Pout<< "UPstream::setParRun :" + << " nProcs:" << nProcs + << " haveThreads:" << haveThreads + << endl; + } } diff --git a/src/OpenFOAM/global/argList/argList.C b/src/OpenFOAM/global/argList/argList.C index 4199abcdaa..4a5d6e2537 100644 --- a/src/OpenFOAM/global/argList/argList.C +++ b/src/OpenFOAM/global/argList/argList.C @@ -778,7 +778,7 @@ Foam::argList::argList } // Detect any parallel options - bool needsThread = fileOperations::fileOperationInitialise::New + const bool needsThread = fileOperations::fileOperationInitialise::New ( handlerType, argc, diff --git a/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.C b/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.C index b5a3ced736..1fa2baca74 100644 --- a/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.C +++ b/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.C @@ -52,7 +52,7 @@ bool Foam::OFstreamCollator::writeFile const PtrList>& slaveData, // optional slave data IOstreamOption streamOpt, const bool append, - const dictionary* headerEntriesPtr + const dictionary& headerEntries ) { if (debug) @@ -96,7 +96,7 @@ bool Foam::OFstreamCollator::writeFile "", // note "", // location (leave empty instead inaccurate) fName.name(), // object name - headerEntriesPtr + headerEntries ); } } @@ -350,7 +350,7 @@ bool Foam::OFstreamCollator::write IOstreamOption streamOpt, const bool append, const bool useThread, - const dictionary* headerEntriesPtr + const dictionary& headerEntries ) { // Determine (on master) sizes to receive. Note: do NOT use thread @@ -389,7 +389,7 @@ bool Foam::OFstreamCollator::write dummySlaveData, streamOpt, append, - headerEntriesPtr + headerEntries ); } else if (totalSize <= maxBufferSize_) @@ -427,7 +427,7 @@ bool Foam::OFstreamCollator::write recvSizes, streamOpt, append, - headerEntriesPtr + headerEntries ) ); writeData& fileAndData = fileAndDataPtr(); @@ -552,7 +552,7 @@ bool Foam::OFstreamCollator::write recvSizes, streamOpt, append, - headerEntriesPtr + headerEntries ) ); diff --git a/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.H b/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.H index 1146cff2b4..dfcfdae236 100644 --- a/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.H +++ b/src/OpenFOAM/global/fileOperations/collatedFileOperation/OFstreamCollator.H @@ -57,15 +57,13 @@ SourceFiles #include "labelList.H" #include "FIFOStack.H" #include "SubList.H" +#include "dictionary.H" // * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * // namespace Foam { -// Forward Declarations -class dictionary; - /*---------------------------------------------------------------------------*\ Class OFstreamCollator Declaration \*---------------------------------------------------------------------------*/ @@ -84,7 +82,7 @@ class OFstreamCollator PtrList> slaveData_; const IOstreamOption streamOpt_; const bool append_; - const dictionary* headerEntries_; + const dictionary headerEntries_; writeData ( @@ -95,7 +93,7 @@ class OFstreamCollator const labelList& sizes, IOstreamOption streamOpt, const bool append, - const dictionary* headerEntriesPtr = nullptr + const dictionary& headerEntries ) : comm_(comm), @@ -106,7 +104,7 @@ class OFstreamCollator slaveData_(), streamOpt_(streamOpt), append_(append), - headerEntries_(headerEntriesPtr) + headerEntries_(headerEntries) {} //- The (approximate) size of master + any optional slave data @@ -160,7 +158,7 @@ class OFstreamCollator const PtrList>& slaveData, IOstreamOption streamOpt, const bool append, - const dictionary* headerEntriesPtr + const dictionary& headerEntries ); //- Write all files in stack @@ -204,7 +202,7 @@ public: IOstreamOption streamOpt, const bool append, const bool useThread = true, - const dictionary* headerEntriesPtr = nullptr + const dictionary& headerEntries = dictionary::null ); //- Wait for all thread actions to have finished diff --git a/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.C b/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.C index 85c1536cc4..3464f119aa 100644 --- a/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.C +++ b/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.C @@ -274,7 +274,7 @@ Foam::fileOperations::collatedFileOperation::collatedFileOperation false ), myComm_(comm_), - writer_(maxThreadFileBufferSize, comm_), + writer_(mag(maxThreadFileBufferSize), comm_), nProcs_(Pstream::nProcs()), ioRanks_(ioRanks()) { @@ -295,7 +295,7 @@ Foam::fileOperations::collatedFileOperation::collatedFileOperation : masterUncollatedFileOperation(comm, false), myComm_(-1), - writer_(maxThreadFileBufferSize, comm), + writer_(mag(maxThreadFileBufferSize), comm), nProcs_(Pstream::nProcs()), ioRanks_(ioRanks) { @@ -310,6 +310,9 @@ Foam::fileOperations::collatedFileOperation::collatedFileOperation Foam::fileOperations::collatedFileOperation::~collatedFileOperation() { + // Wait for any outstanding file operations + flush(); + if (myComm_ != -1 && myComm_ != UPstream::worldComm) { UPstream::freeCommunicator(myComm_); @@ -460,7 +463,7 @@ bool Foam::fileOperations::collatedFileOperation::writeObject { // Re-check static maxThreadFileBufferSize variable to see // if needs to use threading - const bool useThread = (maxThreadFileBufferSize > 0); + const bool useThread = (maxThreadFileBufferSize != 0); if (debug) { diff --git a/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.H b/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.H index 6d635ccb18..23d5335646 100644 --- a/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.H +++ b/src/OpenFOAM/global/fileOperations/collatedFileOperation/collatedFileOperation.H @@ -6,7 +6,7 @@ \\/ M anipulation | ------------------------------------------------------------------------------- Copyright (C) 2017 OpenFOAM Foundation - Copyright (C) 2019-2020 OpenCFD Ltd. + Copyright (C) 2019-2021 OpenCFD Ltd. ------------------------------------------------------------------------------- License This file is part of OpenFOAM. @@ -31,7 +31,12 @@ Description Version of masterUncollatedFileOperation that collates regIOobjects into a container in the processors/ subdirectory. - Uses threading if maxThreadFileBufferSize > 0. + Uses threading if maxThreadFileBufferSize != 0. + > 0 : Can use mpi inside thread to collect data if buffer is not + large enough. Does need full thread support inside MPI. + + < 0 : special : -maxThreadFileBufferSize is guaranteed large enough + for all writing. Initialises MPI without thread support. See also masterUncollatedFileOperation diff --git a/src/OpenFOAM/global/fileOperations/collatedFileOperation/threadedCollatedOFstream.C b/src/OpenFOAM/global/fileOperations/collatedFileOperation/threadedCollatedOFstream.C index bc62facfd2..c00ab58213 100644 --- a/src/OpenFOAM/global/fileOperations/collatedFileOperation/threadedCollatedOFstream.C +++ b/src/OpenFOAM/global/fileOperations/collatedFileOperation/threadedCollatedOFstream.C @@ -61,7 +61,7 @@ Foam::threadedCollatedOFstream::~threadedCollatedOFstream() IOstreamOption(IOstream::BINARY, version(), compression_), false, // append=false useThread_, - &headerEntries_ + headerEntries_ ); } diff --git a/src/Pstream/mpi/UPstream.C b/src/Pstream/mpi/UPstream.C index edd9315e47..f36485840b 100644 --- a/src/Pstream/mpi/UPstream.C +++ b/src/Pstream/mpi/UPstream.C @@ -279,7 +279,15 @@ bool Foam::UPstream::init(int& argc, char**& argv, const bool needsThread) if (debug) { - Pout<< "UPstream::init : procs:" << numprocs + Pout<< "UPstream::init :" + << " thread-support : wanted:" << needsThread + << " obtained:" + << ( + provided_thread_support == MPI_THREAD_MULTIPLE + ? "MPI_THREAD_MULTIPLE" + : "MPI_THREAD_SINGLE" + ) + << " procs:" << numprocs << " rank:" << myRank << " world:" << world << endl; } diff --git a/src/overset/cellCellStencil/inverseDistance/waveMethod.C b/src/overset/cellCellStencil/inverseDistance/waveMethod.C index ef54ca9de4..e3dd8a9c55 100644 --- a/src/overset/cellCellStencil/inverseDistance/waveMethod.C +++ b/src/overset/cellCellStencil/inverseDistance/waveMethod.C @@ -5,7 +5,7 @@ \\ / A nd | www.openfoam.com \\/ M anipulation | ------------------------------------------------------------------------------- - Copyright (C) 2017-2020 OpenCFD Ltd. + Copyright (C) 2017-2021 OpenCFD Ltd. ------------------------------------------------------------------------------- License This file is part of OpenFOAM. @@ -129,7 +129,7 @@ void Foam::waveMethod::calculate changedFacesInfo, faceData, cellData, - src.globalData().nTotalCells(), // max iterations + src.globalData().nTotalCells()+1, // max iterations td ); } diff --git a/tutorials/basic/laplacianFoam/implicitAMI/system/decomposeParDict b/tutorials/basic/laplacianFoam/implicitAMI/system/decomposeParDict index a1ee695090..fa80348b2e 100644 --- a/tutorials/basic/laplacianFoam/implicitAMI/system/decomposeParDict +++ b/tutorials/basic/laplacianFoam/implicitAMI/system/decomposeParDict @@ -22,4 +22,16 @@ numberOfSubdomains 2; method hierarchical; n (2 1 1); +constraints +{ + patches + { + //- Keep owner and neighbour on same processor for faces in patches + // (only makes sense for cyclic patches and cyclicAMI) + type preservePatches; + patches (".*"); + } +} + + // ************************************************************************* //