Annotate cms::cuda::HostAllocator::allocate() as thread safe #33246

makortel · 2021-03-22T13:33:27Z

PR description:

This should silence 80 static analyzer warnings that @jpata reported in https://mattermost.web.cern.ch/cms-o-and-c/pl/6efrf7q4s7yi8k3oaamjhwdr3a

PR validation:

Code compiles.

cmsbuild · 2021-03-22T13:39:11Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33246/21700

This PR adds an extra 12KB to repository

cmsbuild · 2021-03-22T13:39:34Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

HeterogeneousCore/CUDAUtilities

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

cms-bot commands are listed here

makortel · 2021-03-22T13:40:49Z

@cmsbuild, please test

cmsbuild · 2021-03-22T16:13:03Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d981/13665/summary.html
COMMIT: 12c0f78
CMSSW: CMSSW_11_3_X_2021-03-21-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/33246/13665/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 5 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2639935
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2639900
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): 0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

fwyzard · 2021-03-22T17:37:51Z

HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h

@@ -28,7 +30,7 @@ namespace cms {
        using other = HostAllocator<U, FLAGS>;
      };

-      T* allocate(std::size_t n) const __attribute__((warn_unused_result)) __attribute__((malloc))
+      T* allocate CMS_THREAD_SAFE(std::size_t n) const __attribute__((warn_unused_result)) __attribute__((malloc))


is it the right place, between allocate and (std::size_t n) ?

That appears to be a legitimate place for C++ attributes, honestly I was following an example in

cmssw/DataFormats/TrackReco/interface/Track.h

Lines 74 to 76 in 8ffadd1

CovarianceMatrix& fillOuter CMS_THREAD_SAFE(CovarianceMatrix& v) const { return extra_->fillOuter(v); }

CovarianceMatrix& fillInner CMS_THREAD_SAFE(CovarianceMatrix& v) const { return extra_->fillInner(v); }

On a further thought I agree the placement looks a bit weird. @gartung, could you comment how picky the static analyzer is for the placement of [[cms::thread_safe]] attribute? Could it be placed e.g. before the function declaration? (that we seem to do with CMS_SA_ALLOW here

cmssw/FWCore/Concurrency/interface/WaitingTaskHolder.h

Line 77 in 8ffadd1

CMS_SA_ALLOW tbb::task_group* group() const noexcept { return m_group; }

)

as far as I know, both

CMS_THREAD_SAFE T* allocate(std::size_t n) const __attribute__((warn_unused_result)) __attribute__((malloc))

and

T* allocate(std::size_t n) const CMS_THREAD_SAFE __attribute__((warn_unused_result)) __attribute__((malloc))

(or after the other attributes) should be fine

As I recall there was some trial and error on where to place the attribute so the tokenizer would process it and attach it to the correct statement.

clang-check -ast-dump -ast-dump-filter=function-name will show you the AST the tokenizer produces and you can see if the attribute is attached to the function.

HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

makortel · 2021-03-31T14:26:21Z

@cmsbuild, please test

cmsbuild · 2021-03-31T14:34:24Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33246/21850

This PR adds an extra 12KB to repository

cmsbuild · 2021-03-31T14:40:51Z

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

cmsbuild · 2021-03-31T17:18:05Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-33d981/13889/summary.html
COMMIT: f20397f
CMSSW: CMSSW_11_3_X_2021-03-30-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/33246/13889/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2640841
DQMHistoTests: Total failures: 6
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2640812
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 155 log files, 37 edm output root files, 37 DQM output files
TriggerResults: no differences found

makortel · 2021-03-31T18:13:46Z

Seems that this way works as well, the static analyzer report is not complaining about const member function returning a pointer to non-const object.

gartung · 2021-04-01T00:35:43Z

I noticed a bug in the summary which claims there are ESR::get warnings in the static analyzer report but there are not. I figured out the bug in the script and added a fix to
cms-sw/cms-bot#1527

makortel · 2021-04-05T16:25:16Z

+heterogeneous

cmsbuild · 2021-04-05T16:25:37Z

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, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2021-04-06T07:07:26Z

+1

Annotate cms::cuda::HostAllocator::allocate() as thread safe

12c0f78

cmsbuild added this to the CMSSW_11_3_X milestone Mar 22, 2021

cmsbuild added code-checks-pending heterogeneous-pending orp-pending pending-signatures tests-pending labels Mar 22, 2021

makortel mentioned this pull request Mar 22, 2021

Annotate cms::cuda::HostAllocator::allocate() as thread safe cms-sw/framework-team#90

Closed

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 22, 2021

cmsbuild added tests-started and removed tests-pending labels Mar 22, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 22, 2021

fwyzard reviewed Mar 22, 2021

View reviewed changes

fwyzard reviewed Mar 31, 2021

View reviewed changes

HeterogeneousCore/CUDAUtilities/interface/HostAllocator.h Outdated Show resolved Hide resolved

Try clearer placement of CMS_THREAD_SAFE

f20397f

Co-authored-by: Andrea Bocci <andrea.bocci@cern.ch>

cmsbuild added code-checks-pending tests-started and removed code-checks-approved tests-approved labels Mar 31, 2021

cmsbuild added code-checks-approved and removed code-checks-pending labels Mar 31, 2021

cmsbuild added tests-approved and removed tests-started labels Mar 31, 2021

cmsbuild added fully-signed heterogeneous-approved and removed heterogeneous-pending pending-signatures labels Apr 5, 2021

cmsbuild added orp-approved and removed orp-pending labels Apr 6, 2021

cmsbuild merged commit bb3c404 into cms-sw:master Apr 6, 2021

makortel deleted the threadSafeHostAllocatorAllocate branch April 6, 2021 12:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate cms::cuda::HostAllocator::allocate() as thread safe #33246

Annotate cms::cuda::HostAllocator::allocate() as thread safe #33246

makortel commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

makortel commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

fwyzard Mar 22, 2021

makortel Mar 23, 2021

fwyzard Mar 31, 2021

gartung Mar 31, 2021 •

edited

gartung Mar 31, 2021

makortel commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

makortel commented Mar 31, 2021

gartung commented Apr 1, 2021

makortel commented Apr 5, 2021

cmsbuild commented Apr 5, 2021

silviodonato commented Apr 6, 2021

	CovarianceMatrix& fillOuter CMS_THREAD_SAFE(CovarianceMatrix& v) const { return extra_->fillOuter(v); }

	CovarianceMatrix& fillInner CMS_THREAD_SAFE(CovarianceMatrix& v) const { return extra_->fillInner(v); }

Navigation Menu

Annotate cms::cuda::HostAllocator::allocate() as thread safe #33246

Annotate cms::cuda::HostAllocator::allocate() as thread safe #33246

Conversation

makortel commented Mar 22, 2021

PR description:

PR validation:

cmsbuild commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

makortel commented Mar 22, 2021

cmsbuild commented Mar 22, 2021

Comparison Summary

fwyzard Mar 22, 2021

Choose a reason for hiding this comment

makortel Mar 23, 2021

Choose a reason for hiding this comment

fwyzard Mar 31, 2021

Choose a reason for hiding this comment

gartung Mar 31, 2021 • edited

Choose a reason for hiding this comment

gartung Mar 31, 2021

Choose a reason for hiding this comment

makortel commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

cmsbuild commented Mar 31, 2021

Comparison Summary

makortel commented Mar 31, 2021

gartung commented Apr 1, 2021

makortel commented Apr 5, 2021

cmsbuild commented Apr 5, 2021

silviodonato commented Apr 6, 2021

gartung Mar 31, 2021 •

edited