[Asterisk-code-review] main/cdr.c: Alleviate CDR deadlock (asterisk[15])

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/9270 )

Change subject: main/cdr.c: Alleviate CDR deadlock
......................................................................


Patch Set 3: Code-Review-1

(9 comments)

https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c
File main/cdr.c:

https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3787
PS3, Line 3787: 	int nextms = 0;
No need to initialize nextms.  You always initialize it below.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3796
PS3, Line 3796: 	/* manually reschedule from this point in time */
Comment isn't quite right anymore.  How about:

Calculate the next scheduled interval.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3799
PS3, Line 3799: 	if (!nextms) {
              : 		/* I hate doing this, but previously if you set the batch time to 0, it would continuously
              : 		   resubmit batches to the scheduler.  IMHO that seems buggy, but I'd rather not break it
              : 		   if someone was relying on that behavior */
              : 		nextms = 1;
              : 	}
I'd say it is buggy.  All they are doing is needlessly loading their CPU when CDR's already load the CPU enough.  The user should not be allowed to set it to zero.  I'd say set nextms to 1000 and fix the allowed range of the "time" option in process_config() to 1 - MAX_BATCH_TIME.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3807
PS3, Line 3807: 	/* returning zero so the scheduler does not automatically reschedule */
This comment is not needed anymore as you never return zero now.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3884
PS3, Line 3884: 	/* Don't call start_batch with cdr_batch_lock held */
s/call start_batch/submit a batch/


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3899
PS3, Line 3899: 		/* this only can happen if someone sets there batch time to 0 or does 'cdr submit' from the CLI back to back quickly */
Restore the previous comment as there will normally not be an empty scheduler when in batch mode.  The only time I can see the scheduler being empty here is if someone is rescheduling a batch in start_batch_mode().


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@3910
PS3, Line 3910: 		ast_debug(2, "Processed %d scheduled CDR batches from the run queue\n", numevents);
This debug msg isn't quite right.
s/scheduled //


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4410
PS3, Line 4410: 	SCOPED_LOCK(batch, &cdr_batch_lock, ast_mutex_lock, ast_mutex_unlock);
This scoped lock can cause a deadlock when you call start_batch_mode().

Sched task submit_scheduled_batch() calls cdr_submit_batch() which locks the cdr_batch_lock.  If it is running when start_batch_mode() tries to reschedule you have deadlock.

I don't see why we even need to lock cdr_batch_lock here as its job is to protect access to the CDR batch list.


https://gerrit.asterisk.org/#/c/9270/3/main/cdr.c@4424
PS3, Line 4424: 	/* Kill the currently scheduled item */
This comment needs to be removed.



-- 
To view, visit https://gerrit.asterisk.org/9270
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 15
Gerrit-MessageType: comment
Gerrit-Change-Id: I6333e865db7c593c102c2fd948cecdb96481974d
Gerrit-Change-Number: 9270
Gerrit-PatchSet: 3
Gerrit-Owner: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Fri, 22 Jun 2018 19:13:19 +0000
Gerrit-HasComments: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180622/72ddc5ae/attachment-0001.html>