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
