lchan_free() is called when RSL released the logical channel. all ressources that 'uses' the channel, must be released. i aggree with you, that it makes no sense to consider everything that 'uses' the logical channel. to fix this, i have an idea and need some response before implementing.
problems: - what instance uses the logical channel? - how do we release them, if the channel is closed? - how do we change the channel (handover/SDCCH->TCH ...)? (is this required?)
one possible solution:
every logical channel gets a list of 'users'. these users have nodes in this list. users can be i.e.: an active call, a call on hold, an SMS transaction, a location update....
each node has a pointer to the lchan. the 'user' can access the logical channel via node.
each node has a destructor function pointer. during lchan_free(), the destructor of each node is called. the 'user' will remove the node. with that, the chan_alloc does not need to know how to free 'users'
if we want to change the logical channel, we just unlink the nodes from the old channel and attach them to the new channel.
each node has: - "struct llist_head" to link to previous and next node - pointer to the logical channel (lchan) - pointer to the destructor for the node owner. (the 'user') - void pointer to the owner's private structure.
user 'uses' a channel: - it calls the lchan_use() function with the logical channel, the destructor, and private structure pointer - the node is created and linked to the logical channel - the pointers within that node are set
user 'drops' a channel: - it calls the lchan_put() function with the pointer to the node. - the node is unlinked from lchan and destroyed
lchan closes: - for each node of a logical channel, the destructor is created. - within this destructor, the 'user' must remove all relations to that channel, remove the node, and set the node pointer to NULL.
user can refers to it's channel: struct gsm_lchan *lchan = private->lchan_node->lchan;
what do you think?
-----Ursprüngliche Nachricht----- Von: openbsc-bounces@lists.gnumonks.org [mailto:openbsc-bounces@lists.gnumonks.org] Im Auftrag von Holger Freyther Gesendet: Dienstag, 2. Juni 2009 03:02 An: openbsc@lists.gnumonks.org Betreff: Re: patch 22: lchan_use
I don't like this part. First of all when making release_loc_update_req "public" it should be properly prefixed, but I don't think chan_alloc.c should know/care about gsm_04_08.c at all. Also tying the timeout of the Location Update with the autorelease of the channel does not seem appropriate.
I would very much prefer if this logic can stay within gsm_04_08.c and we fix the usage count issue there.
For me it looks like: - We get a reference when creating the loc_update_request - We start a timer - We put the reference when destroying the loc update request...
- So we might just remove the extra put/use for the waiting for IMSI/IMEI and fix the "leak" like this?
what do you think?
z.
On Tuesday 02 June 2009 17:04:33 Andreas.Eversberg wrote:
lchan_free() is called when RSL released the logical channel. all ressources that 'uses' the channel, must be released. i aggree with you, that it makes no sense to consider everything that 'uses' the logical channel. to fix this, i have an idea and need some response before implementing.
problems:
- what instance uses the logical channel?
- how do we release them, if the channel is closed?
- how do we change the channel (handover/SDCCH->TCH ...)? (is this
required?)
what do you think?
Let me tell you what I intended for the lchan and what I see is missing.
Normal operation: Basic: - lchan is a logical channel - we use refcount, when refcount drops zero we will release it (arbitrary timeout to delay it)
Logical Operations: - On top of this we have a logical operation/transaction... - E.g. Location Updating Request.... - It has a specified timeout of 20 seconds T3210 (okay that is cheating as this timer is from the mobile station) - During this operation we send a couple of commands, want the IMSI, IMEISV, etc. and ultimately want to end in a CM SERVICE REQUEST (oops... another bug in our code) - But we might not get every command answered, so manipulating the refcount for IMSI, IMEISV, etc. might result in leaks.. - This is where the logical operation comes into place to "hold" the ref throughout the operation and put_lchan once it is done (either by timeout, failure or success)
On the specific bug you see, I think removing use_lchan/put_lchan for the separate IMSI/IMEISV should fix your leak, and should be safe as the creation of the logical operation is getting the lock.
Failure Case: - We get a RSL_MT_RF_CHAN_REL_ACK with refcount != 0 - so the channel got closed under our back... - The question is how to do error recovery in general in OpenBSC
- Your proposal is to have a list of users in each lchan to run "destructors".
- I think something as easy as our callback system can be used. Just broadcast an UNEXPECTED LCHAN RELEASE event and let the client code figure out what resources to release.
So I think with "users" we already have them in the sense of logical operation that can be embedded into the lchan. So far I would opt for pointers in the lchan structure, we can make that a llist once we have too many of them.
But I wouldn't want to mix that with the "error" case, normally the "destructors" of an user are ran when the operation is completed and afterwards the reference gets dropped. The only case this does not happen is the unexpected release of a channel.
what do you think?
-
Andreas.Eversberg -
Holger Freyther