Protected mutex locking with RAII. #30501

ericcano · 2020-07-02T13:25:37Z

In case some of the cudaCheck() calls threw (line 596), the mutex would have been left locked.

PR description:

This PR fixes a possible leftover locked mutex in case of exception.

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

In case some of the cudaCheck() calls threw (line 596), the mutex would have been left locked.

cmsbuild · 2020-07-02T13:26:04Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-02T13:30:23Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30501/16690

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HeterogeneousCore/CUDAUtilities/src/CachingDeviceAllocator.h modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936

cmsbuild · 2020-07-02T13:30:46Z

A new Pull Request was created by @ericcano (Eric Cano) for master.

It involves the following packages:

HeterogeneousCore/CUDAUtilities

@makortel, @cmsbuild, @fwyzard can you please review it and eventually sign? Thanks.
@makortel, @rovere this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2020-07-02T13:39:32Z

@makortel what do you think ?

In general I prefer to use std::lock_guard which should be slightly more lightweight; here that approach would require adding a couple of extra scopes.

makortel · 2020-07-02T15:18:46Z

In general I agree on std::lock_guard, on the other hand with std::unique_lock here I like that the changes are minimal. The caching allocator code anyway has much more profound performance issues than the lock type. So if there no strong opinions otherwise I'd go with unique_lock.

makortel

Thanks, I never really liked the explicit locking of the mutex anyway. For CachingDeviceAllocator we try to document inline the changes CMS has done to the CachingDeviceAllocator from cub.

Could you do similar changes to CachingHostAllocator as well (comment decoration are not needed there because it is all CMS code, even if cloned from the CachingDeviceAllocator).

HeterogeneousCore/CUDAUtilities/src/CachingDeviceAllocator.h

Co-authored-by: Matti Kortelainen <matti.kortelainen@cern.ch>

cmsbuild · 2020-07-02T16:45:47Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-02T16:50:30Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30501/16708

This PR adds an extra 16KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HeterogeneousCore/CUDAUtilities/src/CachingDeviceAllocator.h modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936

cmsbuild · 2020-07-02T16:50:51Z

Pull request #30501 was updated. @makortel, @cmsbuild, @fwyzard can you please check and sign again.

ericcano · 2020-07-02T16:53:35Z

Could you do similar changes to CachingHostAllocator as well (comment decoration are not needed there because it is all CMS code, even if cloned from the CachingDeviceAllocator).

Sure, I will add that to the PR.

In case some of the cudaCheck() calls threw (line 596), the mutex would have been left locked. (Ported from CachingDeviceAllocator). Also merged instanciation and locking where they appeared bach-to-back in CachingDeviceAllocator.

cmsbuild · 2020-07-02T17:37:58Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-02T17:42:58Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30501/16713

This PR adds an extra 20KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File HeterogeneousCore/CUDAUtilities/src/CachingDeviceAllocator.h modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936
- File HeterogeneousCore/CUDAUtilities/src/CachingHostAllocator.h modified in PR(s): [RFC] Test clang-tidy --checks modernize-use-auto #29936

cmsbuild · 2020-07-02T17:43:23Z

Pull request #30501 was updated. @makortel, @cmsbuild, @fwyzard can you please check and sign again.

makortel · 2020-07-02T17:53:13Z

@cmsbuild, please test

Thanks!

cmsbuild · 2020-07-02T17:53:33Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-02-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7622/console Started: 2020/07/02 19:56

cmsbuild · 2020-07-02T19:10:19Z

+1
Tested at: 2a042ca
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ff47d/7622/summary.html
CMSSW: CMSSW_11_2_X_2020-07-02-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-02T19:10:22Z

Comparison job queued.

cmsbuild · 2020-07-02T21:06:07Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-0ff47d/7622/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2784120
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2784069
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 154 log files, 17 edm output root files, 37 DQM output files

makortel · 2020-07-03T00:08:29Z

+1

cmsbuild · 2020-07-03T00:08:55Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-07-04T08:14:30Z

+1

Protected mutex locking with RAII.

082d9cf

In case some of the cudaCheck() calls threw (line 596), the mutex would have been left locked.

cmsbuild added this to the CMSSW_11_2_X milestone Jul 2, 2020

cmsbuild added code-checks-pending comparison-pending heterogeneous-pending orp-pending pending-signatures tests-pending labels Jul 2, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 2, 2020

makortel reviewed Jul 2, 2020

View reviewed changes

Apply suggestions from code review

37f07e3

Co-authored-by: Matti Kortelainen <matti.kortelainen@cern.ch>

cmsbuild added code-checks-pending and removed code-checks-approved labels Jul 2, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 2, 2020

Protected mutex locking with RAII.

2a042ca

In case some of the cudaCheck() calls threw (line 596), the mutex would have been left locked. (Ported from CachingDeviceAllocator). Also merged instanciation and locking where they appeared bach-to-back in CachingDeviceAllocator.

cmsbuild added code-checks-pending and removed code-checks-approved labels Jul 2, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 2, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 2, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 2, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 2, 2020

cmsbuild added fully-signed heterogeneous-approved and removed heterogeneous-pending pending-signatures labels Jul 3, 2020

cmsbuild added orp-approved and removed orp-pending labels Jul 4, 2020

cmsbuild merged commit fa0cea0 into cms-sw:master Jul 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Protected mutex locking with RAII. #30501

Protected mutex locking with RAII. #30501

ericcano commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

fwyzard commented Jul 2, 2020

makortel commented Jul 2, 2020

makortel left a comment

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

ericcano commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 2, 2020

cmsbuild commented Jul 2, 2020 •

edited

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 3, 2020

cmsbuild commented Jul 3, 2020

silviodonato commented Jul 4, 2020

Protected mutex locking with RAII. #30501

Protected mutex locking with RAII. #30501

Conversation

ericcano commented Jul 2, 2020

PR description:

PR validation:

if this PR is a backport please specify the original PR and why you need to backport that PR:

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

fwyzard commented Jul 2, 2020

makortel commented Jul 2, 2020

makortel left a comment

Choose a reason for hiding this comment

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

ericcano commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 2, 2020

cmsbuild commented Jul 2, 2020 • edited

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 3, 2020

cmsbuild commented Jul 3, 2020

silviodonato commented Jul 4, 2020

cmsbuild commented Jul 2, 2020 •

edited