Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release

[Ameya Palande <ameya.palande@nokia.com>]

Hi Ernesto,

ext Ramos Falcon, Ernesto wrote:
> Hi Ameya,
> 
>> -----Original Message-----
>> From: Ameya Palande [mailto:ameya.palande@nokia.com]
>> Sent: Wednesday, August 05, 2009 4:52 PM
>> To: Ramos Falcon, Ernesto
>> Cc: linux-omap@vger.kernel.org; Doyu Hiroshi (Nokia-D/Helsinki); Guzman
>> Lugo, Fernando; Ramirez Luna, Omar; Tereshonkov Roman (Nokia-D/Helsinki);
>> Moogi, Suyog
>> Subject: Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to
>> bridge_release
>>
>> Hi Ernesto,
>>
>> ext Ramos Falcon, Ernesto wrote:
>>> Hi,
>>>
>>> We have detected a use case where if an application creates a child
>> process using fork call, and then the child and father processes call
>> DSPProcessor_Attach() and create a new process context with new tgid; when
>> the processes are terminated, only the last process calls bridge_release
>> cleaning only the resources in the father process, leaving the child
>> resources unreleased.
>>> One solution we have seen is to perform goes through the entire process
>> context list, clean up all the resources for all terminated processes or
>> in "zombie" state, as below,
>>> DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject);
>>> while (pCtxtclosed != NULL) {
>>> 	printk("pCtxtclosed->pid = %d\n",pCtxtclosed->pid);
>>> 	tsk = find_task_by_pid(pCtxtclosed->pid);
>>>
>>> 	if ((tsk == NULL) || (tsk->exit_state == EXIT_ZOMBIE)) {
>>>
>>> 		GT_1trace(driverTrace, GT_5CLASS,
>>> 			"***Task structure not existing for "
>>> 			 "process***%d\n", pCtxtclosed->pid);
>>> 		DRV_RemoveAllResources(pCtxtclosed);
>>> 		if (pCtxtclosed->hProcessor != NULL) {
>>> 					PROC_Detach
>>> 						 (pCtxtclosed->hProcessor);
>>> 		}
>>> 		pTmp = pCtxtclosed->next;
>>> 		DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject,
>>> 					 pCtxtclosed,
>>> 					 (void *)pCtxtclosed->pid);
>>> 	} else {
>>> 		pTmp = pCtxtclosed->next;
>>> 	}
>>> 	pCtxtclosed = pTmp;
>>> }
>>>
>>> Please let me know your comments.
>>>
>>> /Ernesto
>> Good point :)
>>
>> I would like to simplify this use case ;)
>>
>> If we call DSPProcessor_Attach() twice in the same process and kill the
>> process,
>> then it will leak memory for 1st instance of PROCESSOR object.
>>
>> When we call open() on /dev/DspBridge a new PROCESS_CONTEXT is allocated,
>> and it
>> should be allocated **only once** in bridge_open() unlike in
>> NODE_Allocate() and
>> PROC_Attach(). PROCESS_CONTEXT tracks all the resources allocated on
>> behalf of
>> an open file handle(and not the process / thread). When this handle is
>> closed
>> all these resources should be freed in bridge_release(). Accountability of
>> resources should be done using PROCESS_CONTEXT and **not pid (which will
>> be
>> different for different thread) / tgid (which will be different for parent
>> and
>> child).
>>
>> Above problem occurs because PROCESS_CONTEXT by design tracks only one
>> PROCESSOR
>> object which gets freed in bridge_release().
>>
>> Let me know your comments on this, and then we can proceed to fix this
>> issue.
>>
>> Cheers,
>> Ameya.
> You're right; I think using the PROCESS_CONTEXT to track the resources would resolve the issue. Also, with his approach we don't need to create a new context in the PROC_Attach /NODE_Allocate.
> 
> We can solve the issue by implementing a counter to track the number of calls to the PROC_Attach/Detach, so in that way we create a process handle only for the first time, and for the subsequent calls we need to return the existing handle. In the other hand PROC_Detach would be executed for the last call to this function.
> 
> I don't know yet how we would access or if there is an easy way to get the private_data as if get the pid using the "current", though.
> 
> /Ernesto

Thanks for your comment :)
I am working on this and will try to provide a solution by 10th Aug 2009.

Cheers,
Ameya.