You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by GitBox <gi...@apache.org> on 2021/06/01 13:14:16 UTC

[GitHub] [qpid-dispatch] mgoulish opened a new pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

mgoulish opened a new pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643192133



##########
File path: src/log.c
##########
@@ -648,8 +685,12 @@ qd_error_t qd_log_entity(qd_entity_t *entity)
             src->includeSource = include_source;
         }
 
+        bool add_log_stats = src->add_log_stats;
+        src->add_log_stats = false;
         sys_mutex_unlock(log_source_lock);
-
+        if(add_log_stats) {
+            qd_entity_cache_add(QD_LOG_STATS_TYPE, src);

Review comment:
       I am unsure why we need to add the QD_LOG_STATS_TYPE here ?
   I commented this line and the logStats was still created because qd_log_source() is creating it. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] mgoulish closed pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
mgoulish closed pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] mgoulish commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
mgoulish commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643182790



##########
File path: src/log.c
##########
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
     default_log_source->includeTimestamp = true;
     default_log_source->includeSource = 0;
     default_log_source->sink = log_sink_lh(SINK_STDERR);
+    default_log_source->add_log_stats = false;

Review comment:
       Leave it. It's a nanosecond.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643210620



##########
File path: src/log.c
##########
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
     default_log_source->includeTimestamp = true;
     default_log_source->includeSource = 0;
     default_log_source->sink = log_sink_lh(SINK_STDERR);
+    default_log_source->add_log_stats = false;

Review comment:
       If you look at https://github.com/apache/qpid-dispatch/blob/main/src/log.c#L368, the ZERO(log_source); is called. This sets all bits to 0 on the log_source which then automatically sets the add_log_stats to false, so no need to set the add_log_stats explicitly to false in qd_log_source_defaults() which is called right after the call to ZERO(log_source); 
   But yes, if ZERO(log_source);  was not called in the future, then qd_log_source_defaults() would take care of defaulting the add_log_stats flag. 




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] mgoulish commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
mgoulish commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643196151



##########
File path: src/log.c
##########
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
     default_log_source->includeTimestamp = true;
     default_log_source->includeSource = 0;
     default_log_source->sink = log_sink_lh(SINK_STDERR);
+    default_log_source->add_log_stats = false;

Review comment:
       And anyway -- this function initializes the log. To initialize the log, that flag should be set to false.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] mgoulish commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
mgoulish commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643111160



##########
File path: src/log.c
##########
@@ -379,7 +398,14 @@ qd_log_source_t *qd_log_source(const char *module)
 {
     sys_mutex_lock(log_source_lock);
     qd_log_source_t* src = qd_log_source_lh(module);
+    // Note to Mick: Mick! Please don't access log_source fields
+    // outside of the log_source lock. That's why there's a lock, you know.
+    bool add_me = src->add_me;

Review comment:
       How about    add_me_or_else     ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643105917



##########
File path: src/log.c
##########
@@ -379,7 +398,14 @@ qd_log_source_t *qd_log_source(const char *module)
 {
     sys_mutex_lock(log_source_lock);
     qd_log_source_t* src = qd_log_source_lh(module);
+    // Note to Mick: Mick! Please don't access log_source fields
+    // outside of the log_source lock. That's why there's a lock, you know.
+    bool add_me = src->add_me;

Review comment:
       Can we please call this flag add_log_stats or something instead of add_me ?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643129435



##########
File path: src/log.c
##########
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
     default_log_source->includeTimestamp = true;
     default_log_source->includeSource = 0;
     default_log_source->sink = log_sink_lh(SINK_STDERR);
+    default_log_source->add_log_stats = false;

Review comment:
       This line might not be necessary because the call to qd_log_source() already sets the add_log_stats flag to false.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] mgoulish commented on pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
mgoulish commented on pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#issuecomment-854811592


   Closing this PR in favor of a new one, coming soon.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] codecov-commenter commented on pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#issuecomment-852143148


   # [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#1239](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d3b1f4a) into [main](https://codecov.io/gh/apache/qpid-dispatch/commit/753ea62ffde1de016128faac04d2fe3f6c7d67f3?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (753ea62) will **decrease** coverage by `0.02%`.
   > The diff coverage is `90.00%`.
   
   > :exclamation: Current head d3b1f4a differs from pull request most recent head a92f418. Consider uploading reports for the commit a92f418 to get more accurate results
   [![Impacted file tree graph](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/graphs/tree.svg?width=650&height=150&src=pr&token=rk2Cgd27pP&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff             @@
   ##             main    #1239      +/-   ##
   ==========================================
   - Coverage   84.73%   84.70%   -0.03%     
   ==========================================
     Files         113      113              
     Lines       28035    28053      +18     
   ==========================================
   + Hits        23756    23763       +7     
   - Misses       4279     4290      +11     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [src/log.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2xvZy5j) | `91.14% <90.00%> (-0.13%)` | :arrow_down: |
   | [...router\_core/modules/edge\_router/link\_route\_proxy.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvbGlua19yb3V0ZV9wcm94eS5j) | `78.69% <0.00%> (-4.15%)` | :arrow_down: |
   | [src/router\_core/modules/edge\_router/edge\_mgmt.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL21vZHVsZXMvZWRnZV9yb3V0ZXIvZWRnZV9tZ210LmM=) | `84.15% <0.00%> (-1.00%)` | :arrow_down: |
   | [src/router\_core/transfer.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL3RyYW5zZmVyLmM=) | `93.31% <0.00%> (-0.65%)` | :arrow_down: |
   | [src/router\_core/forwarder.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2ZvcndhcmRlci5j) | `92.64% <0.00%> (-0.40%)` | :arrow_down: |
   | [src/adaptors/http1/http1\_server.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX3NlcnZlci5j) | `84.57% <0.00%> (-0.30%)` | :arrow_down: |
   | [src/router\_core/connections.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2Nvbm5lY3Rpb25zLmM=) | `90.44% <0.00%> (-0.20%)` | :arrow_down: |
   | [src/router\_core/delivery.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL3JvdXRlcl9jb3JlL2RlbGl2ZXJ5LmM=) | `93.14% <0.00%> (-0.19%)` | :arrow_down: |
   | [src/message.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21lc3NhZ2UuYw==) | `87.34% <0.00%> (+0.07%)` | :arrow_up: |
   | [src/adaptors/http1/http1\_codec.c](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL2FkYXB0b3JzL2h0dHAxL2h0dHAxX2NvZGVjLmM=) | `85.15% <0.00%> (+0.12%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/qpid-dispatch/pull/1239/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [753ea62...a92f418](https://codecov.io/gh/apache/qpid-dispatch/pull/1239?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org

[GitHub] [qpid-dispatch] ganeshmurthy commented on a change in pull request #1239: DISPATCH-1956: Fix TSan-reported lock-inversion

Posted by GitBox <gi...@apache.org>.
ganeshmurthy commented on a change in pull request #1239:
URL: https://github.com/apache/qpid-dispatch/pull/1239#discussion_r643221281



##########
File path: src/log.c
##########
@@ -518,6 +544,7 @@ void qd_log_initialize(void)
     default_log_source->includeTimestamp = true;
     default_log_source->includeSource = 0;
     default_log_source->sink = log_sink_lh(SINK_STDERR);
+    default_log_source->add_log_stats = false;

Review comment:
       I wonder if the call to ZERO should be moved inside the qd_log_source_defaults() function. If we do that, the qd_log_source_defaults() can be called from the place where ZERO is currently being called from. And then, there would be no need to set the add_log_stats to false.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org