From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id BED8FA0544; Wed, 12 Oct 2022 09:28:32 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 67C9142D86; Wed, 12 Oct 2022 09:28:32 +0200 (CEST) Received: from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187]) by mails.dpdk.org (Postfix) with ESMTP id 2A30842BF0 for ; Wed, 12 Oct 2022 09:28:30 +0200 (CEST) Received: from dggpeml500024.china.huawei.com (unknown [172.30.72.56]) by szxga01-in.huawei.com (SkyGuard) with ESMTP id 4MnPLj1MxGzpVXp; Wed, 12 Oct 2022 15:25:17 +0800 (CST) Received: from [10.67.100.224] (10.67.100.224) by dggpeml500024.china.huawei.com (7.185.36.10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2375.31; Wed, 12 Oct 2022 15:28:28 +0800 Subject: Re: [PATCH v8 3/9] memarea: support alloc/free/update-refcnt API To: Dmitry Kozlyuk CC: , , , , , , References: <20220721044648.6817-1-fengchengwen@huawei.com> <20221011121720.2657-1-fengchengwen@huawei.com> <20221011121720.2657-4-fengchengwen@huawei.com> <20221011185819.7ac322fe@sovereign> From: fengchengwen Message-ID: Date: Wed, 12 Oct 2022 15:28:27 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.11.0 MIME-Version: 1.0 In-Reply-To: <20221011185819.7ac322fe@sovereign> Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.100.224] X-ClientProxiedBy: dggems702-chm.china.huawei.com (10.3.19.179) To dggpeml500024.china.huawei.com (7.185.36.10) X-CFilter-Loop: Reflected X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Hi Dmitry, On 2022/10/11 23:58, Dmitry Kozlyuk wrote: > 2022-10-11 12:17 (UTC+0000), Chengwen Feng: >> This patch supports rte_memarea_alloc()/rte_memarea_free()/ >> rte_memarea_update_refcnt() API. >> ... >> + >> +void >> +rte_memarea_update_refcnt(struct rte_memarea *ma, void *ptr, int16_t value) >> +{ >> + struct memarea_elem *elem = (struct memarea_elem *)RTE_PTR_SUB(ptr, >> + sizeof(struct memarea_elem)); >> + >> + if (unlikely(ma == NULL || ptr == NULL)) >> + return; > > This check is probably not needed: > the user is only supposed to change refcnt of a valid object. > Since rte_memarea_free(ma, NULL) must be a no-op, the check should be there. okay > >> + >> + memarea_lock(ma); >> + if (unlikely(elem->magic != MEMAREA_ALLOCATED_ELEM_MAGIC)) { >> + RTE_LOG(ERR, MEMAREA, "memarea: %s magic: 0x%x check fail!\n", >> + ma->init.name, elem->magic); > > Element pointer might be useful here, > because cookie will likely not be unique for each object. RTE_LOG pointer may lead to security risks. I tend to keep the status quo. > >> + memarea_unlock(ma); >> + return; >> + } >> + >> + if (unlikely(elem->refcnt <= 0 || elem->refcnt + value < 0)) { >> + RTE_LOG(ERR, MEMAREA, >> + "memarea: %s cookie: 0x%x curr refcnt: %d update refcnt: %d check fail!\n", >> + ma->init.name, elem->cookie, elem->refcnt, value); > > Same as above. > >> + ma->refcnt_check_fails++; >> + if (elem->refcnt > 0) >> + elem->refcnt += value; > > This may change refcnt of some unrelated block allocated concurrently, > producing another bug on top of the bug that has lead to this branch > (the case is different from the race described below). > >> + memarea_unlock(ma); >> + return; >> + } >> + >> + elem->refcnt += value; >> + if (elem->refcnt == 0) >> + memarea_free_elem(ma, elem); >> + memarea_unlock(ma); >> +} > > This is not thread-safe: > > 1. Thread A wants to decrement refnct and takes the lock. > 2. Thread B wants to increment refcnt and waits for the lock. The refcnt must at least 2 because two threads hold the object. > 3. Thread C wants to allocate and waits for the lock. > 4. Thread A decrements refcnt, frees the object, and releases the lock. The refcnt will updated to be 1, and the object will not freed. > 5. Thread C takes the lock, allocates the same element, and releases the lock> 6. Thread B takes the lock and increments refcnt of the new element allocated > by thread C and not the old element it had a pointer to in the beginning. Because object's refcnt at least 1, so the increments refcnt will success. The refcnt just like mbuf's refcnt, If app wants to send one object to multiple entity (rings or other), it should updated refcnt by increment (multiple entity num - 1) and then send. It work well when keep the above rule. > > Probably some memarea generation counter could solve this. > > I still tend to think that reference counting is too usage-specific > to be a part of a generic memory management library, > though others don't seem to object. > >> diff --git a/lib/memarea/rte_memarea.h b/lib/memarea/rte_memarea.h >> index ac2d63a4d3..f866fa7b67 100644 >> --- a/lib/memarea/rte_memarea.h >> +++ b/lib/memarea/rte_memarea.h >> @@ -129,6 +129,62 @@ struct rte_memarea *rte_memarea_create(const struct rte_memarea_param *init); >> __rte_experimental >> void rte_memarea_destroy(struct rte_memarea *ma); >> >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * Allocate memory from memarea. >> + * >> + * Allocate one memory object from the memarea. >> + * >> + * @param ma >> + * The pointer of memarea. >> + * @param size >> + * The memory size to be allocated. >> + * @param cookie >> + * User-provided footprint which could used to debug memory leak. >> + * >> + * @return >> + * Non-NULL on success. Otherwise NULL is returned. > > Also NULL when size == 0 (OK, like malloc) or when ma == NULL (not obvious). ok > >> + */ >> +__rte_experimental >> +void *rte_memarea_alloc(struct rte_memarea *ma, size_t size, uint32_t cookie); >> + >> +/** >> + * @warning >> + * @b EXPERIMENTAL: this API may change without prior notice. >> + * >> + * Free memory to memarea. >> + * >> + * Free one memory object to the memarea. > > Not really if refcnt > 1. > Should it assert somehow that refcnt <= 1? > > The second sentence adds nothing to the first. will add more description. > >> + * >> + * @param ma >> + * The pointer of memarea. >> + * @param ptr >> + * The pointer of memory object which need be freed. >> + */ >> +__rte_experimental >> +void rte_memarea_free(struct rte_memarea *ma, void *ptr); > . >