[sr-dev] shm_lock in free_cell()

[Miklos Tirpak]

Hi Jan,

On 09/14/2009 01:48 PM, Jan Janak wrote:
> Hi Miklos,
> 
> On 10-09 18:42, Miklos Tirpak wrote:
>> Hi Jan,
>>
>> when free_cell() frees the memory of a transaction the shm memory lock  
>> is already held:
>>
>> shm_lock();
>> ...
>> /* callbacks */
>> for( cbs=(struct tm_callback*)dead_cell->tmcb_hl.first ; cbs ; ) {
>> 	cbs_tmp = cbs;
>> 	cbs = cbs->next;
>> 	if (cbs_tmp->release) {
>> 		cbs_tmp->release(cbs_tmp->param);
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> I think this can cause a dead-lock because the release function is not  
>> aware of the state of the shm mem lock.
> 
> I am not sure I understand. The release function assumes that it is called
> from within the lock, so it knows about it, right?

yes, the release function should know about this fact however I would 
rather not depend on this. The module writers have to be aware of this, 
shall not forget it, the release function may be quite complex, ... so I 
think it is just safer to call the release func outside of the locks.

> 
>> 	}
>> 	shm_free_unsafe( cbs_tmp );
>> }
>>
>> I saw that you have added this function call, but I cannot found any  
>> customer of this function in the repository, so I do not know whether  
>> the cb functions use safe or unsafe shm_free(). Do you know anything  
>> about this?
> 
> The release function was added there on request. It was needed by the kamailio
> version of dialog module. See modules_k/dialog, function unref_new_dialog is
> the handle that is called from the code above.
> 
> Unfortunately I do not know much about the dialog module.

Thanks for the pointer. This is what I was afraid of, it seems that 
unref_new_dialog() does not use the unsafe version of shm_free. It 
unrefs the dialog that may be freed, so it uses shm_free regardless of 
whether or not the shm lock is held.

> 
>> I recently added another place where this cb is called from (without  
>> locking), hence I think it would be better to move this outside of the  
>> shm mem lock to be on the safe side.
> 
> Why is the shm_lock there? Is this some sort of performance optimization to
> ensure that as little as possible is done with the cell lock being?

I do not think that it is related to the cell lock but rather to the 
lock of the shm memory allocator. Lots of memory needs to be freed, 
hence I assume Andrei decided to lock, free all the stuff, and than 
unlock, so the frequent lock-unlock is not needed.

> 
> I think you are right that the release callabacks should not be executed with
> the shm_lock being held, because the callbacks can execute arbitrary complex
> functions and those functions might not be aware that they need to use the
> non-locking version of shm related functions.

Indeed.

> 
> So feel free to change it it is safe.

Ok, I will.

Thanks,
Miklos

>   
>    Jan.