[asterisk-dev] [Code Review] 3777: module loader: Unload modules in reverse order of their start order

[Joshua Colp]

> On July 21, 2014, 5:49 p.m., Mark Michelson wrote:
> > /trunk/main/loader.c, lines 1001-1005
> > <https://reviewboard.asterisk.org/r/3777/diff/2/?file=64886#file64886line1001>
> >
> >     Feel free to cringe at this suggestion, but since you've created both an AST_DLLIST_ENTRY and an AST_LIST_ENTRY on the ast_module structure, you could maintain two parallel lists of modules. You'd have the module_list, that is important for module loading and unloading, and you could have the alphabetical list that is used by the CLI command.
> >     
> >     At the time that you add the module to the module_list (what I've highlighted), you can also add it to the alphabetical list. That way, you would not have to lock the module_list and rebuild the alphabetical list each time ast_update_module_list() is called. Up to you really.
> 
> Matt Jordan wrote:
>     /me cringes
>     
>     So, when I first started at this work, I approached it from the angle of ordering the module_list during start up. I had a vector of modules lists, where the vector had slots based on the various possible module priorities. That was a mistake.
>     
>     Let me tell you: module loading is some strange joo-joo. Things get opened, "loaded", and discarded more times than you would believe. By the time *something* gets around calling load_module, that particular 'module' has probably been in and out of memory (and possibly the modules list) a number of times. And don't even get me started on embedded modules. That's a whole other ball of string to untangle.
>     
>     I fully admit that this is the "poor man's" fix for the module loader - ideally we'd have an actual dependency tree, and other niceties. I'm hesitant to make too many changes in here - and having two module lists feels... dangerous.

Agreed, I think complicating things more in that area with trying to maintain two lists is asking for trouble. I think doing a temporary list is a nice compromise for the few times that the list is actually shown. It doesn't have to be performant.


- Joshua


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/3777/#review12789
-----------------------------------------------------------


On July 21, 2014, 4:58 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3777/
> -----------------------------------------------------------
> 
> (Updated July 21, 2014, 4:58 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> When Asterisk starts a module (calling its load_module function), it re-orders the module list, sorting it alphabetically. Ostensibly, this was done so that the output of 'module show' listed modules in alphabetic order. This had the unfortunate side effect of making modules with complex usage patterns practically unloadable. A module that has a large number of modules that depend on it is typically abandoned during the unloading process. This results in its memory not being reclaimed during exit.
> 
> Generally, this isn't harmful - when the process is destroyed, the operating system will reclaim all memory allocated by the process. However, it makes tracking memory leaks or ref debug leaks a real pain.
> 
> While this patch is not a complete overhaul of the module loader - such an effort would be beyond the scope of what could be done for Asterisk 13 - this does make some marginal improvements to the loader such that modules like res_pjsip or res_stasis *may* be made properly un-loadable in the future.
> 1. The linked list of modules has been replaced with a doubly linked list. This allows traversal of the module list to occur backwards. The module shutdown routine now walks the global list backwards when it attempts to unload modules.
> 2. The alphabetic reorganization of the module list on startup has been removed. Instead, a started module is placed at the end of the module list.
> 3. The ast_update_module_list function - which is used by the CLI to display the modules - now does the sorting alphabetically itself. It creates its own linked list and inserts the modules into it in alphabetic order. This allows for the intent of the previous code to be maintained.
> 
> This patch also contains a fix for res_calendar. Without calendar.conf, the calendar modules were improperly bumping the use count of res_calendar, then failing to load themselves. This patch makes it so that we detect whether or not calendaring is enabled before altering the use count.
> 
> 
> Diffs
> -----
> 
>   /trunk/res/res_calendar.c 419109 
>   /trunk/main/loader.c 419109 
> 
> Diff: https://reviewboard.asterisk.org/r/3777/diff/
> 
> 
> Testing
> -------
> 
> Verified that the CLI command worked appropriately.
> 
> Verified that module loading/unloading of res_calendar (whose calendar modules modify the res_calendar use count) loaded/unloaded properly:
> 
>  Asterisk Dynamic Loader Starting:
> [Jul 14 15:14:52] NOTICE[11877]: loader.c:1317 load_modules: 6 modules will be loaded.
>  Loading res_calendar.so.
>  Loading res_calendar_icalendar.so.
>  Loading res_calendar_exchange.so.
>  Loading res_calendar_caldav.so.
>  Loading res_calendar_ews.so.
> Asterisk Ready.
> *CLI> core stop gracefully
> Waiting for inactivity to perform halt...
>  Unloading res_calendar_ews.so
>  Unloading res_calendar_caldav.so
>  Unloading res_calendar_exchange.so
>  Unloading res_calendar_icalendar.so
>  Unloading res_calendar.so
> Asterisk cleanly ending (0).
> Executing last minute cleanups
> 
> Verified that using autoload with all modules, module are started as they were previously, and now are stopped/unloaded in the reverse order.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140724/bee3812c/attachment.html>