20 Mar
2019
20 Mar
'19
10:52 a.m.
Hi all,
this idea has been floating around for quite some time, and I finally took
some time for a proposed implementation: The introduction of a "volatile"
talloc context which one can use to allocate whatever temporary things from,
and which would automatically be free'd once we leave the select dispatch.
The current proposal is in https://gerrit.osmocom.org/#/c/libosmocore/+/13312
and I'm happy to receive any review.
How this works:
* within a new osmo_select_main_ctx() function, we create a temporary talloc
context before calling the filedescriptor call-back functions, and we
free that after the call-backs have returned
* any of the code running from within the select loop dispatch (which for
"normal" osmocom CNI projects is virtually everything) can use that
temporary context for allocations. There's a OTC_SELECT #define for
convenience. So you could do something like talloc_zero(OTC_SELECT, ...)
which would be automatically free'd after the current select loop
iteration.
Where is this useful? There's at least two common use cases:
* allocation of message buffers without having to worry about msgb ownership
* various temporary buffers e.g. for something-to-string conversions where
currently we use library-internal static buffers with all their known problems
(not being able to use them twice within a single printf() statement, not
being thread-safe, ...)
To Neels' disappointment, this is not all automatic. You
a) have to call the _c suffix of the respective function, e.g. osmo_hexdump_c()
instead of osmo_hexdump() with one extra initial argument:
OTC_SELECT. There's also msgb_alloc_c(), msgb_copy_c() and the like,
allowing msgb allocation from a context of your choice, as opposed
to the library-internal msgb_tall_ctx that we had so far.
b) have to use osmo_select_main_ctx() instead of osmo_select_main(). This is
an arbitrary limitation for optimization that Pau requested, to make sure
applications that don't want this can avoid all the extra talloc+free at
every select iteration. This is debatable, and we can make it automatic
in osmo_select_main(). It's probably worth a benchmark how expensive
that 'empty' allocation + free is.
However, I think that's a rather "OK" price to pay. Converting the
existing
callers can be more or less done with "sed" (yes, spatch is of course better).
While introducing this feature, I also tried to address two other topics, which
are not strictly related. However, ignoring those two now would mean
we'd have API/ABI breaks if we'd care about them in the future. Hence I
think we should resolve them right away:
1) introduce another context: OTC_GLOBAL. This is basically a library-internal
replacement for the "g_tall_ctx" that we typically generate as one of the
first things in every application. You can use it anywhere where you'd want
to allocate something form the global talloc context
2) make those contexts thread-local. As you may know, talloc is not thread
safe. So you cannot allocate/free from a single context on multiple
threds. However, it is safe to have separate talloc contexts on each
thread. Making the new OTC_* contexts per-thread, we are enabling the
[limited] use of this functionality in multi-threaded programs. This
is of course very far from making libosmocore thread-safe. There are
many library-internal data structures like timers, file descriptors,
the VTY, ... which are absolutely not thread-safe. However, I think
it's a step in the right direction and I don't expect any performance
impact for single-threaded programs by marking the contexts as
__thread.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
20 Mar
20 Mar
12:06 p.m.
Some additions below.
On Wed, Mar 20, 2019 at 10:52:46AM +0100, Harald Welte wrote:
Where is this useful? There's at least two common
use cases:
* allocation of message buffers without having to worry about msgb ownership
I forgot here that this obviously only works if the msgb is created + consumed
within the same select dispatch. If e.g. the msgb is enqueued on a transmit queue,
you would have to talloc_steal() / talloc_reparent() it to a different context
before returning from the select dispatch.
In terms of our usual use cases, I see the following scenarios:
a) incoming, locally handled message
The msgb can normally be allocated from OTC_SELECT as we are processing the
entire msgb within the select dispatch. If we end up queueing it somewhere,
we need to steal it.
b) incoming, forwarded message
if we already know we'll forward it (e.g. in gb-proxy or bsc_nat), we might
directly allocate it from OTC_GLOBAL or any of its children.
c) outgoing message
As we're operating in non-blocking mode, we can never make the assumption
that any of the outbound sockets will be write-able. Hence, we always have
a transmit queue, and as a result we have to allocate from OTC_GLOBAL or
one of its children.
d) local primitives (osmo_prim)
As osmo_prim are msgb-wrapped, and primitives [at least for now] are always
only between our local SAP user and provider (or vice versa) we might use
OTC_SELECT here, and make sure that all users that want to recycle a osmo_prim
msgb will steal it, if needed (I don't think that's a very valid use case).
This applies both ways: for primitives from the provider to the user,
as well as the other way around.
Regards,
Harald
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
12:54 p.m.
Hi,
First of all, I'm not against addition of this "select scoped talloc
context", and it's fine for me to merge if others find it's a really
handy feature. But I have the feeling it really adds unneeded extra
complexity and scenarios to take care in the code. New ways to get shot
on your knee, having to use talloc_steal() and talloc_reparent(). Not
sure if the benefits are worth the effort and increase of complexity.
IMHO we should be fine using regular global context (and freeing stuff
around) together with static/stack buffers when possible.
Regards,
Pau
--
- Pau Espin Pedrol <pespin(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Alt-Moabit 93
* 10559 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Director: Harald Welte
5:30 p.m.
Hi Pau,
On Wed, Mar 20, 2019 at 12:54:11PM +0100, Pau Espin Pedrol wrote:
First of all, I'm not against addition of this
"select scoped talloc
context", and it's fine for me to merge if others find it's a really handy
feature. But I have the feeling it really adds unneeded extra complexity and
scenarios to take care in the code. New ways to get shot on your knee,
having to use talloc_steal() and talloc_reparent(). Not sure if the benefits
are worth the effort and increase of complexity.
What is your solution to neels' recently described problem with msgb
ownership in successful/error cases when passing msgb's between different
subsystems (in his example the libosmo-sccp sigtran stack) ?
I think this is where the beauty of this system really shows.
But maybe it's possible even without the 'select scoped context' with keeping
allocations all global (like now) and using talloc_reference()? This way the
default behavior would be an explicit free() by the caller, i.e.
msg = msgb_alloc()
/* put some data in the msgb, e.g. received from socket */
call_some_other_subsystem(msg);
talloc_unlink(msg); /* cannot use {msgb,talloc}_free() anymore! */
And if that other_subsystem would want to keep msgb around, they would have
to talloc_reference() the msgb, thereby having two parents. The msgb would
only be free'd at the last talloc_unlink(). But that turns the talloc tree
into a graph and to me it really messes up things. Also the fact that you
can no longer use talloc_free() makes things very complicated. So not really
an option.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
21 Mar
21 Mar
2:24 a.m.
Talking of features, another idea comes to mind: freeing of FSM instances. I
more often than not have the problem that a child FSM is freeing, telling the
parent about it, then the parent decides to free because the child's result
signalled some end, and then we have a double free because the child was
allocated with the parent as talloc context, the parent has just deallocated,
but after the child's cleanup is through, the fsm code wants to also deallocate
the child. In these situations I so far need to introduce checks that prevent
the parent from freeing until the children are completely finished with their
actions -- for example, avoid signalling end from the cleanup function or event
handling yet, instead completely rely on the parent_term_event, which is
dispatched only after the child was freed. With a volatile context, when an FSM
instance should go away I could reparent it to the volatile context, it will be
cleaned up once all event handling is through, and I avoid running into double
free by accident. It would probably have to be integrated in the fsm.c
deallocation code.
On Wed, Mar 20, 2019 at 12:54:11PM +0100, Pau Espin Pedrol wrote:
But I have the feeling it really adds unneeded extra
complexity and
scenarios to take care in the code.
On the contrary, it might require some thinking about contexts once at a few
pivotal points -- AFAICT it only becomes complex with wqueues, and I have the
impression we could fairly easily handle this only once in the wqueue API
(reparent when adding to or removing from a wqueue) so that callers are
actually going to be relieved of all thinking about msgb scoping. The benefit
on the other hand: I am quite sure that we still have a whole score of mem
leaks / use after free bugs in lots of error handling code paths, which no-one
has noticed and no-one is fixing. With a volatile talloc context approach we
can swat *all* of those bugs by design. We trade a limited amount of brain work
for, theoretically, an inifnite amount of hidden bugs getting fixed.
IMHO we should be fine using regular global context
(and freeing stuff
around) together with static/stack buffers when possible.
These static buffers have so many various ways of shooting yourself in the
foot, it is very desirable to get rid of them. The idea for me is to achieve
less complexity in using the API. A volatile context is the means to that.
...are we talking about a libosmocore 2.0 now?
~N
9:30 a.m.
Hi Neels,
On Thu, Mar 21, 2019 at 02:24:30AM +0100, Neels Hofmeyr wrote:
Talking of features, another idea comes to mind:
freeing of FSM instances. I
more often than not have the problem that a child FSM is freeing, telling the
parent about it, then the parent decides to free because the child's result
signalled some end, and then we have a double free because the child was
allocated with the parent as talloc context, the parent has just deallocated,
but after the child's cleanup is through, the fsm code wants to also deallocate
the child.
That just sounds like a bug and it should be fixed? The question is whether
we want to mandate all child contexts to be allocated from within a parent. If
so, then whatever mechanism to prevent this could / should become part
of the fsm core.
In these situations I so far need to introduce checks
that prevent
the parent from freeing until the children are completely finished with their
actions -- for example, avoid signalling end from the cleanup function or event
handling yet, instead completely rely on the parent_term_event, which is
dispatched only after the child was freed.
I wonder if those checks could be made part of the core. Do you have a
concrete code example to look at?
With a volatile context, when an FSM instance should
go away I could
reparent it to the volatile context, it will be cleaned up once all
event handling is through, and I avoid running into double free by
accident. It would probably have to be integrated in the fsm.c
deallocation code.
that's of course also an option. It actually reminds me a bit of RCU
(read-copy-update)
in the Linux kernel, where a similar 'delay any actual free() until all possible
users have moved beyond a given 'scheduling point' is used as part of a rather
ingenious and complex lock-less synchronization mechanism.
...are we talking about a libosmocore 2.0 now?
I think the volatile context alone is not sufficient for such a large
jump. But let's see where we end up.
--
- Harald Welte <laforge(a)gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
24 Mar
24 Mar
12:43 a.m.
On Thu, Mar 21, 2019 at 09:30:28AM +0100, Harald Welte wrote:
Hi Neels,
On Thu, Mar 21, 2019 at 02:24:30AM +0100, Neels Hofmeyr wrote:
Just after writing the above, I have introduced another deallocation failure
mode in the cleanup among RTP-stream/call leg/MGW endpoint/MSC subscriber FSMs,
and am fed up with solving these ad hoc as they show up.
So now I am writing an explicit FSM deallocation test, to figure out how to
properly solve various cleanup scenarios -- so that I can hopefully apply one
and the same mechanism to all FSM implementations.
(This might get a bit complex to follow, a proposed solution is in the bottom.)
Here is the test: branch neels/fsm_dealloc_test of libosmocore
http://git.osmocom.org/libosmocore/commit/?h=neels/fsm_dealloc_test&id=…
The "scene" struct shows a number of objects, but have reduced to only three
being in use.
branch0 <-parent/child-> twig0a
\ /
----- other -------
So both a parent and child FSM reference some "other" object which they depend
upon.
(A real world equivalent is that both a call_leg and rtp_stream FSM depend on
the MGW endpoint -- in which situation also all of those are children of an MSC
subscriber FSM, just saying complexity is easily added in real life)
The underlying problem here is:
- when the child has an error and goes away, the parent should go away as
well. So there is an event from the child to the parent saying "I am gone,
you too go now": EV_CHILD_GONE.
- if now I tell branch0 to deallocate regularly, it first tells the child to
deallocate. The child then also says EV_CHILD_GONE.
- The parent receives EV_CHILD_GONE and again triggers its own deallocation.
Now there are two osmo_fsm_inst_term() called on the same FSM instance.
A way to fix this is to introduce a ST_DESTROYING state. It acts as a flag to
prevent re-triggering an osmo_fsm_inst_term(). So if as soon as once in
ST_DESTROYING, it will no longer issue osmo_fsm_inst_term() on itself.
My test models this structure.
In the test code try in turns to deallocate with a EV_DESTROY event, and then a
direct osmo_fsm_inst_term(). Both of them should ideally be safe to do,
because on weird errors anything should be able to just term an FSM instance.
Dispatching an EV_DESTROY actually works out fine.
But calling osmo_fsm_inst_term() directly fails.
The difference is that in the EV_DESTROY case, the branch0 FSM has changed to
state ST_DESTROYING, and if it receives events that peer objects are gone, it
ignores them. In the term case, the FSM instance stays in the "ALIVE" state, in
which case a term triggered at the child twig0a in turn dispatches an
EV_CHILD_GONE, which the parent branch0 acts upon by transitioning to the
ST_DESTROYING state, which launches a *second* osmo_fsm_inst_term() on itself.
The output looks like this -- first the successful run, then the failing one:
▶ ./fsm_dealloc_test
DLGLOBAL DEBUG scene_alloc()
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0)
DLGLOBAL DEBUG test(other){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0
DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a
DLGLOBAL DEBUG scene_alloc()
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0)
DLGLOBAL DEBUG test(other){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0
DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a
DLGLOBAL DEBUG scene_alloc()
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0)
DLGLOBAL DEBUG test(other){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0
DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a
DLGLOBAL DEBUG ---------------------- test_destroy(branch0)
DLGLOBAL DEBUG --- before destroy cascade, got:
branch0
twig0a
other
DLGLOBAL DEBUG ---
DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_DESTROY
DLGLOBAL DEBUG 1 (branch0.alive())
DLGLOBAL DEBUG test(branch0){alive}: alive(EV_DESTROY)
DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying
DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter())
DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(branch0){destroying}: pre_term()
DLGLOBAL DEBUG test(twig0a){alive}: Terminating (cause = OSMO_FSM_TERM_PARENT)
DLGLOBAL DEBUG test(twig0a){alive}: pre_term()
DLGLOBAL DEBUG test(twig0a){alive}: Removing from parent test(branch0)
DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup())
DLGLOBAL DEBUG test(twig0a){alive}: cleanup()
DLGLOBAL DEBUG test(twig0a){alive}: scene forgets twig0a
DLGLOBAL DEBUG test(twig0a){alive}: removing reference twig0a.other[0] -> other
DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE
DLGLOBAL DEBUG 4
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE)
DLGLOBAL DEBUG 5
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.other_gone())
DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a
DLGLOBAL DEBUG 4
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: state_chg to destroying
DLGLOBAL DEBUG 5
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(other){destroying}: pre_term()
DLGLOBAL DEBUG 6
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup()
DLGLOBAL DEBUG test(other){destroying}: scene forgets other
DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0
DLGLOBAL DEBUG test(branch0){destroying}: Received Event EV_OTHER_GONE
DLGLOBAL DEBUG 7
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc
DLGLOBAL DEBUG test(branch0){destroying}: destroying(EV_OTHER_GONE)
DLGLOBAL DEBUG 8
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc
DLGLOBAL DEBUG test(branch0){destroying}: Dropped reference branch0.other[0] = other
DLGLOBAL DEBUG 7
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branc
DLGLOBAL DEBUG 6
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup() done
DLGLOBAL DEBUG 5
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: Freeing instance
DLGLOBAL DEBUG test(other){destroying}: Deallocated
DLGLOBAL DEBUG 4
(branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),twig0a.cleanup())
DLGLOBAL DEBUG test(twig0a){alive}: cleanup() done
DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter())
DLGLOBAL DEBUG test(twig0a){alive}: Freeing instance
DLGLOBAL DEBUG test(twig0a){alive}: Deallocated
DLGLOBAL DEBUG 3 (branch0.alive(),branch0.destroying_onenter(),branch0.cleanup())
DLGLOBAL DEBUG test(branch0){destroying}: cleanup()
DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0
DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done
DLGLOBAL DEBUG 2 (branch0.alive(),branch0.destroying_onenter())
DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance
DLGLOBAL DEBUG test(branch0){destroying}: Deallocated
DLGLOBAL DEBUG 1 (branch0.alive())
DLGLOBAL DEBUG 0 (-)
DLGLOBAL DEBUG --- after destroy cascade, still got: <------ SUCCESS!
DLGLOBAL DEBUG --- cleaning up
DLGLOBAL DEBUG scene_alloc()
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: is child of test(branch0)
DLGLOBAL DEBUG test(other){alive}: Allocated
DLGLOBAL DEBUG test(branch0){alive}: branch0.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[0] = branch0
DLGLOBAL DEBUG test(twig0a){alive}: twig0a.other[0] = other
DLGLOBAL DEBUG test(other){alive}: other.other[1] = twig0a
DLGLOBAL DEBUG ---------------------- test_term(branch0)
DLGLOBAL DEBUG --- before term cascade, got:
branch0
twig0a
other
DLGLOBAL DEBUG ---
DLGLOBAL DEBUG test(branch0){alive}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
<------------ branch0 terminating
DLGLOBAL DEBUG test(branch0){alive}: pre_term()
DLGLOBAL DEBUG test(twig0a){alive}: Terminating (cause = OSMO_FSM_TERM_PARENT)
DLGLOBAL DEBUG test(twig0a){alive}: pre_term()
DLGLOBAL DEBUG test(twig0a){alive}: Removing from parent test(branch0)
DLGLOBAL DEBUG 1 (twig0a.cleanup())
DLGLOBAL DEBUG test(twig0a){alive}: cleanup()
DLGLOBAL DEBUG test(twig0a){alive}: scene forgets twig0a
DLGLOBAL DEBUG test(twig0a){alive}: removing reference twig0a.other[0] -> other
DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE
DLGLOBAL DEBUG 2 (twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE)
DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.other_gone())
DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a
DLGLOBAL DEBUG 2 (twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: state_chg to destroying
DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(other){destroying}: pre_term()
DLGLOBAL DEBUG 4
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup()
DLGLOBAL DEBUG test(other){destroying}: scene forgets other
DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0
DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_OTHER_GONE
DLGLOBAL DEBUG 5
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive())
DLGLOBAL DEBUG test(branch0){alive}: alive(EV_OTHER_GONE)
DLGLOBAL DEBUG 6
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.other_gone())
DLGLOBAL DEBUG test(branch0){alive}: Dropped reference branch0.other[0] = other
DLGLOBAL DEBUG 5
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive())
DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying
DLGLOBAL DEBUG 6
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter())
DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
<-------- branch0 terminating AGAIN!?!?
DLGLOBAL DEBUG test(branch0){destroying}: pre_term()
DLGLOBAL DEBUG 7
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter(),branc
DLGLOBAL DEBUG test(branch0){destroying}: cleanup()
DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0
DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done
DLGLOBAL DEBUG 6
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive(),branch0.destroying_onenter())
DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance
DLGLOBAL DEBUG test(branch0){destroying}: Deallocated
DLGLOBAL DEBUG 5
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0.alive())
DLGLOBAL DEBUG 4
(twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup() done
DLGLOBAL DEBUG 3 (twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: Freeing instance
DLGLOBAL DEBUG test(other){destroying}: Deallocated
=================================================================
==9774==ERROR: AddressSanitizer: heap-use-after-free on address 0x612000001128 at pc
0x7f74d8b8f249 bp 0x7ffebeffe490 sp 0x7ffebeffe488
WRITE of size 8 at 0x612000001128 thread T0
#0 0x7f74d8b8f248 in __llist_del
../../../src/libosmocore/include/osmocom/core/linuxlist.h:114
#1 0x7f74d8b8f380 in llist_del
../../../src/libosmocore/include/osmocom/core/linuxlist.h:126
#2 0x7f74d8b93eaa in osmo_fsm_inst_free ../../../src/libosmocore/src/fsm.c:404
#3 0x7f74d8b9ba9c in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:738
#4 0x563b17db94b5 in destroying_onenter
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173
#5 0x7f74d8b987be in state_chg ../../../src/libosmocore/src/fsm.c:521
#6 0x7f74d8b98870 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577
#7 0x563b17db8db8 in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:147
#8 0x7f74d8b99e2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685
#9 0x563b17dbabc0 in cleanup
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:251
#10 0x7f74d8b9b292 in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:733
#11 0x7f74d8b9c1b1 in _osmo_fsm_inst_term_children
../../../src/libosmocore/src/fsm.c:784
#12 0x7f74d8b9a85e in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:720
#13 0x563b17dbd09d in obj_term
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:392
#14 0x563b17dbd8fb in test_term
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:425
#15 0x563b17dbdb62 in main ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:453
#16 0x7f74d7db909a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
#17 0x563b17db5319 in _start
(/n/s/dev/make/libosmocore/tests/fsm/fsm_dealloc_test+0x10319)
Fixing this particular run can be done by manually setting this fi->state =
ST_DESTROYING (without osmo_fsm_inst_state_chg(), since that would run the
onenter() function and trigger an osmo_fsm_inst_term()).
So I did:
void pre_term(struct osmo_fsm_inst *fi, enum osmo_fsm_term_cause cause)
{
+ /* sneak into "destroying" state without triggering events */
+ fi->state = ST_DESTROYING;
LOGPFSML(fi, LOGL_DEBUG, "%s()\n", __func__);
}
That's not so nice, changing states in a cheating way.
A different solution could be some fsm.c internal "already terminating" flag,
so I can call osmo_fsm_inst_term() any number of times, and only the first call
will have an effect.
One might think then the problem is solved?
But then I still face this problem:
- If the child is sent an EV_DESTROY, it goes into osmo_fsm_inst_term().
- As part of its cleanup, it terminates its reference to the "other" object.
- Out of its own free will, the other object then decides that this means some
problem appeared and decides to deallocate as well.
- But the parent object also has a reference to "other". Since "other"
is
deallocating, it notifies the parent branch0 that it is now gone.
- branch0 decides that if "other" has a problem, it is no longer useful and
deallocates.
- twig0a is *not* signalled to terminate from the parent branch0, because it
has already removed itself from the parent at the start of its termination.
- But branch0 calls talloc_free() on itself. Since twig0a was a talloc child of
branch0, its memory storage is now gone.
- All of the above happened while telling "other" that the twig0a is about to
go. Now this code path wants to continue after the twig0a.cleanup(). Alas,
its memory is no longer valid and the fsm.c termination code causes a
use-after-free.
Looks like this:
DLGLOBAL DEBUG ---------------------- test_destroy(twig0a)
DLGLOBAL DEBUG --- before destroy cascade, got:
branch0
twig0a
other
DLGLOBAL DEBUG ---
DLGLOBAL DEBUG test(twig0a){alive}: Received Event EV_DESTROY
DLGLOBAL DEBUG 1 (twig0a.alive())
DLGLOBAL DEBUG test(twig0a){alive}: alive(EV_DESTROY)
DLGLOBAL DEBUG test(twig0a){alive}: state_chg to destroying
DLGLOBAL DEBUG 2 (twig0a.alive(),twig0a.destroying_onenter())
DLGLOBAL DEBUG test(twig0a){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(twig0a){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(twig0a){destroying}: pre_term()
DLGLOBAL DEBUG test(twig0a){destroying}: Removing from parent test(branch0)
DLGLOBAL DEBUG 3 (twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup())
DLGLOBAL DEBUG test(twig0a){destroying}: cleanup()
DLGLOBAL DEBUG test(twig0a){destroying}: scene forgets twig0a
DLGLOBAL DEBUG test(twig0a){destroying}: removing reference twig0a.other[0] -> other
<----- twig0a tells "other" that it is going
DLGLOBAL DEBUG test(other){alive}: Received Event EV_OTHER_GONE
DLGLOBAL DEBUG 4
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: alive(EV_OTHER_GONE)
DLGLOBAL DEBUG 5
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.other_gone())
DLGLOBAL DEBUG test(other){alive}: Dropped reference other.other[1] = twig0a
DLGLOBAL DEBUG 4
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive())
DLGLOBAL DEBUG test(other){alive}: state_chg to destroying
DLGLOBAL DEBUG 5
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(other){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(other){destroying}: pre_term()
DLGLOBAL DEBUG 6
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup()
DLGLOBAL DEBUG test(other){destroying}: scene forgets other
DLGLOBAL DEBUG test(other){destroying}: removing reference other.other[0] -> branch0
DLGLOBAL DEBUG test(branch0){alive}: Received Event EV_OTHER_GONE
<----- branch0 gets told "other" is gone
DLGLOBAL DEBUG 7
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){alive}: alive(EV_OTHER_GONE)
DLGLOBAL DEBUG 8
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){alive}: Dropped reference branch0.other[0] = other
DLGLOBAL DEBUG 7
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){alive}: state_chg to destroying
DLGLOBAL DEBUG 8
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){destroying}: destroying_onenter() from alive
DLGLOBAL DEBUG test(branch0){destroying}: Terminating (cause = OSMO_FSM_TERM_REGULAR)
DLGLOBAL DEBUG test(branch0){destroying}: pre_term()
DLGLOBAL DEBUG 9
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){destroying}: cleanup()
DLGLOBAL DEBUG test(branch0){destroying}: scene forgets branch0
DLGLOBAL DEBUG test(branch0){destroying}: cleanup() done
DLGLOBAL DEBUG 8
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG test(branch0){destroying}: Freeing instance
DLGLOBAL DEBUG test(branch0){destroying}: Deallocated
<----- branch0 calls talloc_free()
DLGLOBAL DEBUG 7
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup(),branch0
DLGLOBAL DEBUG 6
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter(),other.cleanup())
DLGLOBAL DEBUG test(other){destroying}: cleanup() done
DLGLOBAL DEBUG 5
(twig0a.alive(),twig0a.destroying_onenter(),twig0a.cleanup(),other.alive(),other.destroying_onenter())
DLGLOBAL DEBUG test(other){destroying}: Freeing instance
DLGLOBAL DEBUG test(other){destroying}: Deallocated <----
twig0a.cleanup() is done, fsm.c wants to continue termination...
=================================================================
==16426==ERROR: AddressSanitizer: heap-use-after-free on address 0x6120000015a8 at pc
0x7faea5cb4149 bp 0x7fff936ffed0 sp 0x7fff936ffec8
WRITE of size 8 at 0x6120000015a8 thread T0
#0 0x7faea5cb4148 in __llist_del
../../../src/libosmocore/include/osmocom/core/linuxlist.h:114
#1 0x7faea5cb4280 in llist_del
../../../src/libosmocore/include/osmocom/core/linuxlist.h:126
#2 0x7faea5cb8daa in osmo_fsm_inst_free ../../../src/libosmocore/src/fsm.c:404
#3 0x7faea5cc099c in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:738
#4 0x56367132a4c5 in destroying_onenter
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173
#5 0x7faea5cbd6be in state_chg ../../../src/libosmocore/src/fsm.c:521
#6 0x7faea5cbd770 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577
#7 0x563671329dc8 in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:147
#8 0x7faea5cbed2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685
#9 0x56367132bbd0 in cleanup
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:251
#10 0x7faea5cc0192 in _osmo_fsm_inst_term ../../../src/libosmocore/src/fsm.c:733
#11 0x56367132a4c5 in destroying_onenter
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:173
#12 0x7faea5cbd6be in state_chg ../../../src/libosmocore/src/fsm.c:521
#13 0x7faea5cbd770 in _osmo_fsm_inst_state_chg ../../../src/libosmocore/src/fsm.c:577
#14 0x563671329e3d in alive ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:159
#15 0x7faea5cbed2f in _osmo_fsm_inst_dispatch ../../../src/libosmocore/src/fsm.c:685
#16 0x56367132e0be in obj_destroy
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:389
#17 0x56367132e519 in test_destroy
../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:408
#18 0x56367132ebf4 in main ../../../src/libosmocore/tests/fsm/fsm_dealloc_test.c:454
#19 0x7faea4edf09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
#20 0x563671326329 in _start
(/n/s/dev/make/libosmocore/tests/fsm/fsm_dealloc_test+0x10329)
So, even though twig0a sends the EV_CHILD_GONE to its parent FSM only *after*
all terminating is done, some events crossing over via "other" objects can
still trigger the parent to also deallocate at the same time.
Here the problem merely is that the child is allocated from the parent's talloc
context. Had it been allocated from an independent talloc context, then there
would not be any use-after-free. So let's try that:
fi = osmo_fsm_inst_alloc_child(&test_fsm, parent->fi, EV_CHILD_GONE);
OSMO_ASSERT(fi);
+ talloc_steal(s, fi);
Now the next problem arises: when the child is done deallocating, fsm.c still
has the parent FSM pointer to which it must still dispatch the parent_term
event.
When there is only a parent and a child involved, using the parent_term event
actually solves this problem: if we were to dispatch EV_CHILD_GONE from the
child's cleanup() function, then the parent would deallocate and free the child
along with its own talloc context. By using the parent_term event, we wait
until after cleanup() and is done and until after talloc_free() on the child is
done, and only then dispatch EV_CHILD_GONE to the parent.
But because a third "other" object is involved, the deallocation triger
ricochets across that other object. It is out of scope to tell that other
object to care about which two callers are using it and why they must not be
told to deallocate in certain situations. It is certainly possible to do that,
but it requires a lot of careful thinking through all possible variations,
which is very easy to get wrong.
The vision for fixing these kinds of situations is to avoid a use-after-free
completely by keeping the talloc contexts around; Also all event dispatch and
osmo_fsm_inst_term() calls must be stopped when a fi is marked to be released.
- add flag osmo_fsm_inst.deallocating. If true, fsm.c denies all action
triggers on the FSM instance (no osmo_fsm_inst_term(), no
osmo_fsm_inst_state_chg(), no osmo_fsm_inst_dispatch()).
- add a talloc bucket into which to reparent FSM instances instead of
deallocating directly. Then each parent/child/"other" object that wants to
trigger things on it will be able to see the "deallocating" flag, and still
even use memory like the fi->id for logging. This talloc bucket could be a
volatile select context, or it could even be a global context that exists
once in fsm.c, the first osmo_fsm_inst_term() creates it, other callers add
and remove use counts, and as soon as the last use count is removed, the
entire context gets freed.
I think this would solve all of the above problems.
~N
Talking of features, another idea comes to mind:
freeing of FSM instances. I
more often than not have the problem that a child FSM is freeing, telling the
parent about it, then the parent decides to free because the child's result
signalled some end, and then we have a double free because the child was
allocated with the parent as talloc context, the parent has just deallocated,
but after the child's cleanup is through, the fsm code wants to also deallocate
the child.
That just sounds like a bug and it should be fixed? The question is whether
we want to mandate all child contexts to be allocated from within a parent. If
so, then whatever mechanism to prevent this could / should become part
of the fsm core.
In these situations I so far need to introduce
checks that prevent
the parent from freeing until the children are completely finished with their
actions -- for example, avoid signalling end from the cleanup function or event
handling yet, instead completely rely on the parent_term_event, which is
dispatched only after the child was freed.
I wonder if those checks could be made part of the core. Do you have a
concrete code example to look at?
6:54 a.m.
This patch (of course) still was buggy, and now I can actually also add a
working general solution and a bit more insight.
At first I was really annoyed by this deallocation spaghetti distracting me yet
another time from breaking through to inter-MSC HO; I was already questioning
all the nice and logical FSM references I had designed in osmo-msc, even
contemplated just running off and letting someone else solve it... But now I am
quite glad that I took a closer look, because with this patch I can even remove
some events and states (maybe drop some FSM instances entirely, which were only
introduced to receive a parent_term event), while actually becoming safer than
before and having to do almost no thinking to achieve that.
The new fsm_dealloc_test.c and an improvement to fsm.c is pushed at
branch neels/fsm_dealloc_test.
http://git.osmocom.org/libosmocore/log/?h=neels/fsm_dealloc_test
The first patch of three shows the current situation totally not working out.
The second patch applies fsm.c "fixes" and shows all situations magically
working well.
The third patch simplifies fsm_dealloc_test.c, because it no longer needs the
ST_DESTROYING after the new safeguards are in place. Nice.
So far I am talloc_steal()ing FSM instances "freed" in osmo_fsm_inst_term()
cascades to the first/outermost osmo_fsm_inst_term() fi as talloc parent, so
that all get freed once in the end.
Instead, "freed" instances could be reparented to a future thread volatile
select context once it shows up. For now I'm very glad that I can easily fix
my osmo-msc without having to depend on the select volatile ctx.
- add flag osmo_fsm_inst.deallocating. If true, fsm.c
denies all action
triggers on the FSM instance (no osmo_fsm_inst_term(), no
osmo_fsm_inst_state_chg(), no osmo_fsm_inst_dispatch()).
Actually it is only necessary to avoid re-entering osmo_fsm_inst_term() for the
same FSM instance.
- Dispatching events is fine: if FSM implementations require thwarting events
when terminating, the event handlers can simply test for the new
fi->proc.terminating flag; also, some FSM implementations may actually rely
on receiving events while already terminating, e.g. to dereference other
deallocating objects and not attempt to clean those twice.
- Changing state during osmo_fsm_inst_term() is also fine, along the same
reasoning.
~N
25 Mar
25 Mar
2:29 p.m.
On Sun, Mar 24, 2019 at 06:54:15AM +0100, Neels Hofmeyr wrote:
So far I am talloc_steal()ing FSM instances
"freed" in osmo_fsm_inst_term()
cascades to the first/outermost osmo_fsm_inst_term() fi as talloc parent, so
that all get freed once in the end.
Interesting to note here is that I can apparently steal a talloc parent to
become a talloc child (this is the result of the child osmo_fsm_inst_term()
also causing the parent to term)
parent_fi
|
+- child_fi
talloc_steal(new_ctx=child_fi, parent_fi)
I guess this should result in:
child_fi
|
+- parent_fi
I'm not entirely clear how this works out. Are those then still attached to
whatever parent_fi had as a parent context? Are they floating alone?
The test shows that it does work, no leaks, no loops.
Since child_fi is guaranteed to be deallocated, either way would be fine. If
this is bugging us we can use a different talloc_ctx to steal into -- I just
wanted to avoid allocating another short-lived ctx.
~N
20 Mar
20 Mar
7:01 p.m.
On Wed, Mar 20, 2019 at 10:52:46AM +0100, Harald Welte wrote:
To Neels' disappointment, this is not all
automatic.
Flabbergasted is the word. I thought things would get easier instead of more
convoluted and bloated.
To me clearly the drawbacks of making it explicit are humungous and cumbersome
while avoiding fixing hidden bugs plus keeping all static buffers around.
Making it implicit has only positive effects AFAICT.
My bottom line is that I absolutely full on don't understand why we would
volunarily burden ourselves with this huge patch writing overhead and code
bloat, now and into the future. We could so trivially collapse all of it.
My single question is, why. What is the reason for accepting this huge tail?
(ง •̀_•́)ง -- why?
If you're interested, here are my opinions in detail...
a) have to call the _c suffix of the respective
function, e.g. osmo_hexdump_c()
instead of osmo_hexdump() with one extra initial argument:
OTC_SELECT. There's also msgb_alloc_c(), msgb_copy_c() and the like,
allowing msgb allocation from a context of your choice, as opposed
to the library-internal msgb_tall_ctx that we had so far.
These are the problems I have with this:
- complexity: constantly have to take care to choose a talloc context, mostly
burdens writing log statements:
- single-threaded program: Is there a global pointer to a main()
volatile talloc context we can always use? Ok, OTC_SELECT is passed
everywhere.
- multi-threaded program: Is OTC_SELECT implicitly distinct per thread? Then
multi-threaded programs pass OTC_SELECT everywhere.
- So everyone always passes OTC_SELECT everywhere, then what's the use of the
ctx argument.
In the super rare cases where something like osmo_plmn_name() wants to
allocate from a different context, I would just do a talloc_strcpy() to the
other context.
- coding style:
- bloat: we need to pass the same ctx argument to all those name() functions.
That makes it longer, harder to read, more to type.
With the implicit solution:
LOGP(DMSC, LOGL_ERROR, "Peer's PLMN has changed: was %s, now is %s.
MSC's PLMN is %s\n",
osmo_plmn_name(&parsed_plmn), osmo_plmn_name(&vsub->cgi.lai.plmn),
osmo_plmn_name(&net->plmn));
With the explicit solution:
LOGP(DMSC, LOGL_ERROR, "Peer's PLMN has changed: was %s, now is %s.
MSC's PLMN is %s\n",
osmo_plmn_name_c(OTC_SELECT, &parsed_plmn), osmo_plmn_name_c(OTC_SELECT,
&vsub->cgi.lai.plmn),
osmo_plmn_name_c(OTC_SELECT, &net->plmn));
Would irritate me a lot. So I would usually just use the old ones instead.
- complexity: then we are again in the situation where, when writing new code,
I have to consciously decide where to allocate the strings from. That's
something I wanted to get rid of. It would be so relieving to just know:
"osmo_plmn_name() is safe" and just spam on using it without extra args,
without the first call being osmo_plmn_name() and the second call
osmo_plmn_name_c(OTC_SELECT, plmn), and then oh damn, I forgot that the
third one also has to be _c, or some indirectly called function also fails
to use the _c variant... If there is just one osmo_plmn_name() I cannot
possibly do anything wrong, no hidden bugs anywhere, by design.
- less arguments, less signatures: The reason why we have value_string[]
shortcuts like osmo_gsup_message_type_name() is so that we can save an
extra argument (the names[] array) when writing log statements. We didn't
want to add so many bin symbols to the API just for that, so we agreed on
using static inline functions in the .h files. Fair enough. So each
value_string[] definition currently has a tail of such a static inline
function attached. With this proposal we're re-adding a second static
inline _c variant to each and every value_string[] tail, with another
function arg again added to the _c function signature.
We will also need a get_value_string_c() function because for unknown
values it uses a static buffer to return a number string. We can so
trivially avoid all of this mess.
- less bugs: get_value_string() is a prime example for wanting to fix things
implicitly. If a log statement uses two get_value_string() based
implementations, even for completely different names, then it would print
nonsense if both hit an undefined value.
Pseudo code example:
LOGP(DRSL, LOGL_ERROR, "Unknown message: proto %s message %s\n",
proto_name(h->proto), msg_type_name(h->msg_type));
If there is both an unkown proto 42 and msg_type 23, then only one of the
"unknown 42" result strings from get_value_string() survives. It would say
"Unknown message: proto unknown (42) message unknown (42)"
These are hidden bugs which we would completely eradicate by making
distinct buffer use implicit. It is not always obvious which functions use
get_value_string() in their implementation. So we should use _c functions
always. So then why even have them as distinct signatures.
- static buffers: We have quite a number of static char[] around the code base,
my idea was that we could get rid of those. With this proposal we have to
keep them.
- API bloat:
- This adds a lot of new API signatures to the existing ones. Both static
inline for value_string[], and bin symbols in the library files for things
like osmo_plmn_name_c().
- Each future function needs a _c variant added to it. We may forget them,
then more discussions on gerrit. More patches to fix that. I would rather
avoid that by design.
I don't understand why you want to do it this way. What is the justification
for all this bloat, required patches now and future complexity in writing code?
b) have to use osmo_select_main_ctx() instead of
osmo_select_main(). This is
an arbitrary limitation for optimization that Pau requested, to make sure
applications that don't want this can avoid all the extra talloc+free at
every select iteration.
IMO that is the wrong place to hinge the argument.
- If nothing used the volatile context, nothing needs to be freed.
- Applications wanting to avoid dynamic allocations can avoid calling functions
that allocate dynamically. Admitted, with my under-the-hood proposal even
functions like osmo_plmn_name() would do dynamic allocations, but:
- In the real world, I don't know why you would want to avoid dynamic
allocations. AFAICT the performance impact is basically non-existent. How
else do we get away with dynamically allocating *all* of our msgb and even
copying them around quite often? We also dynamically allocate user contexts,
transaction structs, basically all llist entries, strings that objects
reference,...
- And finally, if dynamic allocations are identified as a problem, then there
are ways to avoid them: keep N "volatile" buffers around, re-use them across
osmo_select() iterations without freeing in-between. A dedicated function to
return volatile buffers could take care of that.
I mean instead of passing a specific ctx, rather call a separate function
osmo_volatile_buffer(size), so we can slip in an optimisation like that at
any point in the future.
- Finally finally, optimisations can be done by talloc, the kernel and whatnot,
and I'm fairly sure there is a lot of thought going into memory management
and optimising dynamic allocations so that applications can completely skip
this argument, which I propose we do.
- And triple finally, switch off debug logging to completely cut out almost all
allocations for logging. If the performance argument holds, the improvement
should be dramatic. So far a dramatic improvement is seen when switching off
debug logging for low-level logging categories on a sysmobts, for osmo-bts
and osmo-pcu, but that isn't related to dynamic allocations, much rather to
plain fprintf() I/O -- which is much more perf critical than de/allocating.
This is debatable, and we can make it automatic
in osmo_select_main().
+1
It's probably worth a benchmark how expensive that
'empty' allocation + free is.
If some (inline?) osmo_volatile_buffer() function creates such a talloc context
only on demand, then I don't see how this can possibly have any impact at all.
However, I think that's a rather "OK"
price to pay. Converting the existing
callers can be more or less done with "sed" (yes, spatch is of course better).
ugh really; I still argue that the API is then bloated, and more importantly
writing new code becomes more complex and more bloated as well.
I don't really want to live in that API.
While introducing this feature, I also tried to
address two other topics, which
are not strictly related. However, ignoring those two now would mean
we'd have API/ABI breaks if we'd care about them in the future. Hence I
think we should resolve them right away:
1) introduce another context: OTC_GLOBAL. This is basically a library-internal
replacement for the "g_tall_ctx" that we typically generate as one of the
first things in every application. You can use it anywhere where you'd want
to allocate something form the global talloc context
Nice.
Shouldn't the name start with OSMO_ though?
2) make those contexts thread-local.
That's pretty cool.
Ok, so we already have distinct volatile talloc contexts per thread, implicitly.
Then why would you still want to pass this context to each function call explicitly?
[[
Trying to read up on __thread, I noticed there are portability / C version
considerations. This macro, if we remove the windows bit, looks interesting:
https://stackoverflow.com/questions/18298280/how-to-declare-a-variable-as-t…
i.e.
#ifndef thread_local
# if __STDC_VERSION__ >= 201112 && !defined __STDC_NO_THREADS__
# define thread_local _Thread_local
# elif defined __GNUC__ || \
defined __SUNPRO_C || \
defined __xlC__
# define thread_local __thread
# else
# error "Cannot define thread_local"
# endif
#endif
?
]]
~N
1870
days inactive
1875
days old
9 comments
3 participants
participants (3)
-
Harald Welte -
Neels Hofmeyr -
Pau Espin Pedrol