[Asterisk-code-review] res pjsip: Add serialized scheduler (res pjsip/pjsip schedu... (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change.

Change subject: res_pjsip:  Add serialized scheduler (res_pjsip/pjsip_scheduler.c)
......................................................................


Patch Set 4: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/2443/4/include/asterisk/res_pjsip.h
File include/asterisk/res_pjsip.h:

Line 1364: 	AST_SIP_SCHED_TASK_FIXED = (0 << 0),
I don't understand why these 0 bit-shifted flags exist. It looks like they serve as "opposites" to corresponding 1 bit-shifted flags. However, there would never be any reason to explicitly specify one of the 0 bit-shifted flags since the absence of the 1 bit-shifted flag would imply the same thing.


https://gerrit.asterisk.org/#/c/2443/4/res/res_pjsip/pjsip_scheduler.c
File res/res_pjsip/pjsip_scheduler.c:

PS4, Line 41: 	/*! ast_sip_sched task id */
            : 	int task_id;
Given how this gets stringified on initialization, you may want to make this an int32_t. Alternatively, you could make this a uint32_t since I'm not sure if you actually want the ID rolling over to negative values.


PS4, Line 160: 	res = ast_sched_del(scheduler_context, schtd->current_scheduler_id);
             : 	ao2_unlock_and_unref(schtd);
             : 	ao2_unlink(tasks, schtd);
Things can get a bit bumpy here. ast_sched_del() can potentially deadlock. If you call ast_sched_del() while run_task() is executing, ast_sched_del() will wait for the task to complete. The problem is that since you are holding the schtd lock, run_task() will be unable to progress since it will be stuck trying to acquire the schtd lock.

I think that once you have set schtd->interval to 0, it should be fine to unlock schtd, then call ast_sched_del(). This would prevent the deadlock, and I believe it would also ensure that the data of schtd is protected as desired.


Line 252: 			since_when = schtd->is_running ? ast_tvnow() : schtd->last_end;
I think the is_running logic here is skewed. If the task is currently running, then ast_tvnow() will be set as since_when. Then later, we calculate

    delay -= ast_tvdiff_ms(ast_tvnow(), since_when);

The calculation may come out to 0, or it may come out to a very small number of milliseconds.

It's not far-fetched to interpret 0 to mean the task is currently running. However, the small number of milliseconds would have to be interpreted to mean that the task is just about to be run. In actuality, the task isn't going to run again until ast_tvnow() + schtd->interval really.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7af6ad2b2d896ea68e478aa1ae201d6dd016ba1c
Gerrit-PatchSet: 4
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes