Fix suspend race cond #2167

lbudai · 2018-07-05T15:47:48Z

@MrAnno : please check whether it supersedes #2045 or not.

suspend is set by log_source_flow_control_suspend
suspend is unset by log_source_wakeup
free_to_send blocks reading until suspend bit is set or the window size is 0
when window size was 0 before log_source_flow_control_adjust is called, then we call log_source_wakeup
when ack_tracker receives an ACK, then it checks if source is suspended or not (and based on the acknowledge type it makes a decision whether we need to suspend/wakeup the source)

kira-syslogng · 2018-07-05T16:08:53Z

Build FAILURE

MrAnno · 2018-07-05T15:58:31Z

lib/early_ack_tracker.c

log_source_wakeup is unnecessary here. log_source_flow_control_adjust should schedule the wakeup based on its knowledge (window size + suspend bit).

I think after my change it's not...
Maybe it's a wrong direction but trying to make suspend/wakeup more symmetrical:
suspend bit is set by suspend and unset by wakeup...

I made a small change:

suspend is set by log_source_flow_control_suspend

suspend is unset by log_source_wakeup

free_to_send blocks reading until suspend bit is set or the window size is 0

when window size was 0 before log_source_flow_control_adjust is called, then we call log_source_wakeup

when ack_tracker receives an ACK, then it checks if source is suspended or not (and based on the acknowledge type it makes a decision whether we need to suspend/wakeup the source)

MrAnno · 2018-07-05T15:58:37Z

lib/late_ack_tracker.c

log_source_wakeup is unnecessary here. log_source_flow_control_adjust should schedule the wakeup based on its knowledge (window size + suspend bit).

MrAnno · 2018-07-05T16:06:59Z

lib/logsource.c

The new (incremented) window size should be used here instead of the old one.

Instead of calling window_size_counter_get here, we could write old_window_size + window_size_increment == ... which is guaranteed to preserve the value of the previous window_size_counter_add call (preserves atomicity).

yes, you are right

MrAnno · 2018-07-05T16:10:27Z

lib/logsource.c

I would not add this condition to log_source_wakeup.
The name of this function tells me that it will wake the source up without condition.

well... someone should check this suspend bit...
This is not the best and not the final place (this is why I added the wip flag :) )

Oops, okay, sorry. I'll check it.

I'm also playing with this a little bit.

kira-syslogng · 2018-07-05T16:15:35Z

Build FAILURE

kira-syslogng · 2018-07-05T17:18:56Z

Build FAILURE

kira-syslogng · 2018-07-05T17:33:53Z

Build FAILURE

kira-syslogng · 2018-07-05T17:47:01Z

Build FAILURE

kira-syslogng · 2018-07-05T18:25:19Z

Build SUCCESS

MrAnno · 2018-07-06T10:19:18Z

lib/atomic-gssize.h

I'd remove these 3 racy operations. We had only problems with them in the past.

...and I would not, not part of this PR. I agree with you that we should consider to remove all the racy atomic operations, but... but I also know that I don't want to discuss it it as part of this PR.
This is why I wanted to atomic-gssize PR handled separately. Please, add these notes to there, or, after that PR merged, we could open a new PR where we remove all these operations and explain why we don't need them anymore.

If no other issues, don't forget to approve the requested changes :)

Okay. I've added my comment to #2159.

kira-syslogng · 2018-07-06T11:35:57Z

Build SUCCESS

MrAnno · 2018-07-06T12:05:07Z

lib/ack_tracker.h

I think we should not check free_to_send() here. We might skip suspend() while another thread is waking up the source.

MrAnno · 2018-07-06T12:06:46Z

lib/ack_tracker.h

The free_to_send() call is a problem here too.
Instead of calling wakeup here, I would reset the suspend bit in _flow_control_window_size_adjust(), where we can do it safely.

kira-syslogng · 2018-07-06T13:16:15Z

Build FAILURE

kira-syslogng · 2018-07-06T16:13:16Z

Build SUCCESS

MrAnno · 2018-07-06T16:13:42Z

lib/window-size-counter.c

atomic_gssize is implemented with atomic pointers, so I would add a static_assert to compare its size with gsize if we want to use G_MAXSIZE on it.

MrAnno · 2018-07-06T16:26:01Z

lib/window-size-counter.c

Should not it be 1ULL << ...? I think the cast would be unnecessary this way.

Wouldn't be the type of the expression unsigned long long? I'd stick to the gsize type.

MrAnno · 2018-07-06T16:37:21Z

lib/logsource.c

Would you like to leave this debug message in the final patch?

Well.. I found it very useful during development... but it is possible to print out a better message, I'll try to do that. But I'd keep a debug message here.

MrAnno · 2018-07-06T16:41:06Z

lib/logsource.c

Please consider adding this new evt tag to log_source_flow_control_suspend as well. We have the same debug message there.

I don't know about our evt tag conventions, but it seems we prefer lowercase tag names (function vs. Function).

ok, will check it.

MrAnno · 2018-07-06T16:42:33Z

@kira-syslogng do perftest

kira-syslogng · 2018-07-06T17:19:46Z

Build SUCCESS

kira-syslogng · 2018-07-09T09:49:20Z

Build FAILURE

kira-syslogng · 2018-07-09T11:29:11Z

Build SUCCESS

This is an atomic counter for tracking window size and suspend state. On 64 bit systems the max. window size is 2^63-1, while on 32 bit systems 2^31-1. Code asserts in case of overflow/underflow. Signed-off-by: Laszlo Budai <[email protected]>

A window-size leak also fixed: when ack_type is AT_SUSPENDED, we have to count the window size even if there is no continuous range in ringbuffer. Signed-off-by: Laszlo Budai <[email protected]>

kira-syslogng · 2018-07-10T18:02:20Z

Build SUCCESS

kira-syslogng · 2018-07-10T18:08:07Z

Build SUCCESS

MrAnno

🔲 (This is a window.)

Kokan added the in progress label Jul 5, 2018

lbudai force-pushed the fix-suspend-race-cond branch from 422b974 to ad3f796 Compare July 5, 2018 15:53

lbudai requested a review from MrAnno July 5, 2018 15:56

MrAnno suggested changes Jul 5, 2018

View reviewed changes

lbudai force-pushed the fix-suspend-race-cond branch 2 times, most recently from ce03904 to 2a676d0 Compare July 5, 2018 17:14

lbudai force-pushed the fix-suspend-race-cond branch from 2a676d0 to 47c3259 Compare July 5, 2018 17:32

MrAnno reviewed Jul 6, 2018

View reviewed changes

lbudai mentioned this pull request Jul 6, 2018

Add atomic gssize #2159

Merged

lbudai force-pushed the fix-suspend-race-cond branch from a298db9 to dfd022b Compare July 6, 2018 11:17

MrAnno reviewed Jul 6, 2018

View reviewed changes

lbudai force-pushed the fix-suspend-race-cond branch from 178af2d to b8fc53a Compare July 6, 2018 15:49

lbudai changed the title ~~[wip] Fix suspend race cond~~ Fix suspend race cond Jul 6, 2018

MrAnno previously approved these changes Jul 6, 2018

View reviewed changes

MrAnno mentioned this pull request Jul 6, 2018

[WIP] Fix window leak in log_source_flow_control_suspend() #2045

Closed

lbudai dismissed MrAnno’s stale review via 5f43934 July 9, 2018 09:29

lbudai force-pushed the fix-suspend-race-cond branch from b8fc53a to 5f43934 Compare July 9, 2018 09:29

lbudai force-pushed the fix-suspend-race-cond branch from 5f43934 to 7c17cc7 Compare July 9, 2018 11:08

MrAnno previously approved these changes Jul 9, 2018

View reviewed changes

lib: add window-size-counter

d639ffe

This is an atomic counter for tracking window size and suspend state. On 64 bit systems the max. window size is 2^63-1, while on 32 bit systems 2^31-1. Code asserts in case of overflow/underflow. Signed-off-by: Laszlo Budai <[email protected]>

lbudai dismissed MrAnno’s stale review via fe7c692 July 10, 2018 17:41

lbudai force-pushed the fix-suspend-race-cond branch from 7c17cc7 to fe7c692 Compare July 10, 2018 17:41

logsource: fix a race in window-size/suspend counting

d5dc3bd

A window-size leak also fixed: when ack_type is AT_SUSPENDED, we have to count the window size even if there is no continuous range in ringbuffer. Signed-off-by: Laszlo Budai <[email protected]>

lbudai force-pushed the fix-suspend-race-cond branch from fe7c692 to d5dc3bd Compare July 10, 2018 17:44

MrAnno approved these changes Jul 10, 2018

View reviewed changes

furiel approved these changes Jul 11, 2018

View reviewed changes

furiel merged commit 8779d8b into syslog-ng:master Jul 12, 2018

Kokan removed the in progress label Jul 12, 2018

Fix suspend race cond #2167

Fix suspend race cond #2167

Uh oh!

Conversation

lbudai commented Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

MrAnno Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrAnno Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrAnno Jul 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

kira-syslogng commented Jul 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kira-syslogng commented Jul 6, 2018

Uh oh!

MrAnno Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kira-syslogng commented Jul 6, 2018

Uh oh!

kira-syslogng commented Jul 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrAnno commented Jul 6, 2018

lbudai commented Jul 5, 2018 •

edited

Loading

MrAnno Jul 5, 2018 •

edited

Loading

MrAnno Jul 5, 2018 •

edited

Loading

MrAnno Jul 5, 2018 •

edited

Loading

MrAnno Jul 6, 2018 •

edited

Loading