* [PATCH 1/3] DSPBRIDGE: Cleanup bridge_release and remove DSP_Close @ 2009-08-05 13:25 Ameya Palande 2009-08-05 13:25 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ameya Palande 0 siblings, 1 reply; 7+ messages in thread From: Ameya Palande @ 2009-08-05 13:25 UTC (permalink / raw) To: linux-omap; +Cc: hiroshi.doyu, x0095840, omar.ramirez, roman.tereshonkov, suyog Signed-off-by: Ameya Palande <ameya.palande@nokia.com> --- arch/arm/plat-omap/include/dspbridge/dspdrv.h | 17 ---------------- drivers/dsp/bridge/rmgr/drv_interface.c | 19 ++--------------- drivers/dsp/bridge/rmgr/dspdrv.c | 26 ------------------------- 3 files changed, 3 insertions(+), 59 deletions(-) diff --git a/arch/arm/plat-omap/include/dspbridge/dspdrv.h b/arch/arm/plat-omap/include/dspbridge/dspdrv.h index f500ffb..3d04967 100644 --- a/arch/arm/plat-omap/include/dspbridge/dspdrv.h +++ b/arch/arm/plat-omap/include/dspbridge/dspdrv.h @@ -50,23 +50,6 @@ #define MAX_DEV 10 /* Max support of 10 devices */ /* - * ======== DSP_Close ======== - * Purpose: - * Called when the client application/driver unloads the DDSP DLL. Upon - * unloading, the DDSP DLL will call CloseFile(). - * Parameters: - * dwDeviceContext: Handle returned by XXX_Open used to identify - * the open context of the device - * Returns: - * TRUE indicates the device is successfully closed. FALSE indicates - * otherwise. - * Requires: - * dwOpenContext!= NULL. - * Ensures:The Application instance owned objects are cleaned up. - */ -extern bool DSP_Close(u32 dwDeviceContext); - -/* * ======== DSP_Deinit ======== * Purpose: * This function is called by Device Manager to de-initialize a device. diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c index 24f6336..8cbdeee 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.c +++ b/drivers/dsp/bridge/rmgr/drv_interface.c @@ -655,22 +655,9 @@ func_cont: * driver. */ static int bridge_release(struct inode *ip, struct file *filp) { - int status; - u32 pid; - - GT_0trace(driverTrace, GT_ENTER, "-> driver_release\n"); - - /* Return PID instead of process handle */ - pid = current->pid; - - status = DSP_Close(pid); - - - (status == true) ? (status = 0) : (status = -1); - - GT_0trace(driverTrace, GT_ENTER, " <- driver_release\n"); - - return status; + GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n"); + GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n"); + return 0; } /* This function provides IO interface to the bridge driver. */ diff --git a/drivers/dsp/bridge/rmgr/dspdrv.c b/drivers/dsp/bridge/rmgr/dspdrv.c index a7a74fc..b5caf39 100644 --- a/drivers/dsp/bridge/rmgr/dspdrv.c +++ b/drivers/dsp/bridge/rmgr/dspdrv.c @@ -248,29 +248,3 @@ bool DSP_Deinit(u32 deviceContext) return retVal; } - -/* - * ======== DSP_Close ======== - * The Calling Process handle is passed to DEV_CleanupProcesState - * for cleaning up of any resources used by the application - */ -bool DSP_Close(u32 dwOpenContext) -{ - bool retVal = false; - - DBC_Require(dwOpenContext != 0); - - GT_0trace(curTrace, GT_ENTER, "Entering DSP_Close\n"); - -#ifdef RES_CLEANUP_DISABLE - - if (DSP_SUCCEEDED(DEV_CleanupProcessState((HANDLE) dwOpenContext))) { - GT_0trace(curTrace, GT_1CLASS, "DSP_Close Succeeded \r\n"); - retVal = true; - } else { - GT_0trace(curTrace, GT_7CLASS, "DSP_Close failed \r\n"); - } -#endif - - return retVal; -} -- 1.6.2.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release 2009-08-05 13:25 [PATCH 1/3] DSPBRIDGE: Cleanup bridge_release and remove DSP_Close Ameya Palande @ 2009-08-05 13:25 ` Ameya Palande 2009-08-05 13:25 ` [PATCH 3/3] DSPBRIDGE: Use TGID instead of PID for resource accounting Ameya Palande 2009-08-05 19:20 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ramos Falcon, Ernesto 0 siblings, 2 replies; 7+ messages in thread From: Ameya Palande @ 2009-08-05 13:25 UTC (permalink / raw) To: linux-omap; +Cc: hiroshi.doyu, x0095840, omar.ramirez, roman.tereshonkov, suyog Instead of doing lazy resource cleanup in bridge_open, this patch will move it to bridge_release. This way the resources allocated will be cleaned up when that instance of the open is closed and will not wait till the next instance of open happens. Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com> Signed-off-by: Ameya Palande <ameya.palande@nokia.com> --- drivers/dsp/bridge/rmgr/drv_interface.c | 132 ++++++++++++------------------- 1 files changed, 52 insertions(+), 80 deletions(-) diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c index 8cbdeee..2fcd6f9 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.c +++ b/drivers/dsp/bridge/rmgr/drv_interface.c @@ -562,102 +562,74 @@ static void __exit bridge_exit(void) platform_driver_unregister(&bridge_driver); } -/* This function is called when an application opens handle to the - * bridge driver. */ - +/* + * This function is called when an application opens handle to the + * bridge driver. + */ static int bridge_open(struct inode *ip, struct file *filp) { int status = 0; -#ifndef RES_CLEANUP_DISABLE - u32 hProcess; - DSP_STATUS dsp_status = DSP_SOK; - HANDLE hDrvObject = NULL; - struct PROCESS_CONTEXT *pPctxt = NULL; - struct PROCESS_CONTEXT *next_node = NULL; - struct PROCESS_CONTEXT *pCtxtclosed = NULL; - struct PROCESS_CONTEXT *pCtxttraverse = NULL; - struct task_struct *tsk = NULL; - GT_0trace(driverTrace, GT_ENTER, "-> driver_open\n"); - dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); + DSP_STATUS dsp_status; + HANDLE hDrvObject; + struct PROCESS_CONTEXT *pr_ctxt = NULL; - /* Checking weather task structure for all process existing - * in the process context list If not removing those processes*/ - if (DSP_FAILED(dsp_status)) - goto func_cont; + GT_0trace(driverTrace, GT_ENTER, "-> bridge_open\n"); - DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject); - while (pCtxtclosed != NULL) { - tsk = find_task_by_vpid(pCtxtclosed->pid); - next_node = pCtxtclosed->next; - - 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) { - DRV_GetProcCtxtList(&pCtxttraverse, - (struct DRV_OBJECT *)hDrvObject); - if (pCtxttraverse->next == NULL) { - PROC_Detach(pCtxtclosed->hProcessor); - } else { - if ((pCtxtclosed->pid == - pCtxttraverse->pid) && - (pCtxttraverse->next != NULL)) { - pCtxttraverse = - pCtxttraverse->next; - } - while ((pCtxttraverse != NULL) && - (pCtxtclosed->hProcessor - != pCtxttraverse->hProcessor)) { - pCtxttraverse = - pCtxttraverse->next; - if ((pCtxttraverse != NULL) && - (pCtxtclosed->pid == - pCtxttraverse->pid)) { - pCtxttraverse = - pCtxttraverse->next; - } - } - if (pCtxttraverse == NULL) { - PROC_Detach - (pCtxtclosed->hProcessor); - } - } - } - DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject, - pCtxtclosed, - (void *)pCtxtclosed->pid); + dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); + if (DSP_SUCCEEDED(dsp_status)) { + /* + * Allocate a new process context and insert it into global + * process context list. + */ + DRV_InsertProcContext(hDrvObject, &pr_ctxt); + if (pr_ctxt) { + DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED); + DRV_ProcSetPID(pr_ctxt, current->pid); + } else { + status = -ENOMEM; } - pCtxtclosed = next_node; + } else { + status = -EIO; } -func_cont: - dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); - if (DSP_SUCCEEDED(dsp_status)) - dsp_status = DRV_InsertProcContext( - (struct DRV_OBJECT *)hDrvObject, &pPctxt); - if (pPctxt != NULL) { - /* Return PID instead of process handle */ - hProcess = current->pid; + filp->private_data = pr_ctxt; - DRV_ProcUpdatestate(pPctxt, PROC_RES_ALLOCATED); - DRV_ProcSetPID(pPctxt, hProcess); - } -#endif - - GT_0trace(driverTrace, GT_ENTER, " <- driver_open\n"); + GT_0trace(driverTrace, GT_ENTER, "<- bridge_open\n"); return status; } -/* This function is called when an application closes handle to the bridge - * driver. */ +/* + * This function is called when an application closes handle to the bridge + * driver. + */ static int bridge_release(struct inode *ip, struct file *filp) { + int status = 0; + DSP_STATUS dsp_status; + HANDLE hDrvObject; + struct PROCESS_CONTEXT *pr_ctxt; + GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n"); + + if (!filp->private_data) { + status = -EIO; + } else { + pr_ctxt = filp->private_data; + dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); + if (DSP_SUCCEEDED(dsp_status)) { + flush_signals(current); + DRV_RemoveAllResources(pr_ctxt); + PROC_Detach(pr_ctxt->hProcessor); + DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject, + pr_ctxt, (void *)pr_ctxt->pid); + } else { + status = -EIO; + } + filp->private_data = NULL; + } + GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n"); - return 0; + return status; } /* This function provides IO interface to the bridge driver. */ -- 1.6.2.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] DSPBRIDGE: Use TGID instead of PID for resource accounting 2009-08-05 13:25 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ameya Palande @ 2009-08-05 13:25 ` Ameya Palande 2009-08-05 19:20 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ramos Falcon, Ernesto 1 sibling, 0 replies; 7+ messages in thread From: Ameya Palande @ 2009-08-05 13:25 UTC (permalink / raw) To: linux-omap; +Cc: hiroshi.doyu, x0095840, omar.ramirez, roman.tereshonkov, suyog This patch uses TGID instead of PID for accounting resources tied to a user space process. This should fix all the issues related to user space multithreading and resource cleanup. Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com> Signed-off-by: Ameya Palande <ameya.palande@nokia.com> --- drivers/dsp/bridge/pmgr/cmm.c | 4 ++-- drivers/dsp/bridge/rmgr/drv_interface.c | 2 +- drivers/dsp/bridge/rmgr/node.c | 12 ++++++------ drivers/dsp/bridge/rmgr/proc.c | 22 +++++++++++----------- drivers/dsp/bridge/rmgr/strm.c | 16 ++++++++-------- drivers/dsp/bridge/services/kfile.c | 7 ++----- drivers/dsp/bridge/wmd/chnl_sm.c | 4 ++-- 7 files changed, 32 insertions(+), 35 deletions(-) diff --git a/drivers/dsp/bridge/pmgr/cmm.c b/drivers/dsp/bridge/pmgr/cmm.c index 99a2432..7dea18c 100644 --- a/drivers/dsp/bridge/pmgr/cmm.c +++ b/drivers/dsp/bridge/pmgr/cmm.c @@ -284,8 +284,8 @@ void *CMM_CallocBuf(struct CMM_OBJECT *hCmmMgr, u32 uSize, * We'll need to free up a process's alloc'd SM if the * client process goes away. */ - /* Return PID instead of process handle */ - pNode->hClientProc = current->pid; + /* Return TGID instead of process handle */ + pNode->hClientProc = current->tgid; /* put our node on InUse list */ LST_PutTail(pAllocator->pInUseListHead, diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c index 2fcd6f9..80c0eeb 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.c +++ b/drivers/dsp/bridge/rmgr/drv_interface.c @@ -584,7 +584,7 @@ static int bridge_open(struct inode *ip, struct file *filp) DRV_InsertProcContext(hDrvObject, &pr_ctxt); if (pr_ctxt) { DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED); - DRV_ProcSetPID(pr_ctxt, current->pid); + DRV_ProcSetPID(pr_ctxt, current->tgid); } else { status = -ENOMEM; } diff --git a/drivers/dsp/bridge/rmgr/node.c b/drivers/dsp/bridge/rmgr/node.c index 53a42bf..5ff87e9 100644 --- a/drivers/dsp/bridge/rmgr/node.c +++ b/drivers/dsp/bridge/rmgr/node.c @@ -789,8 +789,8 @@ func_cont2: #ifndef RES_CLEANUP_DISABLE if (DSP_SUCCEEDED(status)) { - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); @@ -813,8 +813,8 @@ func_cont2: } } if (DSP_SUCCEEDED(status)) { - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_SUCCEEDED(res_status)) { @@ -1793,8 +1793,8 @@ func_cont1: /* Free host-side resources allocated by NODE_Create() * DeleteNode() fails if SM buffers not freed by client! */ #ifndef RES_CLEANUP_DISABLE - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_FAILED(res_status)) goto func_cont; diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c index 0af05eb..469cf21 100644 --- a/drivers/dsp/bridge/rmgr/proc.c +++ b/drivers/dsp/bridge/rmgr/proc.c @@ -215,7 +215,7 @@ DSP_STATUS PROC_CleanupAllResources(void) goto func_end; DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject); while (pCtxtclosed != NULL) { - if (current->pid != pCtxtclosed->pid) { + if (current->tgid != pCtxtclosed->pid) { GT_1trace(PROC_DebugMask, GT_5CLASS, "***Cleanup of " "process***%d\n", pCtxtclosed->pid); @@ -308,8 +308,8 @@ PROC_Attach(u32 uProcessor, OPTIONAL CONST struct DSP_PROCESSORATTRIN *pAttrIn, pProcObject->hMgrObject = hMgrObject; pProcObject->uProcessor = devType; /* Get Caller Process and store it */ - /* Return PID instead of process handle */ - pProcObject->hProcess = current->pid; + /* Return TGID instead of process handle */ + pProcObject->hProcess = current->tgid; if (pAttrIn) pProcObject->uTimeout = pAttrIn->uTimeout; @@ -385,8 +385,8 @@ func_end: if (DSP_FAILED(status)) goto func_cont; - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDRVObject, REG_DRV_OBJECT); if (DSP_FAILED(res_status)) @@ -402,8 +402,8 @@ func_end: } } func_cont: - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDRVObject, REG_DRV_OBJECT); if (DSP_SUCCEEDED(res_status)) { @@ -1468,8 +1468,8 @@ DSP_STATUS PROC_Map(DSP_HPROCESSOR hProcessor, void *pMpuAddr, u32 ulSize, #ifndef RES_CLEANUP_DISABLE if (DSP_SUCCEEDED(status)) { /* Update the node and stream resource status */ - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); @@ -1847,8 +1847,8 @@ DSP_STATUS PROC_UnMap(DSP_HPROCESSOR hProcessor, void *pMapAddr) goto func_end; /* Update the node and stream resource status */ - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_FAILED(res_status)) diff --git a/drivers/dsp/bridge/rmgr/strm.c b/drivers/dsp/bridge/rmgr/strm.c index bd55fd3..b6dbe46 100644 --- a/drivers/dsp/bridge/rmgr/strm.c +++ b/drivers/dsp/bridge/rmgr/strm.c @@ -203,8 +203,8 @@ DSP_STATUS STRM_AllocateBuffer(struct STRM_OBJECT *hStrm, u32 uSize, if (DSP_FAILED(status)) goto func_end; - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_FAILED(res_status)) @@ -277,8 +277,8 @@ DSP_STATUS STRM_Close(struct STRM_OBJECT *hStrm) goto func_end; /* Update the node and stream resource status */ - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_FAILED(res_status)) @@ -430,8 +430,8 @@ DSP_STATUS STRM_FreeBuffer(struct STRM_OBJECT *hStrm, u8 **apBuffer, } #ifndef RES_CLEANUP_DISABLE /* Update the node and stream resource status */ - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_SUCCEEDED(res_status)) { @@ -779,8 +779,8 @@ func_cont: (void)DeleteStrm(pStrm); #ifndef RES_CLEANUP_DISABLE - /* Return PID instead of process handle */ - hProcess = current->pid; + /* Return TGID instead of process handle */ + hProcess = current->tgid; res_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); if (DSP_SUCCEEDED(res_status)) { diff --git a/drivers/dsp/bridge/services/kfile.c b/drivers/dsp/bridge/services/kfile.c index d29bc22..dd2a962 100644 --- a/drivers/dsp/bridge/services/kfile.c +++ b/drivers/dsp/bridge/services/kfile.c @@ -92,7 +92,6 @@ s32 KFILE_Close(struct KFILE_FileObj *hFile) { s32 cRetVal = 0; /* 0 indicates success */ s32 fRetVal = 0; - __kernel_pid_t curr_pid; GT_1trace(KFILE_debugMask, GT_ENTER, "KFILE_Close: hFile 0x%x\n", hFile); @@ -101,8 +100,6 @@ s32 KFILE_Close(struct KFILE_FileObj *hFile) if (MEM_IsValidHandle(hFile, SIGNATURE)) { /* Close file only if opened by the same process (id). Otherwise * Linux closes all open file handles when process exits.*/ - /* Return PID instead of process handle */ - curr_pid = (__kernel_pid_t)current->pid; fRetVal = filp_close(hFile->fileDesc, NULL) ; if (fRetVal) { cRetVal = E_KFILE_ERROR; @@ -181,8 +178,8 @@ struct KFILE_FileObj *KFILE_Open(CONST char *pszFileName, CONST char *pszMode) hFile->size = fileDesc->f_op->llseek(fileDesc, 0, SEEK_END); fileDesc->f_op->llseek(fileDesc, 0, SEEK_SET); - /* Return PID instead of process handle */ - hFile->owner_pid = current->pid; + /* Return TGID instead of process handle */ + hFile->owner_pid = current->tgid; status = DSP_SOK; } diff --git a/drivers/dsp/bridge/wmd/chnl_sm.c b/drivers/dsp/bridge/wmd/chnl_sm.c index e8ffb2f..c70dba8 100644 --- a/drivers/dsp/bridge/wmd/chnl_sm.c +++ b/drivers/dsp/bridge/wmd/chnl_sm.c @@ -933,8 +933,8 @@ DSP_STATUS WMD_CHNL_Open(OUT struct CHNL_OBJECT **phChnl, pChnl->uMode = uMode; pChnl->hUserEvent = hSyncEvent; /* for Linux */ pChnl->hSyncEvent = hSyncEvent; - /* get the process handle */ - pChnl->hProcess = current->pid; + /* Return TGID instead of process handle */ + pChnl->hProcess = current->tgid; pChnl->pCBArg = 0; pChnl->cBytesMoved = 0; /* Default to proc-copy */ -- 1.6.2.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release 2009-08-05 13:25 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ameya Palande 2009-08-05 13:25 ` [PATCH 3/3] DSPBRIDGE: Use TGID instead of PID for resource accounting Ameya Palande @ 2009-08-05 19:20 ` Ramos Falcon, Ernesto 2009-08-05 21:52 ` Ameya Palande 1 sibling, 1 reply; 7+ messages in thread From: Ramos Falcon, Ernesto @ 2009-08-05 19:20 UTC (permalink / raw) To: Ameya Palande, linux-omap@vger.kernel.org Cc: hiroshi.doyu@nokia.com, Guzman Lugo, Fernando, Ramirez Luna, Omar, roman.tereshonkov@nokia.com, Moogi, Suyog Hi, > -----Original Message----- > From: linux-omap-owner@vger.kernel.org [mailto:linux-omap- > owner@vger.kernel.org] On Behalf Of Ameya Palande > Sent: Wednesday, August 05, 2009 8:25 AM > To: linux-omap@vger.kernel.org > Cc: hiroshi.doyu@nokia.com; Guzman Lugo, Fernando; Ramirez Luna, Omar; > roman.tereshonkov@nokia.com; Moogi, Suyog > Subject: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release > > Instead of doing lazy resource cleanup in bridge_open, this patch will > move > it to bridge_release. This way the resources allocated will be cleaned up > when that instance of the open is closed and will not wait till the next > instance of open happens. > > Signed-off-by: Fernando Guzman Lugo <x0095840@ti.com> > Signed-off-by: Ameya Palande <ameya.palande@nokia.com> > --- > drivers/dsp/bridge/rmgr/drv_interface.c | 132 ++++++++++++-------------- > ----- > 1 files changed, 52 insertions(+), 80 deletions(-) > > diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c > b/drivers/dsp/bridge/rmgr/drv_interface.c > index 8cbdeee..2fcd6f9 100644 > --- a/drivers/dsp/bridge/rmgr/drv_interface.c > +++ b/drivers/dsp/bridge/rmgr/drv_interface.c > @@ -562,102 +562,74 @@ static void __exit bridge_exit(void) > platform_driver_unregister(&bridge_driver); > } > > -/* This function is called when an application opens handle to the > - * bridge driver. */ > - > +/* > + * This function is called when an application opens handle to the > + * bridge driver. > + */ > static int bridge_open(struct inode *ip, struct file *filp) > { > int status = 0; > -#ifndef RES_CLEANUP_DISABLE > - u32 hProcess; > - DSP_STATUS dsp_status = DSP_SOK; > - HANDLE hDrvObject = NULL; > - struct PROCESS_CONTEXT *pPctxt = NULL; > - struct PROCESS_CONTEXT *next_node = NULL; > - struct PROCESS_CONTEXT *pCtxtclosed = NULL; > - struct PROCESS_CONTEXT *pCtxttraverse = NULL; > - struct task_struct *tsk = NULL; > - GT_0trace(driverTrace, GT_ENTER, "-> driver_open\n"); > - dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); > + DSP_STATUS dsp_status; > + HANDLE hDrvObject; > + struct PROCESS_CONTEXT *pr_ctxt = NULL; > > - /* Checking weather task structure for all process existing > - * in the process context list If not removing those processes*/ > - if (DSP_FAILED(dsp_status)) > - goto func_cont; > + GT_0trace(driverTrace, GT_ENTER, "-> bridge_open\n"); > > - DRV_GetProcCtxtList(&pCtxtclosed, (struct DRV_OBJECT *)hDrvObject); > - while (pCtxtclosed != NULL) { > - tsk = find_task_by_vpid(pCtxtclosed->pid); > - next_node = pCtxtclosed->next; > - > - 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) { > - DRV_GetProcCtxtList(&pCtxttraverse, > - (struct DRV_OBJECT *)hDrvObject); > - if (pCtxttraverse->next == NULL) { > - PROC_Detach(pCtxtclosed->hProcessor); > - } else { > - if ((pCtxtclosed->pid == > - pCtxttraverse->pid) && > - (pCtxttraverse->next != NULL)) { > - pCtxttraverse = > - pCtxttraverse->next; > - } > - while ((pCtxttraverse != NULL) && > - (pCtxtclosed->hProcessor > - != pCtxttraverse->hProcessor)) { > - pCtxttraverse = > - pCtxttraverse->next; > - if ((pCtxttraverse != NULL) && > - (pCtxtclosed->pid == > - pCtxttraverse->pid)) { > - pCtxttraverse = > - pCtxttraverse->next; > - } > - } > - if (pCtxttraverse == NULL) { > - PROC_Detach > - (pCtxtclosed->hProcessor); > - } > - } > - } > - DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject, > - pCtxtclosed, > - (void *)pCtxtclosed->pid); > + dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); > + if (DSP_SUCCEEDED(dsp_status)) { > + /* > + * Allocate a new process context and insert it into global > + * process context list. > + */ > + DRV_InsertProcContext(hDrvObject, &pr_ctxt); > + if (pr_ctxt) { > + DRV_ProcUpdatestate(pr_ctxt, PROC_RES_ALLOCATED); > + DRV_ProcSetPID(pr_ctxt, current->pid); > + } else { > + status = -ENOMEM; > } > - pCtxtclosed = next_node; > + } else { > + status = -EIO; > } > -func_cont: > - dsp_status = CFG_GetObject((u32 *)&hDrvObject, REG_DRV_OBJECT); > - if (DSP_SUCCEEDED(dsp_status)) > - dsp_status = DRV_InsertProcContext( > - (struct DRV_OBJECT *)hDrvObject, &pPctxt); > > - if (pPctxt != NULL) { > - /* Return PID instead of process handle */ > - hProcess = current->pid; > + filp->private_data = pr_ctxt; > > - DRV_ProcUpdatestate(pPctxt, PROC_RES_ALLOCATED); > - DRV_ProcSetPID(pPctxt, hProcess); > - } > -#endif > - > - GT_0trace(driverTrace, GT_ENTER, " <- driver_open\n"); > + GT_0trace(driverTrace, GT_ENTER, "<- bridge_open\n"); > return status; > } > > -/* This function is called when an application closes handle to the > bridge > - * driver. */ > +/* > + * This function is called when an application closes handle to the > bridge > + * driver. > + */ > static int bridge_release(struct inode *ip, struct file *filp) > { > + int status = 0; > + DSP_STATUS dsp_status; > + HANDLE hDrvObject; > + struct PROCESS_CONTEXT *pr_ctxt; > + > GT_0trace(driverTrace, GT_ENTER, "-> bridge_release\n"); > + > + if (!filp->private_data) { > + status = -EIO; > + } else { > + pr_ctxt = filp->private_data; > + dsp_status = CFG_GetObject((u32 *)&hDrvObject, > REG_DRV_OBJECT); > + if (DSP_SUCCEEDED(dsp_status)) { > + flush_signals(current); > + DRV_RemoveAllResources(pr_ctxt); > + PROC_Detach(pr_ctxt->hProcessor); > + DRV_RemoveProcContext((struct DRV_OBJECT *)hDrvObject, > + pr_ctxt, (void *)pr_ctxt->pid); > + } else { > + status = -EIO; > + } > + filp->private_data = NULL; > + } > + > GT_0trace(driverTrace, GT_ENTER, "<- bridge_release\n"); > - return 0; > + return status; > } > > /* This function provides IO interface to the bridge driver. */ > -- > 1.6.2.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release 2009-08-05 19:20 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ramos Falcon, Ernesto @ 2009-08-05 21:52 ` Ameya Palande 2009-08-06 18:06 ` Ramos Falcon, Ernesto 0 siblings, 1 reply; 7+ messages in thread From: Ameya Palande @ 2009-08-05 21:52 UTC (permalink / raw) To: ext 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 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release 2009-08-05 21:52 ` Ameya Palande @ 2009-08-06 18:06 ` Ramos Falcon, Ernesto 2009-08-06 18:12 ` Ameya Palande 0 siblings, 1 reply; 7+ messages in thread From: Ramos Falcon, Ernesto @ 2009-08-06 18:06 UTC (permalink / raw) To: Ameya Palande Cc: linux-omap@vger.kernel.org, Doyu Hiroshi (Nokia-D/Helsinki), Guzman Lugo, Fernando, Ramirez Luna, Omar, Tereshonkov Roman (Nokia-D/Helsinki), Moogi, Suyog 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release 2009-08-06 18:06 ` Ramos Falcon, Ernesto @ 2009-08-06 18:12 ` Ameya Palande 0 siblings, 0 replies; 7+ messages in thread From: Ameya Palande @ 2009-08-06 18:12 UTC (permalink / raw) To: ext 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 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2009-08-06 18:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-08-05 13:25 [PATCH 1/3] DSPBRIDGE: Cleanup bridge_release and remove DSP_Close Ameya Palande 2009-08-05 13:25 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ameya Palande 2009-08-05 13:25 ` [PATCH 3/3] DSPBRIDGE: Use TGID instead of PID for resource accounting Ameya Palande 2009-08-05 19:20 ` [PATCH 2/3] DSPBRIDGE: Move resource cleanup to bridge_release Ramos Falcon, Ernesto 2009-08-05 21:52 ` Ameya Palande 2009-08-06 18:06 ` Ramos Falcon, Ernesto 2009-08-06 18:12 ` Ameya Palande
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox