* [PATCHv2 0/2] DSPBRIDGE: Improved reserved memory cleanup @ 2010-02-16 13:20 Ameya Palande 2010-02-16 13:20 ` [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT Ameya Palande 2010-02-16 13:20 ` [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework Ameya Palande 0 siblings, 2 replies; 8+ messages in thread From: Ameya Palande @ 2010-02-16 13:20 UTC (permalink / raw) To: linux-omap; +Cc: felipe.contreras, nm, deepak.chitriki, omar.ramirez, x0095840 This patch series splits DMM_RES_OBJECT into DMM_MAP_OBJECT and DMM_RSV_OBJECT which are used independently for mapped and reserved memory resources accounting. This will help in cleanup of reserved memory resources which was not handled properly before. With these patches resource cleanup mechanism will work perfectly in a use case where a big chunk of memory is reserved and then lot of mappings are created inside it. v2: Addresses following comments about reducing indentation http://marc.info/?l=linux-omap&m=126624982331523&w=2 Ameya Palande (2): DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT DSPBRIDGE: New reserved memory accounting framework arch/arm/plat-omap/include/dspbridge/drv.h | 25 ++++++-- arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- drivers/dsp/bridge/pmgr/dmm.c | 3 +- drivers/dsp/bridge/pmgr/wcd.c | 7 +- drivers/dsp/bridge/rmgr/drv.c | 84 ++++++++++++++++----------- drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- drivers/dsp/bridge/rmgr/node.c | 5 +- drivers/dsp/bridge/rmgr/proc.c | 58 +++++++++++++++---- 8 files changed, 132 insertions(+), 62 deletions(-) ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT 2010-02-16 13:20 [PATCHv2 0/2] DSPBRIDGE: Improved reserved memory cleanup Ameya Palande @ 2010-02-16 13:20 ` Ameya Palande 2010-02-16 18:27 ` Felipe Contreras 2010-02-16 13:20 ` [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework Ameya Palande 1 sibling, 1 reply; 8+ messages in thread From: Ameya Palande @ 2010-02-16 13:20 UTC (permalink / raw) To: linux-omap; +Cc: felipe.contreras, nm, deepak.chitriki, omar.ramirez, x0095840 In its current state DMM_RES_OBJECT is not correctly tracking reserve and unreserve but only map and unmap. So lets rename it to DMM_MAP_OBJECT to clarify what it is really doing! In addition to this, this patch also does some trivial code cleanup. Signed-off-by: Ameya Palande <ameya.palande@nokia.com> --- arch/arm/plat-omap/include/dspbridge/drv.h | 12 +++--- drivers/dsp/bridge/rmgr/drv.c | 57 ++++++++++++++-------------- 2 files changed, 35 insertions(+), 34 deletions(-) diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat-omap/include/dspbridge/drv.h index b6a5fd2..3069e74 100644 --- a/arch/arm/plat-omap/include/dspbridge/drv.h +++ b/arch/arm/plat-omap/include/dspbridge/drv.h @@ -90,15 +90,15 @@ struct NODE_RES_OBJECT { struct NODE_RES_OBJECT *next; } ; -/* New structure (member of process context) abstracts DMM resource info */ -struct DMM_RES_OBJECT { +/* New structure (member of process context) abstracts DMM mapped memory info */ +struct DMM_MAP_OBJECT { s32 dmmAllocated; /* DMM status */ u32 ulMpuAddr; u32 ulDSPAddr; u32 ulDSPResAddr; - u32 dmmSize; + u32 size; HANDLE hProcessor; - struct DMM_RES_OBJECT *next; + struct DMM_MAP_OBJECT *next; } ; /* New structure (member of process context) abstracts DMM resource info */ @@ -138,8 +138,8 @@ struct PROCESS_CONTEXT{ /* DSP Node resources */ struct NODE_RES_OBJECT *pNodeList; - /* DMM resources */ - struct DMM_RES_OBJECT *pDMMList; + /* DMM mapped memory resources */ + struct DMM_MAP_OBJECT *dmm_map_list; /* DSP Heap resources */ struct DSPHEAP_RES_OBJECT *pDSPHEAPList; diff --git a/drivers/dsp/bridge/rmgr/drv.c b/drivers/dsp/bridge/rmgr/drv.c index 36bab9f..3dc38d0 100644 --- a/drivers/dsp/bridge/rmgr/drv.c +++ b/drivers/dsp/bridge/rmgr/drv.c @@ -188,28 +188,28 @@ static DSP_STATUS DRV_ProcFreeNodeRes(HANDLE hPCtxt) DSP_STATUS DRV_InsertDMMResElement(HANDLE hDMMRes, HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; - struct DMM_RES_OBJECT **pDMMRes = (struct DMM_RES_OBJECT **)hDMMRes; + struct DMM_MAP_OBJECT **pDMMRes = (struct DMM_MAP_OBJECT **)hDMMRes; DSP_STATUS status = DSP_SOK; - struct DMM_RES_OBJECT *pTempDMMRes = NULL; + struct DMM_MAP_OBJECT *pTempDMMRes = NULL; - *pDMMRes = (struct DMM_RES_OBJECT *) - MEM_Calloc(1 * sizeof(struct DMM_RES_OBJECT), MEM_PAGED); + *pDMMRes = (struct DMM_MAP_OBJECT *) + MEM_Calloc(1 * sizeof(struct DMM_MAP_OBJECT), MEM_PAGED); GT_0trace(curTrace, GT_ENTER, "DRV_InsertDMMResElement: 1"); if (*pDMMRes == NULL) { GT_0trace(curTrace, GT_5CLASS, "DRV_InsertDMMResElement: 2"); status = DSP_EHANDLE; } if (DSP_SUCCEEDED(status)) { - if (pCtxt->pDMMList != NULL) { + if (pCtxt->dmm_map_list) { GT_0trace(curTrace, GT_5CLASS, "DRV_InsertDMMResElement: 3"); - pTempDMMRes = pCtxt->pDMMList; - while (pTempDMMRes->next != NULL) + pTempDMMRes = pCtxt->dmm_map_list; + while (pTempDMMRes->next) pTempDMMRes = pTempDMMRes->next; pTempDMMRes->next = *pDMMRes; } else { - pCtxt->pDMMList = *pDMMRes; + pCtxt->dmm_map_list = *pDMMRes; GT_0trace(curTrace, GT_5CLASS, "DRV_InsertDMMResElement: 4"); } @@ -223,12 +223,12 @@ DSP_STATUS DRV_InsertDMMResElement(HANDLE hDMMRes, HANDLE hPCtxt) DSP_STATUS DRV_RemoveDMMResElement(HANDLE hDMMRes, HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; - struct DMM_RES_OBJECT *pDMMRes = (struct DMM_RES_OBJECT *)hDMMRes; - struct DMM_RES_OBJECT *pTempDMMRes = NULL; + struct DMM_MAP_OBJECT *pDMMRes = (struct DMM_MAP_OBJECT *)hDMMRes; + struct DMM_MAP_OBJECT *pTempDMMRes = NULL; - pTempDMMRes = pCtxt->pDMMList; - if (pCtxt->pDMMList == pDMMRes) { - pCtxt->pDMMList = pDMMRes->next; + pTempDMMRes = pCtxt->dmm_map_list; + if (pCtxt->dmm_map_list == pDMMRes) { + pCtxt->dmm_map_list = pDMMRes->next; } else { while (pTempDMMRes && pTempDMMRes->next != pDMMRes) pTempDMMRes = pTempDMMRes->next; @@ -245,14 +245,14 @@ DSP_STATUS DRV_UpdateDMMResElement(HANDLE hDMMRes, u32 pMpuAddr, u32 ulSize, u32 pReqAddr, u32 pMapAddr, HANDLE hProcessor) { - struct DMM_RES_OBJECT *pDMMRes = (struct DMM_RES_OBJECT *)hDMMRes; + struct DMM_MAP_OBJECT *pDMMRes = (struct DMM_MAP_OBJECT *)hDMMRes; DSP_STATUS status = DSP_SOK; DBC_Assert(hDMMRes != NULL); pDMMRes->ulMpuAddr = pMpuAddr; pDMMRes->ulDSPAddr = pMapAddr; pDMMRes->ulDSPResAddr = pReqAddr; - pDMMRes->dmmSize = ulSize; + pDMMRes->size = ulSize; pDMMRes->hProcessor = hProcessor; pDMMRes->dmmAllocated = 1; @@ -264,11 +264,11 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; DSP_STATUS status = DSP_SOK; - struct DMM_RES_OBJECT *pDMMList = pCtxt->pDMMList; - struct DMM_RES_OBJECT *pDMMRes = NULL; + struct DMM_MAP_OBJECT *pDMMList = pCtxt->dmm_map_list; + struct DMM_MAP_OBJECT *pDMMRes = NULL; GT_0trace(curTrace, GT_ENTER, "\nDRV_ProcFreeDMMRes: 1\n"); - while (pDMMList != NULL) { + while (pDMMList) { pDMMRes = pDMMList; pDMMList = pDMMList->next; if (pDMMRes->dmmAllocated) { @@ -294,41 +294,42 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; DSP_STATUS status = DSP_SOK; - struct DMM_RES_OBJECT *pTempDMMRes2 = NULL; - struct DMM_RES_OBJECT *pTempDMMRes = NULL; + struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; + struct DMM_MAP_OBJECT *pTempDMMRes = NULL; DRV_ProcFreeDMMRes(pCtxt); - pTempDMMRes = pCtxt->pDMMList; + pTempDMMRes = pCtxt->dmm_map_list; while (pTempDMMRes != NULL) { pTempDMMRes2 = pTempDMMRes; pTempDMMRes = pTempDMMRes->next; kfree(pTempDMMRes2); } - pCtxt->pDMMList = NULL; + pCtxt->dmm_map_list = NULL; return status; } DSP_STATUS DRV_GetDMMResElement(u32 pMapAddr, HANDLE hDMMRes, HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; - struct DMM_RES_OBJECT **pDMMRes = (struct DMM_RES_OBJECT **)hDMMRes; + struct DMM_MAP_OBJECT **pDMMRes = (struct DMM_MAP_OBJECT **)hDMMRes; DSP_STATUS status = DSP_SOK; - struct DMM_RES_OBJECT *pTempDMM = NULL; + struct DMM_MAP_OBJECT *pTempDMM = NULL; - pTempDMM = pCtxt->pDMMList; - while ((pTempDMM != NULL) && (pTempDMM->ulDSPAddr != pMapAddr)) { + pTempDMM = pCtxt->dmm_map_list; + while (pTempDMM && (pTempDMM->ulDSPAddr != pMapAddr)) { GT_3trace(curTrace, GT_ENTER, "DRV_GetDMMResElement: 2 pTempDMM:%x " "pTempDMM->ulDSPAddr:%x pMapAddr:%x\n", pTempDMM, pTempDMM->ulDSPAddr, pMapAddr); pTempDMM = pTempDMM->next; } - if (pTempDMM != NULL) { + if (pTempDMM) { GT_0trace(curTrace, GT_ENTER, "DRV_GetDMMResElement: 3"); *pDMMRes = pTempDMM; } else { status = DSP_ENOTFOUND; - } GT_0trace(curTrace, GT_ENTER, "DRV_GetDMMResElement: 4"); + } + GT_0trace(curTrace, GT_ENTER, "DRV_GetDMMResElement: 4"); return status; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT 2010-02-16 13:20 ` [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT Ameya Palande @ 2010-02-16 18:27 ` Felipe Contreras 0 siblings, 0 replies; 8+ messages in thread From: Felipe Contreras @ 2010-02-16 18:27 UTC (permalink / raw) To: Ameya Palande Cc: linux-omap, felipe.contreras, nm, deepak.chitriki, omar.ramirez, x0095840 On Tue, Feb 16, 2010 at 3:20 PM, Ameya Palande <ameya.palande@nokia.com> wrote: > In its current state DMM_RES_OBJECT is not correctly tracking reserve and > unreserve but only map and unmap. So lets rename it to DMM_MAP_OBJECT to > clarify what it is really doing! > > In addition to this, this patch also does some trivial code cleanup. > > Signed-off-by: Ameya Palande <ameya.palande@nokia.com> Reviewed-by: Felipe Contreras <felipe.contreras@nokia.com> -- Felipe Contreras ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework 2010-02-16 13:20 [PATCHv2 0/2] DSPBRIDGE: Improved reserved memory cleanup Ameya Palande 2010-02-16 13:20 ` [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT Ameya Palande @ 2010-02-16 13:20 ` Ameya Palande 2010-02-16 18:42 ` Felipe Contreras 2010-02-17 2:01 ` Guzman Lugo, Fernando 1 sibling, 2 replies; 8+ messages in thread From: Ameya Palande @ 2010-02-16 13:20 UTC (permalink / raw) To: linux-omap; +Cc: felipe.contreras, nm, deepak.chitriki, omar.ramirez, x0095840 DSP_RSV_OBJECT is introduced to track reserved memory accounting information. This will allow us to do proper cleanup for memory reserved using PROC_ReserveMemory(). Signed-off-by: Ameya Palande <ameya.palande@nokia.com> --- arch/arm/plat-omap/include/dspbridge/drv.h | 13 ++++++ arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- drivers/dsp/bridge/pmgr/dmm.c | 3 +- drivers/dsp/bridge/pmgr/wcd.c | 7 ++- drivers/dsp/bridge/rmgr/drv.c | 27 +++++++++--- drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- drivers/dsp/bridge/rmgr/node.c | 5 +- drivers/dsp/bridge/rmgr/proc.c | 58 +++++++++++++++++++++----- 8 files changed, 97 insertions(+), 28 deletions(-) diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat-omap/include/dspbridge/drv.h index 3069e74..6e8187d 100644 --- a/arch/arm/plat-omap/include/dspbridge/drv.h +++ b/arch/arm/plat-omap/include/dspbridge/drv.h @@ -101,6 +101,15 @@ struct DMM_MAP_OBJECT { struct DMM_MAP_OBJECT *next; } ; +/* + * New structure (member of process context) used for accounting of DMM + * reserved memory information + */ +struct DMM_RSV_OBJECT { + struct list_head link; + u32 dsp_reserved_addr; +}; + /* New structure (member of process context) abstracts DMM resource info */ struct DSPHEAP_RES_OBJECT { s32 heapAllocated; /* DMM status */ @@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ /* DMM mapped memory resources */ struct DMM_MAP_OBJECT *dmm_map_list; + /* DMM reserved memory resources */ + struct list_head dmm_rsv_list; + spinlock_t dmm_rsv_list_lock; + /* DSP Heap resources */ struct DSPHEAP_RES_OBJECT *pDSPHEAPList; diff --git a/arch/arm/plat-omap/include/dspbridge/proc.h b/arch/arm/plat-omap/include/dspbridge/proc.h index 8dbdaac..558c053 100644 --- a/arch/arm/plat-omap/include/dspbridge/proc.h +++ b/arch/arm/plat-omap/include/dspbridge/proc.h @@ -560,7 +560,7 @@ * Details: */ extern DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, - u32 ulSize, void **ppRsvAddr); + u32 ulSize, void **ppRsvAddr, struct PROCESS_CONTEXT *pr_ctxt); /* * ======== PROC_UnMap ======== @@ -604,6 +604,7 @@ * Details: */ extern DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, - void *pRsvAddr); + void *pRsvAddr, struct PROCESS_CONTEXT *pr_ctxt, + int cleanup); #endif /* PROC_ */ diff --git a/drivers/dsp/bridge/pmgr/dmm.c b/drivers/dsp/bridge/pmgr/dmm.c index d5a7275..72aee60 100644 --- a/drivers/dsp/bridge/pmgr/dmm.c +++ b/drivers/dsp/bridge/pmgr/dmm.c @@ -386,9 +386,10 @@ DSP_STATUS DMM_ReserveMemory(struct DMM_OBJECT *hDmmMgr, u32 size, node->MappedSize = 0; /* Return the chunk's starting address */ *pRsvAddr = rsvAddr; - } else + } else { /*dSP chunk of given size is not available */ status = DSP_EMEMORY; + } SYNC_LeaveCS(pDmmObj->hDmmLock); GT_3trace(DMM_debugMask, GT_4CLASS, diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c index beea23b..747d069 100644 --- a/drivers/dsp/bridge/pmgr/wcd.c +++ b/drivers/dsp/bridge/pmgr/wcd.c @@ -1054,12 +1054,13 @@ u32 PROCWRAP_ReserveMemory(union Trapped_Args *args, void *pr_ctxt) GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_ReserveMemory: entered\n"); status = PROC_ReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, - args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr); + args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr, + pr_ctxt); if (DSP_SUCCEEDED(status)) { if (put_user(pRsvAddr, args->ARGS_PROC_RSVMEM.ppRsvAddr)) { status = DSP_EINVALIDARG; PROC_UnReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, - pRsvAddr); + pRsvAddr, pr_ctxt, 1); } } return status; @@ -1100,7 +1101,7 @@ u32 PROCWRAP_UnReserveMemory(union Trapped_Args *args, void *pr_ctxt) GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_UnReserveMemory: entered\n"); status = PROC_UnReserveMemory(args->ARGS_PROC_UNRSVMEM.hProcessor, - args->ARGS_PROC_UNRSVMEM.pRsvAddr); + args->ARGS_PROC_UNRSVMEM.pRsvAddr, pr_ctxt, 1); return status; } diff --git a/drivers/dsp/bridge/rmgr/drv.c b/drivers/dsp/bridge/rmgr/drv.c index 3dc38d0..9d5c077 100644 --- a/drivers/dsp/bridge/rmgr/drv.c +++ b/drivers/dsp/bridge/rmgr/drv.c @@ -277,25 +277,20 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) if (DSP_FAILED(status)) pr_debug("%s: PROC_UnMap failed! status =" " 0x%xn", __func__, status); - status = PROC_UnReserveMemory(pDMMRes->hProcessor, - (void *)pDMMRes->ulDSPResAddr); - if (DSP_FAILED(status)) - pr_debug("%s: PROC_UnReserveMemory failed!" - " status = 0x%xn", __func__, status); pDMMRes->dmmAllocated = 0; } } return status; } -/* Release all DMM resources and its context -* This is called from .bridge_release. */ +/* Release all Mapped and Reserved DMM resources */ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) { struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; DSP_STATUS status = DSP_SOK; struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; struct DMM_MAP_OBJECT *pTempDMMRes = NULL; + struct DMM_RSV_OBJECT *temp, *rsv_obj; DRV_ProcFreeDMMRes(pCtxt); pTempDMMRes = pCtxt->dmm_map_list; @@ -305,6 +300,24 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) kfree(pTempDMMRes2); } pCtxt->dmm_map_list = NULL; + + /* Free DMM reserved memory resources */ + spin_lock(&pCtxt->dmm_rsv_list_lock); + list_for_each_entry_safe(rsv_obj, temp, &pCtxt->dmm_rsv_list, link) { + /* + * Since we have already acquired spin lock for dmm_rsv_list, + * we don't want to perform cleanup in PROC_UnReserveMemory. + */ + status = PROC_UnReserveMemory(pCtxt->hProcessor, + (void *)rsv_obj->dsp_reserved_addr, pCtxt, 0); + if (DSP_FAILED(status)) + pr_err("%s: PROC_UnReserveMemory failed!" + " status = 0x%xn", __func__, status); + list_del(&rsv_obj->link); + kfree(rsv_obj); + } + spin_unlock(&pCtxt->dmm_rsv_list_lock); + return status; } diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c b/drivers/dsp/bridge/rmgr/drv_interface.c index 0ce9a5f..ef078cf 100644 --- a/drivers/dsp/bridge/rmgr/drv_interface.c +++ b/drivers/dsp/bridge/rmgr/drv_interface.c @@ -497,10 +497,13 @@ static int bridge_open(struct inode *ip, struct file *filp) * process context list. */ pr_ctxt = MEM_Calloc(sizeof(struct PROCESS_CONTEXT), MEM_PAGED); - if (pr_ctxt) + if (pr_ctxt) { pr_ctxt->resState = PROC_RES_ALLOCATED; - else + spin_lock_init(&pr_ctxt->dmm_rsv_list_lock); + INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list); + } else { status = -ENOMEM; + } filp->private_data = pr_ctxt; diff --git a/drivers/dsp/bridge/rmgr/node.c b/drivers/dsp/bridge/rmgr/node.c index b60d1ed..f85b4b8 100644 --- a/drivers/dsp/bridge/rmgr/node.c +++ b/drivers/dsp/bridge/rmgr/node.c @@ -454,7 +454,7 @@ DSP_STATUS NODE_Allocate(struct PROC_OBJECT *hProcessor, status = PROC_ReserveMemory(hProcessor, pNode->createArgs.asa.taskArgs.uHeapSize + PAGE_SIZE, (void **)&(pNode->createArgs.asa.taskArgs. - uDSPHeapResAddr)); + uDSPHeapResAddr), pr_ctxt); if (DSP_FAILED(status)) { GT_1trace(NODE_debugMask, GT_5CLASS, "NODE_Allocate:Failed to reserve " @@ -2726,7 +2726,8 @@ static void DeleteNode(struct NODE_OBJECT *hNode, " Status = 0x%x\n", (u32)status); } status = PROC_UnReserveMemory(hNode->hProcessor, - (void *)taskArgs.uDSPHeapResAddr); + (void *)taskArgs.uDSPHeapResAddr, + pr_ctxt, 1); if (DSP_SUCCEEDED(status)) { GT_0trace(NODE_debugMask, GT_5CLASS, "DSPProcessor_UnReserveMemory " diff --git a/drivers/dsp/bridge/rmgr/proc.c b/drivers/dsp/bridge/rmgr/proc.c index 2ccbc9b..2ef2e37 100644 --- a/drivers/dsp/bridge/rmgr/proc.c +++ b/drivers/dsp/bridge/rmgr/proc.c @@ -1430,11 +1430,12 @@ func_end: * Reserve a virtually contiguous region of DSP address space. */ DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, u32 ulSize, - void **ppRsvAddr) + void **ppRsvAddr, struct PROCESS_CONTEXT *pr_ctxt) { struct DMM_OBJECT *hDmmMgr; DSP_STATUS status = DSP_SOK; struct PROC_OBJECT *pProcObject = (struct PROC_OBJECT *)hProcessor; + struct DMM_RSV_OBJECT *rsv_obj; GT_3trace(PROC_DebugMask, GT_ENTER, "Entered PROC_ReserveMemory, args:\n\t" @@ -1446,16 +1447,29 @@ DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, u32 ulSize, "InValid Processor Handle \n"); goto func_end; } + status = DMM_GetHandle(pProcObject, &hDmmMgr); if (DSP_FAILED(status)) { GT_1trace(PROC_DebugMask, GT_7CLASS, "PROC_ReserveMemory: " "Failed to get DMM Mgr handle: 0x%x\n", status); - } else - status = DMM_ReserveMemory(hDmmMgr, ulSize, (u32 *)ppRsvAddr); + goto func_end; + } + + status = DMM_ReserveMemory(hDmmMgr, ulSize, (u32 *)ppRsvAddr); + if (status != DSP_SOK) + goto func_end; + + rsv_obj = kmalloc(sizeof(struct DMM_RSV_OBJECT), GFP_KERNEL); + if (rsv_obj) { + rsv_obj->dsp_reserved_addr = (u32) *ppRsvAddr; + spin_lock(&pr_ctxt->dmm_rsv_list_lock); + list_add(&rsv_obj->link, &pr_ctxt->dmm_rsv_list); + spin_unlock(&pr_ctxt->dmm_rsv_list_lock); + } +func_end: GT_1trace(PROC_DebugMask, GT_ENTER, "Leaving PROC_ReserveMemory [0x%x]", status); -func_end: return status; } @@ -1704,11 +1718,13 @@ func_end: * Purpose: * Frees a previously reserved region of DSP address space. */ -DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, void *pRsvAddr) +DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, void *pRsvAddr, + struct PROCESS_CONTEXT *pr_ctxt, int cleanup) { struct DMM_OBJECT *hDmmMgr; DSP_STATUS status = DSP_SOK; struct PROC_OBJECT *pProcObject = (struct PROC_OBJECT *)hProcessor; + struct DMM_RSV_OBJECT *temp, *rsv_obj; GT_2trace(PROC_DebugMask, GT_ENTER, "Entered PROC_UnReserveMemory, args:\n\t" @@ -1719,18 +1735,38 @@ DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, void *pRsvAddr) "InValid Processor Handle \n"); goto func_end; } + status = DMM_GetHandle(pProcObject, &hDmmMgr); - if (DSP_FAILED(status)) + if (DSP_FAILED(status)) { GT_1trace(PROC_DebugMask, GT_7CLASS, "PROC_UnReserveMemory: Failed to get DMM Mgr " "handle: 0x%x\n", status); - else - status = DMM_UnReserveMemory(hDmmMgr, (u32) pRsvAddr); + goto func_end; + } + + status = DMM_UnReserveMemory(hDmmMgr, (u32) pRsvAddr); + + /* + * cleanup flag handles the special case when this function + * is called while doing clenaup from + * DRV_RemoveAllDMMResElements + */ + if (status != DSP_SOK || !cleanup) + goto func_end; + + spin_lock(&pr_ctxt->dmm_rsv_list_lock); + list_for_each_entry_safe(rsv_obj, temp, &pr_ctxt->dmm_rsv_list, link) { + if (rsv_obj->dsp_reserved_addr == (u32)pRsvAddr) { + list_del(&rsv_obj->link); + kfree(rsv_obj); + break; + } + } + spin_unlock(&pr_ctxt->dmm_rsv_list_lock); - GT_1trace(PROC_DebugMask, GT_ENTER, - "Leaving PROC_UnReserveMemory [0x%x]", - status); func_end: + GT_1trace(PROC_DebugMask, GT_ENTER, + "Leaving PROC_UnReserveMemory [0x%x]", status); return status; } -- 1.6.3.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework 2010-02-16 13:20 ` [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework Ameya Palande @ 2010-02-16 18:42 ` Felipe Contreras 2010-02-17 2:05 ` Guzman Lugo, Fernando 2010-02-17 2:01 ` Guzman Lugo, Fernando 1 sibling, 1 reply; 8+ messages in thread From: Felipe Contreras @ 2010-02-16 18:42 UTC (permalink / raw) To: Ameya Palande Cc: linux-omap, felipe.contreras, nm, deepak.chitriki, omar.ramirez, x0095840 On Tue, Feb 16, 2010 at 3:20 PM, Ameya Palande <ameya.palande@nokia.com> wrote: > DSP_RSV_OBJECT is introduced to track reserved memory accounting information. > This will allow us to do proper cleanup for memory reserved using > PROC_ReserveMemory(). > > Signed-off-by: Ameya Palande <ameya.palande@nokia.com> > --- > arch/arm/plat-omap/include/dspbridge/drv.h | 13 ++++++ > arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- > drivers/dsp/bridge/pmgr/dmm.c | 3 +- > drivers/dsp/bridge/pmgr/wcd.c | 7 ++- > drivers/dsp/bridge/rmgr/drv.c | 27 +++++++++--- > drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- > drivers/dsp/bridge/rmgr/node.c | 5 +- > drivers/dsp/bridge/rmgr/proc.c | 58 +++++++++++++++++++++----- > 8 files changed, 97 insertions(+), 28 deletions(-) > > diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat-omap/include/dspbridge/drv.h > index 3069e74..6e8187d 100644 > --- a/arch/arm/plat-omap/include/dspbridge/drv.h > +++ b/arch/arm/plat-omap/include/dspbridge/drv.h > @@ -101,6 +101,15 @@ struct DMM_MAP_OBJECT { > struct DMM_MAP_OBJECT *next; > } ; > > +/* > + * New structure (member of process context) used for accounting of DMM > + * reserved memory information > + */ > +struct DMM_RSV_OBJECT { > + struct list_head link; > + u32 dsp_reserved_addr; > +}; > + > /* New structure (member of process context) abstracts DMM resource info */ > struct DSPHEAP_RES_OBJECT { > s32 heapAllocated; /* DMM status */ > @@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ > /* DMM mapped memory resources */ > struct DMM_MAP_OBJECT *dmm_map_list; > > + /* DMM reserved memory resources */ > + struct list_head dmm_rsv_list; > + spinlock_t dmm_rsv_list_lock; > + Why rsv requires a spinlock, but not map? I guess it would be needed if somehow PROC_UnReserveMemory() was called by user-space at the same time than bridge_close, but is that really possible? If it is, then the same danger is present with PROC_UnMap() unless I'm missing something. > /* DSP Heap resources */ > struct DSPHEAP_RES_OBJECT *pDSPHEAPList; > > diff --git a/arch/arm/plat-omap/include/dspbridge/proc.h b/arch/arm/plat-omap/include/dspbridge/proc.h > index 8dbdaac..558c053 100644 > --- a/arch/arm/plat-omap/include/dspbridge/proc.h > +++ b/arch/arm/plat-omap/include/dspbridge/proc.h > @@ -560,7 +560,7 @@ > * Details: > */ > extern DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, > - u32 ulSize, void **ppRsvAddr); > + u32 ulSize, void **ppRsvAddr, struct PROCESS_CONTEXT *pr_ctxt); > > /* > * ======== PROC_UnMap ======== > @@ -604,6 +604,7 @@ > * Details: > */ > extern DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, > - void *pRsvAddr); > + void *pRsvAddr, struct PROCESS_CONTEXT *pr_ctxt, > + int cleanup); > > #endif /* PROC_ */ > diff --git a/drivers/dsp/bridge/pmgr/dmm.c b/drivers/dsp/bridge/pmgr/dmm.c > index d5a7275..72aee60 100644 > --- a/drivers/dsp/bridge/pmgr/dmm.c > +++ b/drivers/dsp/bridge/pmgr/dmm.c > @@ -386,9 +386,10 @@ DSP_STATUS DMM_ReserveMemory(struct DMM_OBJECT *hDmmMgr, u32 size, > node->MappedSize = 0; > /* Return the chunk's starting address */ > *pRsvAddr = rsvAddr; > - } else > + } else { > /*dSP chunk of given size is not available */ > status = DSP_EMEMORY; > + } > > SYNC_LeaveCS(pDmmObj->hDmmLock); > GT_3trace(DMM_debugMask, GT_4CLASS, > diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c > index beea23b..747d069 100644 > --- a/drivers/dsp/bridge/pmgr/wcd.c > +++ b/drivers/dsp/bridge/pmgr/wcd.c > @@ -1054,12 +1054,13 @@ u32 PROCWRAP_ReserveMemory(union Trapped_Args *args, void *pr_ctxt) > > GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_ReserveMemory: entered\n"); > status = PROC_ReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, > - args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr); > + args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr, > + pr_ctxt); > if (DSP_SUCCEEDED(status)) { > if (put_user(pRsvAddr, args->ARGS_PROC_RSVMEM.ppRsvAddr)) { > status = DSP_EINVALIDARG; > PROC_UnReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, > - pRsvAddr); > + pRsvAddr, pr_ctxt, 1); > } > } > return status; > @@ -1100,7 +1101,7 @@ u32 PROCWRAP_UnReserveMemory(union Trapped_Args *args, void *pr_ctxt) > GT_0trace(WCD_debugMask, GT_ENTER, > "PROCWRAP_UnReserveMemory: entered\n"); > status = PROC_UnReserveMemory(args->ARGS_PROC_UNRSVMEM.hProcessor, > - args->ARGS_PROC_UNRSVMEM.pRsvAddr); > + args->ARGS_PROC_UNRSVMEM.pRsvAddr, pr_ctxt, 1); > return status; > } > > diff --git a/drivers/dsp/bridge/rmgr/drv.c b/drivers/dsp/bridge/rmgr/drv.c > index 3dc38d0..9d5c077 100644 > --- a/drivers/dsp/bridge/rmgr/drv.c > +++ b/drivers/dsp/bridge/rmgr/drv.c > @@ -277,25 +277,20 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) > if (DSP_FAILED(status)) > pr_debug("%s: PROC_UnMap failed! status =" > " 0x%xn", __func__, status); > - status = PROC_UnReserveMemory(pDMMRes->hProcessor, > - (void *)pDMMRes->ulDSPResAddr); > - if (DSP_FAILED(status)) > - pr_debug("%s: PROC_UnReserveMemory failed!" > - " status = 0x%xn", __func__, status); > pDMMRes->dmmAllocated = 0; > } > } > return status; > } > > -/* Release all DMM resources and its context > -* This is called from .bridge_release. */ > +/* Release all Mapped and Reserved DMM resources */ > DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) > { > struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; > DSP_STATUS status = DSP_SOK; > struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; > struct DMM_MAP_OBJECT *pTempDMMRes = NULL; > + struct DMM_RSV_OBJECT *temp, *rsv_obj; > > DRV_ProcFreeDMMRes(pCtxt); > pTempDMMRes = pCtxt->dmm_map_list; > @@ -305,6 +300,24 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) > kfree(pTempDMMRes2); > } > pCtxt->dmm_map_list = NULL; > + > + /* Free DMM reserved memory resources */ > + spin_lock(&pCtxt->dmm_rsv_list_lock); > + list_for_each_entry_safe(rsv_obj, temp, &pCtxt->dmm_rsv_list, link) { > + /* > + * Since we have already acquired spin lock for dmm_rsv_list, > + * we don't want to perform cleanup in PROC_UnReserveMemory. > + */ Sure, but why did we acquire a spin lock in the first place? > + status = PROC_UnReserveMemory(pCtxt->hProcessor, > + (void *)rsv_obj->dsp_reserved_addr, pCtxt, 0); If this behaved similarly to the unmap code, then 'cleanup' could be one and there wouldn't be any need to manually remove the entries. > + if (DSP_FAILED(status)) > + pr_err("%s: PROC_UnReserveMemory failed!" > + " status = 0x%xn", __func__, status); > + list_del(&rsv_obj->link); > + kfree(rsv_obj); > + } > + spin_unlock(&pCtxt->dmm_rsv_list_lock); > + > return status; > } Cheers. -- Felipe Contreras -- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework 2010-02-16 18:42 ` Felipe Contreras @ 2010-02-17 2:05 ` Guzman Lugo, Fernando 2010-02-17 9:50 ` Felipe Contreras 0 siblings, 1 reply; 8+ messages in thread From: Guzman Lugo, Fernando @ 2010-02-17 2:05 UTC (permalink / raw) To: Felipe Contreras, Ameya Palande Cc: linux-omap@vger.kernel.org, felipe.contreras@nokia.com, Menon, Nishanth, Chitriki Rudramuni, Deepak, Ramirez Luna, Omar Hi, >-----Original Message----- >From: Felipe Contreras [mailto:felipe.contreras@gmail.com] >Sent: Tuesday, February 16, 2010 12:43 PM >To: Ameya Palande >Cc: linux-omap@vger.kernel.org; felipe.contreras@nokia.com; Menon, >Nishanth; Chitriki Rudramuni, Deepak; Ramirez Luna, Omar; Guzman Lugo, >Fernando >Subject: Re: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting >framework > >On Tue, Feb 16, 2010 at 3:20 PM, Ameya Palande <ameya.palande@nokia.com> >wrote: >> DSP_RSV_OBJECT is introduced to track reserved memory accounting >information. >> This will allow us to do proper cleanup for memory reserved using >> PROC_ReserveMemory(). >> >> Signed-off-by: Ameya Palande <ameya.palande@nokia.com> >> --- >> arch/arm/plat-omap/include/dspbridge/drv.h | 13 ++++++ >> arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- >> drivers/dsp/bridge/pmgr/dmm.c | 3 +- >> drivers/dsp/bridge/pmgr/wcd.c | 7 ++- >> drivers/dsp/bridge/rmgr/drv.c | 27 +++++++++--- >> drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- >> drivers/dsp/bridge/rmgr/node.c | 5 +- >> drivers/dsp/bridge/rmgr/proc.c | 58 >+++++++++++++++++++++----- >> 8 files changed, 97 insertions(+), 28 deletions(-) >> >> diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat- >omap/include/dspbridge/drv.h >> index 3069e74..6e8187d 100644 >> --- a/arch/arm/plat-omap/include/dspbridge/drv.h >> +++ b/arch/arm/plat-omap/include/dspbridge/drv.h >> @@ -101,6 +101,15 @@ struct DMM_MAP_OBJECT { >> struct DMM_MAP_OBJECT *next; >> } ; >> >> +/* >> + * New structure (member of process context) used for accounting of DMM >> + * reserved memory information >> + */ >> +struct DMM_RSV_OBJECT { >> + struct list_head link; >> + u32 dsp_reserved_addr; >> +}; >> + >> /* New structure (member of process context) abstracts DMM resource info >*/ >> struct DSPHEAP_RES_OBJECT { >> s32 heapAllocated; /* DMM status */ >> @@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ >> /* DMM mapped memory resources */ >> struct DMM_MAP_OBJECT *dmm_map_list; >> >> + /* DMM reserved memory resources */ >> + struct list_head dmm_rsv_list; >> + spinlock_t dmm_rsv_list_lock; >> + > >Why rsv requires a spinlock, but not map? > >I guess it would be needed if somehow PROC_UnReserveMemory() was >called by user-space at the same time than bridge_close, but is that >really possible? If it is, then the same danger is present with >PROC_UnMap() unless I'm missing something. No, that's not possible. However protection is still needed for PROC_ ReserveMemory/UnReserveMemory and also for PROC_Map/UnMap. Regards, Fernando. > >> /* DSP Heap resources */ >> struct DSPHEAP_RES_OBJECT *pDSPHEAPList; >> >> diff --git a/arch/arm/plat-omap/include/dspbridge/proc.h b/arch/arm/plat- >omap/include/dspbridge/proc.h >> index 8dbdaac..558c053 100644 >> --- a/arch/arm/plat-omap/include/dspbridge/proc.h >> +++ b/arch/arm/plat-omap/include/dspbridge/proc.h >> @@ -560,7 +560,7 @@ >> * Details: >> */ >> extern DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, >> - u32 ulSize, void >**ppRsvAddr); >> + u32 ulSize, void **ppRsvAddr, struct PROCESS_CONTEXT >*pr_ctxt); >> >> /* >> * ======== PROC_UnMap ======== >> @@ -604,6 +604,7 @@ >> * Details: >> */ >> extern DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, >> - void *pRsvAddr); >> + void *pRsvAddr, struct PROCESS_CONTEXT *pr_ctxt, >> + int cleanup); >> >> #endif /* PROC_ */ >> diff --git a/drivers/dsp/bridge/pmgr/dmm.c >b/drivers/dsp/bridge/pmgr/dmm.c >> index d5a7275..72aee60 100644 >> --- a/drivers/dsp/bridge/pmgr/dmm.c >> +++ b/drivers/dsp/bridge/pmgr/dmm.c >> @@ -386,9 +386,10 @@ DSP_STATUS DMM_ReserveMemory(struct DMM_OBJECT >*hDmmMgr, u32 size, >> node->MappedSize = 0; >> /* Return the chunk's starting address */ >> *pRsvAddr = rsvAddr; >> - } else >> + } else { >> /*dSP chunk of given size is not available */ >> status = DSP_EMEMORY; >> + } >> >> SYNC_LeaveCS(pDmmObj->hDmmLock); >> GT_3trace(DMM_debugMask, GT_4CLASS, >> diff --git a/drivers/dsp/bridge/pmgr/wcd.c >b/drivers/dsp/bridge/pmgr/wcd.c >> index beea23b..747d069 100644 >> --- a/drivers/dsp/bridge/pmgr/wcd.c >> +++ b/drivers/dsp/bridge/pmgr/wcd.c >> @@ -1054,12 +1054,13 @@ u32 PROCWRAP_ReserveMemory(union Trapped_Args >*args, void *pr_ctxt) >> >> GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_ReserveMemory: >entered\n"); >> status = PROC_ReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, >> - args->ARGS_PROC_RSVMEM.ulSize, >&pRsvAddr); >> + args->ARGS_PROC_RSVMEM.ulSize, >&pRsvAddr, >> + pr_ctxt); >> if (DSP_SUCCEEDED(status)) { >> if (put_user(pRsvAddr, args->ARGS_PROC_RSVMEM.ppRsvAddr)) >{ >> status = DSP_EINVALIDARG; >> PROC_UnReserveMemory(args- >>ARGS_PROC_RSVMEM.hProcessor, >> - pRsvAddr); >> + pRsvAddr, pr_ctxt, 1); >> } >> } >> return status; >> @@ -1100,7 +1101,7 @@ u32 PROCWRAP_UnReserveMemory(union Trapped_Args >*args, void *pr_ctxt) >> GT_0trace(WCD_debugMask, GT_ENTER, >> "PROCWRAP_UnReserveMemory: entered\n"); >> status = PROC_UnReserveMemory(args->ARGS_PROC_UNRSVMEM.hProcessor, >> - args->ARGS_PROC_UNRSVMEM.pRsvAddr); >> + args->ARGS_PROC_UNRSVMEM.pRsvAddr, pr_ctxt, 1); >> return status; >> } >> >> diff --git a/drivers/dsp/bridge/rmgr/drv.c >b/drivers/dsp/bridge/rmgr/drv.c >> index 3dc38d0..9d5c077 100644 >> --- a/drivers/dsp/bridge/rmgr/drv.c >> +++ b/drivers/dsp/bridge/rmgr/drv.c >> @@ -277,25 +277,20 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) >> if (DSP_FAILED(status)) >> pr_debug("%s: PROC_UnMap failed! status =" >> " 0x%xn", __func__, >status); >> - status = PROC_UnReserveMemory(pDMMRes- >>hProcessor, >> - (void *)pDMMRes->ulDSPResAddr); >> - if (DSP_FAILED(status)) >> - pr_debug("%s: PROC_UnReserveMemory >failed!" >> - " status = 0x%xn", __func__, >status); >> pDMMRes->dmmAllocated = 0; >> } >> } >> return status; >> } >> >> -/* Release all DMM resources and its context >> -* This is called from .bridge_release. */ >> +/* Release all Mapped and Reserved DMM resources */ >> DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) >> { >> struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; >> DSP_STATUS status = DSP_SOK; >> struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; >> struct DMM_MAP_OBJECT *pTempDMMRes = NULL; >> + struct DMM_RSV_OBJECT *temp, *rsv_obj; >> >> DRV_ProcFreeDMMRes(pCtxt); >> pTempDMMRes = pCtxt->dmm_map_list; >> @@ -305,6 +300,24 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE >hPCtxt) >> kfree(pTempDMMRes2); >> } >> pCtxt->dmm_map_list = NULL; >> + >> + /* Free DMM reserved memory resources */ >> + spin_lock(&pCtxt->dmm_rsv_list_lock); >> + list_for_each_entry_safe(rsv_obj, temp, &pCtxt->dmm_rsv_list, >link) { >> + /* >> + * Since we have already acquired spin lock for >dmm_rsv_list, >> + * we don't want to perform cleanup in >PROC_UnReserveMemory. >> + */ > >Sure, but why did we acquire a spin lock in the first place? > >> + status = PROC_UnReserveMemory(pCtxt->hProcessor, >> + (void *)rsv_obj->dsp_reserved_addr, >pCtxt, 0); > >If this behaved similarly to the unmap code, then 'cleanup' could be >one and there wouldn't be any need to manually remove the entries. > >> + if (DSP_FAILED(status)) >> + pr_err("%s: PROC_UnReserveMemory failed!" >> + " status = 0x%xn", __func__, >status); >> + list_del(&rsv_obj->link); >> + kfree(rsv_obj); >> + } >> + spin_unlock(&pCtxt->dmm_rsv_list_lock); >> + >> return status; >> } > >Cheers. > >-- >Felipe Contreras -- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework 2010-02-17 2:05 ` Guzman Lugo, Fernando @ 2010-02-17 9:50 ` Felipe Contreras 0 siblings, 0 replies; 8+ messages in thread From: Felipe Contreras @ 2010-02-17 9:50 UTC (permalink / raw) To: Guzman Lugo, Fernando Cc: Ameya Palande, linux-omap@vger.kernel.org, felipe.contreras@nokia.com, Menon, Nishanth, Chitriki Rudramuni, Deepak, Ramirez Luna, Omar On Wed, Feb 17, 2010 at 4:05 AM, Guzman Lugo, Fernando <x0095840@ti.com> wrote: >>> struct DSPHEAP_RES_OBJECT { >>> s32 heapAllocated; /* DMM status */ >>> @@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ >>> /* DMM mapped memory resources */ >>> struct DMM_MAP_OBJECT *dmm_map_list; >>> >>> + /* DMM reserved memory resources */ >>> + struct list_head dmm_rsv_list; >>> + spinlock_t dmm_rsv_list_lock; >>> + >> >>Why rsv requires a spinlock, but not map? >> >>I guess it would be needed if somehow PROC_UnReserveMemory() was >>called by user-space at the same time than bridge_close, but is that >>really possible? If it is, then the same danger is present with >>PROC_UnMap() unless I'm missing something. > > No, that's not possible. However protection is still needed for PROC_ ReserveMemory/UnReserveMemory and also for PROC_Map/UnMap. Ok, then I guess it would make sense to have a patch for DMM_RSV_OBJECT, and another one to add the protection to both PROC_Map/UnMap/Reserve/UnReserve(). -- Felipe Contreras -- 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework 2010-02-16 13:20 ` [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework Ameya Palande 2010-02-16 18:42 ` Felipe Contreras @ 2010-02-17 2:01 ` Guzman Lugo, Fernando 1 sibling, 0 replies; 8+ messages in thread From: Guzman Lugo, Fernando @ 2010-02-17 2:01 UTC (permalink / raw) To: Ameya Palande, linux-omap@vger.kernel.org Cc: felipe.contreras@nokia.com, Menon, Nishanth, Chitriki Rudramuni, Deepak, Ramirez Luna, Omar Hi, >-----Original Message----- >From: Ameya Palande [mailto:ameya.palande@nokia.com] >Sent: Tuesday, February 16, 2010 7:20 AM >To: linux-omap@vger.kernel.org >Cc: felipe.contreras@nokia.com; Menon, Nishanth; Chitriki Rudramuni, >Deepak; Ramirez Luna, Omar; Guzman Lugo, Fernando >Subject: [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework > >DSP_RSV_OBJECT is introduced to track reserved memory accounting >information. >This will allow us to do proper cleanup for memory reserved using >PROC_ReserveMemory(). > >Signed-off-by: Ameya Palande <ameya.palande@nokia.com> >--- > arch/arm/plat-omap/include/dspbridge/drv.h | 13 ++++++ > arch/arm/plat-omap/include/dspbridge/proc.h | 5 +- > drivers/dsp/bridge/pmgr/dmm.c | 3 +- > drivers/dsp/bridge/pmgr/wcd.c | 7 ++- > drivers/dsp/bridge/rmgr/drv.c | 27 +++++++++--- > drivers/dsp/bridge/rmgr/drv_interface.c | 7 ++- > drivers/dsp/bridge/rmgr/node.c | 5 +- > drivers/dsp/bridge/rmgr/proc.c | 58 +++++++++++++++++++++-- >--- > 8 files changed, 97 insertions(+), 28 deletions(-) > >diff --git a/arch/arm/plat-omap/include/dspbridge/drv.h b/arch/arm/plat- >omap/include/dspbridge/drv.h >index 3069e74..6e8187d 100644 >--- a/arch/arm/plat-omap/include/dspbridge/drv.h >+++ b/arch/arm/plat-omap/include/dspbridge/drv.h >@@ -101,6 +101,15 @@ struct DMM_MAP_OBJECT { > struct DMM_MAP_OBJECT *next; > } ; > >+/* >+ * New structure (member of process context) used for accounting of DMM >+ * reserved memory information >+ */ >+struct DMM_RSV_OBJECT { >+ struct list_head link; >+ u32 dsp_reserved_addr; >+}; >+ > /* New structure (member of process context) abstracts DMM resource info >*/ > struct DSPHEAP_RES_OBJECT { > s32 heapAllocated; /* DMM status */ >@@ -141,6 +150,10 @@ struct PROCESS_CONTEXT{ > /* DMM mapped memory resources */ > struct DMM_MAP_OBJECT *dmm_map_list; > >+ /* DMM reserved memory resources */ >+ struct list_head dmm_rsv_list; >+ spinlock_t dmm_rsv_list_lock; >+ > /* DSP Heap resources */ > struct DSPHEAP_RES_OBJECT *pDSPHEAPList; > >diff --git a/arch/arm/plat-omap/include/dspbridge/proc.h b/arch/arm/plat- >omap/include/dspbridge/proc.h >index 8dbdaac..558c053 100644 >--- a/arch/arm/plat-omap/include/dspbridge/proc.h >+++ b/arch/arm/plat-omap/include/dspbridge/proc.h >@@ -560,7 +560,7 @@ > * Details: > */ > extern DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, >- u32 ulSize, void **ppRsvAddr); >+ u32 ulSize, void **ppRsvAddr, struct PROCESS_CONTEXT *pr_ctxt); > > /* > * ======== PROC_UnMap ======== >@@ -604,6 +604,7 @@ > * Details: > */ > extern DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, >- void *pRsvAddr); >+ void *pRsvAddr, struct PROCESS_CONTEXT *pr_ctxt, >+ int cleanup); > > #endif /* PROC_ */ >diff --git a/drivers/dsp/bridge/pmgr/dmm.c b/drivers/dsp/bridge/pmgr/dmm.c >index d5a7275..72aee60 100644 >--- a/drivers/dsp/bridge/pmgr/dmm.c >+++ b/drivers/dsp/bridge/pmgr/dmm.c >@@ -386,9 +386,10 @@ DSP_STATUS DMM_ReserveMemory(struct DMM_OBJECT >*hDmmMgr, u32 size, > node->MappedSize = 0; > /* Return the chunk's starting address */ > *pRsvAddr = rsvAddr; >- } else >+ } else { > /*dSP chunk of given size is not available */ > status = DSP_EMEMORY; >+ } > > SYNC_LeaveCS(pDmmObj->hDmmLock); > GT_3trace(DMM_debugMask, GT_4CLASS, >diff --git a/drivers/dsp/bridge/pmgr/wcd.c b/drivers/dsp/bridge/pmgr/wcd.c >index beea23b..747d069 100644 >--- a/drivers/dsp/bridge/pmgr/wcd.c >+++ b/drivers/dsp/bridge/pmgr/wcd.c >@@ -1054,12 +1054,13 @@ u32 PROCWRAP_ReserveMemory(union Trapped_Args >*args, void *pr_ctxt) > > GT_0trace(WCD_debugMask, GT_ENTER, "PROCWRAP_ReserveMemory: >entered\n"); > status = PROC_ReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, >- args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr); >+ args->ARGS_PROC_RSVMEM.ulSize, &pRsvAddr, >+ pr_ctxt); > if (DSP_SUCCEEDED(status)) { > if (put_user(pRsvAddr, args->ARGS_PROC_RSVMEM.ppRsvAddr)) { > status = DSP_EINVALIDARG; > PROC_UnReserveMemory(args->ARGS_PROC_RSVMEM.hProcessor, >- pRsvAddr); >+ pRsvAddr, pr_ctxt, 1); > } > } > return status; >@@ -1100,7 +1101,7 @@ u32 PROCWRAP_UnReserveMemory(union Trapped_Args >*args, void *pr_ctxt) > GT_0trace(WCD_debugMask, GT_ENTER, > "PROCWRAP_UnReserveMemory: entered\n"); > status = PROC_UnReserveMemory(args->ARGS_PROC_UNRSVMEM.hProcessor, >- args->ARGS_PROC_UNRSVMEM.pRsvAddr); >+ args->ARGS_PROC_UNRSVMEM.pRsvAddr, pr_ctxt, 1); > return status; > } > >diff --git a/drivers/dsp/bridge/rmgr/drv.c b/drivers/dsp/bridge/rmgr/drv.c >index 3dc38d0..9d5c077 100644 >--- a/drivers/dsp/bridge/rmgr/drv.c >+++ b/drivers/dsp/bridge/rmgr/drv.c >@@ -277,25 +277,20 @@ DSP_STATUS DRV_ProcFreeDMMRes(HANDLE hPCtxt) > if (DSP_FAILED(status)) > pr_debug("%s: PROC_UnMap failed! status =" > " 0x%xn", __func__, status); >- status = PROC_UnReserveMemory(pDMMRes->hProcessor, >- (void *)pDMMRes->ulDSPResAddr); >- if (DSP_FAILED(status)) >- pr_debug("%s: PROC_UnReserveMemory failed!" >- " status = 0x%xn", __func__, status); > pDMMRes->dmmAllocated = 0; > } > } > return status; > } > >-/* Release all DMM resources and its context >-* This is called from .bridge_release. */ >+/* Release all Mapped and Reserved DMM resources */ > DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) > { > struct PROCESS_CONTEXT *pCtxt = (struct PROCESS_CONTEXT *)hPCtxt; > DSP_STATUS status = DSP_SOK; > struct DMM_MAP_OBJECT *pTempDMMRes2 = NULL; > struct DMM_MAP_OBJECT *pTempDMMRes = NULL; >+ struct DMM_RSV_OBJECT *temp, *rsv_obj; > > DRV_ProcFreeDMMRes(pCtxt); > pTempDMMRes = pCtxt->dmm_map_list; >@@ -305,6 +300,24 @@ DSP_STATUS DRV_RemoveAllDMMResElements(HANDLE hPCtxt) > kfree(pTempDMMRes2); > } > pCtxt->dmm_map_list = NULL; >+ >+ /* Free DMM reserved memory resources */ >+ spin_lock(&pCtxt->dmm_rsv_list_lock); >+ list_for_each_entry_safe(rsv_obj, temp, &pCtxt->dmm_rsv_list, link) { >+ /* >+ * Since we have already acquired spin lock for dmm_rsv_list, >+ * we don't want to perform cleanup in PROC_UnReserveMemory. >+ */ DRV_ProcFreeDMMRes is only called from bridge_release and there is no other thread from the same process that can be executed concurrent. It is not needed lock at cleanup time. After removing the lock, I think you don't need cleanup flag in PROC_UnReserveMemory. Because the cleanup can be done in PROC_UnReserveMemory without problems. Regards, Fernando. >+ status = PROC_UnReserveMemory(pCtxt->hProcessor, >+ (void *)rsv_obj->dsp_reserved_addr, pCtxt, 0); >+ if (DSP_FAILED(status)) >+ pr_err("%s: PROC_UnReserveMemory failed!" >+ " status = 0x%xn", __func__, status); >+ list_del(&rsv_obj->link); >+ kfree(rsv_obj); >+ } >+ spin_unlock(&pCtxt->dmm_rsv_list_lock); >+ > return status; > } > >diff --git a/drivers/dsp/bridge/rmgr/drv_interface.c >b/drivers/dsp/bridge/rmgr/drv_interface.c >index 0ce9a5f..ef078cf 100644 >--- a/drivers/dsp/bridge/rmgr/drv_interface.c >+++ b/drivers/dsp/bridge/rmgr/drv_interface.c >@@ -497,10 +497,13 @@ static int bridge_open(struct inode *ip, struct file >*filp) > * process context list. > */ > pr_ctxt = MEM_Calloc(sizeof(struct PROCESS_CONTEXT), MEM_PAGED); >- if (pr_ctxt) >+ if (pr_ctxt) { > pr_ctxt->resState = PROC_RES_ALLOCATED; >- else >+ spin_lock_init(&pr_ctxt->dmm_rsv_list_lock); >+ INIT_LIST_HEAD(&pr_ctxt->dmm_rsv_list); >+ } else { > status = -ENOMEM; >+ } > > filp->private_data = pr_ctxt; > >diff --git a/drivers/dsp/bridge/rmgr/node.c >b/drivers/dsp/bridge/rmgr/node.c >index b60d1ed..f85b4b8 100644 >--- a/drivers/dsp/bridge/rmgr/node.c >+++ b/drivers/dsp/bridge/rmgr/node.c >@@ -454,7 +454,7 @@ DSP_STATUS NODE_Allocate(struct PROC_OBJECT >*hProcessor, > status = PROC_ReserveMemory(hProcessor, > pNode->createArgs.asa.taskArgs.uHeapSize + PAGE_SIZE, > (void **)&(pNode->createArgs.asa.taskArgs. >- uDSPHeapResAddr)); >+ uDSPHeapResAddr), pr_ctxt); > if (DSP_FAILED(status)) { > GT_1trace(NODE_debugMask, GT_5CLASS, > "NODE_Allocate:Failed to reserve " >@@ -2726,7 +2726,8 @@ static void DeleteNode(struct NODE_OBJECT *hNode, > " Status = 0x%x\n", (u32)status); > } > status = PROC_UnReserveMemory(hNode->hProcessor, >- (void *)taskArgs.uDSPHeapResAddr); >+ (void *)taskArgs.uDSPHeapResAddr, >+ pr_ctxt, 1); > if (DSP_SUCCEEDED(status)) { > GT_0trace(NODE_debugMask, GT_5CLASS, > "DSPProcessor_UnReserveMemory " >diff --git a/drivers/dsp/bridge/rmgr/proc.c >b/drivers/dsp/bridge/rmgr/proc.c >index 2ccbc9b..2ef2e37 100644 >--- a/drivers/dsp/bridge/rmgr/proc.c >+++ b/drivers/dsp/bridge/rmgr/proc.c >@@ -1430,11 +1430,12 @@ func_end: > * Reserve a virtually contiguous region of DSP address space. > */ > DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR hProcessor, u32 ulSize, >- void **ppRsvAddr) >+ void **ppRsvAddr, struct PROCESS_CONTEXT *pr_ctxt) > { > struct DMM_OBJECT *hDmmMgr; > DSP_STATUS status = DSP_SOK; > struct PROC_OBJECT *pProcObject = (struct PROC_OBJECT *)hProcessor; >+ struct DMM_RSV_OBJECT *rsv_obj; > > GT_3trace(PROC_DebugMask, GT_ENTER, > "Entered PROC_ReserveMemory, args:\n\t" >@@ -1446,16 +1447,29 @@ DSP_STATUS PROC_ReserveMemory(DSP_HPROCESSOR >hProcessor, u32 ulSize, > "InValid Processor Handle \n"); > goto func_end; > } >+ > status = DMM_GetHandle(pProcObject, &hDmmMgr); > if (DSP_FAILED(status)) { > GT_1trace(PROC_DebugMask, GT_7CLASS, "PROC_ReserveMemory: " > "Failed to get DMM Mgr handle: 0x%x\n", status); >- } else >- status = DMM_ReserveMemory(hDmmMgr, ulSize, (u32 *)ppRsvAddr); >+ goto func_end; >+ } >+ >+ status = DMM_ReserveMemory(hDmmMgr, ulSize, (u32 *)ppRsvAddr); >+ if (status != DSP_SOK) >+ goto func_end; >+ >+ rsv_obj = kmalloc(sizeof(struct DMM_RSV_OBJECT), GFP_KERNEL); >+ if (rsv_obj) { >+ rsv_obj->dsp_reserved_addr = (u32) *ppRsvAddr; >+ spin_lock(&pr_ctxt->dmm_rsv_list_lock); >+ list_add(&rsv_obj->link, &pr_ctxt->dmm_rsv_list); >+ spin_unlock(&pr_ctxt->dmm_rsv_list_lock); >+ } > >+func_end: > GT_1trace(PROC_DebugMask, GT_ENTER, "Leaving PROC_ReserveMemory >[0x%x]", > status); >-func_end: > return status; > } > >@@ -1704,11 +1718,13 @@ func_end: > * Purpose: > * Frees a previously reserved region of DSP address space. > */ >-DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, void *pRsvAddr) >+DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR hProcessor, void *pRsvAddr, >+ struct PROCESS_CONTEXT *pr_ctxt, int cleanup) > { > struct DMM_OBJECT *hDmmMgr; > DSP_STATUS status = DSP_SOK; > struct PROC_OBJECT *pProcObject = (struct PROC_OBJECT *)hProcessor; >+ struct DMM_RSV_OBJECT *temp, *rsv_obj; > > GT_2trace(PROC_DebugMask, GT_ENTER, > "Entered PROC_UnReserveMemory, args:\n\t" >@@ -1719,18 +1735,38 @@ DSP_STATUS PROC_UnReserveMemory(DSP_HPROCESSOR >hProcessor, void *pRsvAddr) > "InValid Processor Handle \n"); > goto func_end; > } >+ > status = DMM_GetHandle(pProcObject, &hDmmMgr); >- if (DSP_FAILED(status)) >+ if (DSP_FAILED(status)) { > GT_1trace(PROC_DebugMask, GT_7CLASS, > "PROC_UnReserveMemory: Failed to get DMM Mgr " > "handle: 0x%x\n", status); >- else >- status = DMM_UnReserveMemory(hDmmMgr, (u32) pRsvAddr); >+ goto func_end; >+ } >+ >+ status = DMM_UnReserveMemory(hDmmMgr, (u32) pRsvAddr); >+ >+ /* >+ * cleanup flag handles the special case when this function >+ * is called while doing clenaup from >+ * DRV_RemoveAllDMMResElements >+ */ >+ if (status != DSP_SOK || !cleanup) >+ goto func_end; >+ >+ spin_lock(&pr_ctxt->dmm_rsv_list_lock); >+ list_for_each_entry_safe(rsv_obj, temp, &pr_ctxt->dmm_rsv_list, link) >{ >+ if (rsv_obj->dsp_reserved_addr == (u32)pRsvAddr) { >+ list_del(&rsv_obj->link); >+ kfree(rsv_obj); >+ break; >+ } >+ } >+ spin_unlock(&pr_ctxt->dmm_rsv_list_lock); > >- GT_1trace(PROC_DebugMask, GT_ENTER, >- "Leaving PROC_UnReserveMemory [0x%x]", >- status); > func_end: >+ GT_1trace(PROC_DebugMask, GT_ENTER, >+ "Leaving PROC_UnReserveMemory [0x%x]", status); > return status; > } > >-- >1.6.3.3 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-02-17 9:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-02-16 13:20 [PATCHv2 0/2] DSPBRIDGE: Improved reserved memory cleanup Ameya Palande 2010-02-16 13:20 ` [PATCH 1/2] DSPBRIDGE: Rename DMM_RES_OBJECT to DMM_MAP_OBJECT Ameya Palande 2010-02-16 18:27 ` Felipe Contreras 2010-02-16 13:20 ` [PATCH 2/2] DSPBRIDGE: New reserved memory accounting framework Ameya Palande 2010-02-16 18:42 ` Felipe Contreras 2010-02-17 2:05 ` Guzman Lugo, Fernando 2010-02-17 9:50 ` Felipe Contreras 2010-02-17 2:01 ` Guzman Lugo, Fernando
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox