RFC: talloc contexts / automatic free at select loop
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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@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
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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
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@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
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:
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?
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=31ed1... 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
http://git.osmocom.org/libosmocore/commit/?h=neels/fsm_dealloc_test&id=31ed1...
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
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
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-th... 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
participants (3)
-
Harald Welte -
Neels Hofmeyr -
Pau Espin Pedrol