[asterisk-dev] (mis)use of threadstorage.h ?

[Russell Bryant]

Luigi Rizzo wrote:
> could someone explain how the code in threadstorage.h is used
> and what is it supposed to do ?
> 
> I am not sure i follow the code too well because it is hidden
> behind several macros, nor i could find in the code any comment
> on what this code is supposed to provide.

The threadstorage API provides the ability to define a variable that gets 
allocated per-thread.  This header file also contains a bunch of functions for 
manipulating the ast_dynamic_str object (a string where the buffer will grow if 
needed) as well as functions for creating thread-specific ast_dynamic_str 
objects.  I'll address each usage individually.

> Browsing through the asterisk source code (details below), i see that
> there are the following usages:
> 
> 1. frame.c and iax2-parser.c, seems the only reasonable usage -
>    provide a per-thread freelist of frames. But then i wonder why
>    it is not used also in ast_frame_header_new(). (I am also unsure
>    why the code in frame.c is replicated in iax2-parser.c)

Well, it is actually used in ast_frame_header_new(), so perhaps you accidentally 
overlooked it.  The reason that this is duplicated is that in frame.c, it is a 
cache of ast_frame structures, where as in iax2-parser.c it is a cache of 
iax_frame structures.  I guess there is some code duplication, but it's not a 
large amount.  If someone wanted to centralize this code, it would of course be 
happily accepted.

> 2. in some cases to make certain routines thread-safe
>    (but still not reentrant). As you can see below, in many of these
>    cases this is more expensive than it should be; and the fact
>    that the code is not reentrant is almost as dangerous as not
>    having it thread-safe:
> 
>     utils.c
> 	thread-safe version of ast_inet_ntoa.

This was actually the first use of threadstorage.h in Asterisk.  It was 
suggested to me by Kevin.  The old version worked by having the caller pass in a 
buffer to be used for the result.  I guess the advantage is that its usage is 
now more like what it replaces, inet_ntoa(), but it does make it not reentrant. 
  For most of the uses of it, it didn't matter.  But, in some cases, the result 
had to be copied to a local buffer so that it would be safe to call it again. 
I'm sure someone is going to write new code that calls ast_inet_ntoa() more than 
once before using the result, which would be very bad.

I guess we need to address this somehow.  The safest thing is probably to just 
go back to how it was before.  At the very least, the limitations on the usage 
should be clearly documented.  I really should have already done that ... oops.

>     chan_skinny.c::device2str_threadbuf
> 	local buffer for device2str
> 	but it would be cheaper to use an automatic buffer in
> 	the caller (skinny_show_devices())
> 
>     chan_skinny.c::control2str_threadbuf
> 	local buffer for control2str
> 	but it would be cheaper to use an automatic buffer in
> 	the caller (skinny_indicate())
> 
>     channel.c
> 	buffer to make ast_state2str thread-safe (but not reentrant)

Well, the way it is now is better than these were before.  These functions used 
static char buffers to hold the result, which was not thread-safe or reentrant. 
  What do you mean by "automatic buffer"?

> 3. in the remaining cases, i think threadstorage is used in the wrong way,
>    as it just allocates a variable whose scope is limited to a single
>    function. Here, a plain automatic variable, or an alloca(), would
>    suffice, and would avoid the overhead of the ast_alloc()
>    (which has locking, and is used anyways for the first allocation
>    of these objects):
> 
>     chan_sip.c::ts_temp_pvt
> 	this can just be replaced by an alloca()

I was not involved in this, so I have no idea how it is used here.

>     cli.c
> 	just an alloca() here ? the scope is only local to the function
> 	ast_cli
> 
>     logger.c
> 	verbose_buf, alloca() ?
> 	log_buf, alloca() ?

Here is how this set of changes came about.  I saw that in one of these 
functions, it used a vasprintf() (which does a malloc internally) to build 
whatever was going to logged and then freed it immediately.  So, I figured I 
would use this threadstorage API to make it so the allocation only needed to be 
done once per thread, or perhaps a few more times if the buffer size needed to 
be increased.  This is how the thread-specific ast_dynamic_str usage came about.

Then, another one of these functions had something like a "static char 
buf[4096];" that it used.  So, it used a mutex around the entire body of the 
function.  So, I used a thread-specific ast_dynamic_str object for the buffer so 
that the locking wasn't needed anymore.

This may have been an over-engineered solution to the problem.  The same thing 
could be done with an alloca based string, instead.  I'm trying to think of the 
best way to use alloca but to still not have a maximum length limitation on the 
resulting string.  Maybe we could create a macro that does something like ...

va_list ap;
int res;
char *buf = alloca(128);

va_start(ap, fmt);
res = vsnprintf(buf, 128, fmt, ap);
if (res + 1 > 128) {
    buf = alloca(res + 1);
    va_end(ap);
    va_start(ap, fmt);
    vsnprintf(buf, res + 1, fmt, ap);
}

> Anything wrong in the above analysis ?

I think the analysis is correct and I am very interested in any suggestions that 
you may have to improve what is there now.

-- 
Russell Bryant
Software Engineer
Digium, Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: russell.vcf
Type: text/x-vcard
Size: 266 bytes
Desc: not available
Url : http://lists.digium.com/pipermail/asterisk-dev/attachments/20061125/072d19aa/russell.vcf