-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
L1TStage2EMTF seg faulting with change to DQMEDAnalyzer #29096
Comments
A new Issue was created by @Dr15Jones Chris Jones. @Dr15Jones, @smuzaffar, @silviodonato, @makortel, @davidlange6, @fabiocos can you please review it and eventually sign/assign? Thanks. cms-bot commands are listed here |
The traceback is
|
assign dqm |
New categories assigned: dqm @jfernan2,@andrius-k,@schneiml,@fioriNTU,@kmaeshima you have been requested to review this Pull request/Issue and eventually sign? Thanks |
The problem is here https://github.com/cms-sw/cmssw/blob/master/DQM/L1TMonitor/src/L1TStage2EMTF.cc#L829-L830 where the underlying TH2Fs are being accessed without any locking. |
assign dqm, l1 |
New categories assigned: l1 @benkrikler,@rekovic you have been requested to review this Pull request/Issue and eventually sign? Thanks |
Doing
gives 266 files that should be checked. |
... to avoid issue cms-sw#29096.
I opened a PR to change this specific module back to Regarding all the other The challenge is to systematically find all |
After filtering out all known 'safe' ED module types (which include inheriting from DQMEDHarvester, I still find 222 classes which are calling getTH See
|
I don't think there is a better way than to go through Chris' list by hand and classify the files/classes depending on the action taken (because likely something needs to be done anyway for most/all of them eventually). |
@Dr15Jones Thanks, that list actually looks rather manageable. One more thing to exclude are legacy Now the next hard part is what to do with code like the L1T module here, we'll need to pass that down to the subsystems in many cases since the usages require some domain knowledge to understand. We could just replace a complete replacement for |
those were excluded from the list I made. |
@Dr15Jones ah ok. Though, e.g. everything in |
Well, under an assumption that legacy modules are supported “forever”...
On Mar 12, 2020, at 11:47 AM, Marcel Schneider <[email protected]<mailto:[email protected]>> wrote:
@Dr15Jones<https://github.com/Dr15Jones> ah ok. Though, e.g. everything in SiStripCommissioningClients is to my knowledge only used by legacy modules and will likely never be migrated (there is no need for it, the code only ever runs in private setups and does its task fine, I am told).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#29096 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABGPFQ2OFB2JTTIK6EKPJBLRHC4TFANCNFSM4LBIY34Q>.
|
@davidlange6 From DQM side, they are. From EDM side, it might be required to change them to one modules with lumi transitions, but that is a trivial change. And then, for that specific code the time horizon is probably LS3, where the respective hardware gets removed... but until then, it might still be required and there likely isn't any manpower for any nontrivial changes. |
@schneiml my simple filtering of the list removed code that was directly called by legacy |
Is this issue still alive? |
The problem mentioned in the description and #29096 (comment) appears to still be there cmssw/DQM/L1TMonitor/src/L1TStage2EMTF.cc Lines 834 to 837 in 638bc29
|
Although the segfault was circumvented in #29097 by making the module |
@vukasinmilosevic please consider removing the Divide in: |
ping @vukasinmilosevic |
HI @jfernan2 we have someone taking a look into this. He is currently getting up to speed and will update us soon. |
@vukasinmilosevic Any news on this issue? |
Hi @cecilecaillol, I will check with the EMTF experts and get back at you asap. |
@vukasinmilosevic Did you get a chance to check with the EMTF experts? |
@vukasinmilosevic Do you have any update on this issue? |
Hi @cecilecaillol, apologies this fell down the list of tasks. I will follow up with relevant experts after the holiday period. |
It IB is showing L1TStage2EMTF causing segmentation faults after the change to stream module.
The text was updated successfully, but these errors were encountered: