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 15B6EA0C4B; Fri, 15 Oct 2021 10:52:38 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 03B5C40692; Fri, 15 Oct 2021 10:52:38 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id B5ABE40041 for ; Fri, 15 Oct 2021 10:52:36 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 470B07F690; Fri, 15 Oct 2021 11:52:36 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 470B07F690 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1634287956; bh=x6ENlFxfW3UlnVS5fozyRwufIVyCn0BQAoECV0kJEMc=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=E65VEz6yqqg5yPBx0GZUCJ0h5UGUCz2STaNDLHDDlYD4K1In2WZXXQQEPwG63J1Y5 TCmx73+N/7LyXJAfBkYT36qQWTPpG/EOakFtcYQYmtEmRPZ7Z3f5Z8Dl6jbeqwoNd+ 32fqAjSVfK3KR7hWfKlTCtVUv+JWKtp5HUFacssE= To: Dmitry Kozlyuk , dev@dpdk.org Cc: Matan Azrad , Olivier Matz , Ray Kinsella , Anatoly Burakov References: <20211012000409.2751908-1-dkozlyuk@nvidia.com> <20211013110131.2909604-1-dkozlyuk@nvidia.com> <20211013110131.2909604-2-dkozlyuk@nvidia.com> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: <1dd0b17c-0009-46e7-0f14-c88ce2b58540@oktetlabs.ru> Date: Fri, 15 Oct 2021 11:52:36 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: <20211013110131.2909604-2-dkozlyuk@nvidia.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 1/4] mempool: add event callbacks 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 Sender: "dev" On 10/13/21 2:01 PM, Dmitry Kozlyuk wrote: > Data path performance can benefit if the PMD knows which memory it will > need to handle in advance, before the first mbuf is sent to the PMD. > It is impractical, however, to consider all allocated memory for this > purpose. Most often mbuf memory comes from mempools that can come and > go. PMD can enumerate existing mempools on device start, but it also > needs to track creation and destruction of mempools after the forwarding > starts but before an mbuf from the new mempool is sent to the device. > > Add an API to register callback for mempool life cycle events: > * rte_mempool_event_callback_register() > * rte_mempool_event_callback_unregister() > Currently tracked events are: > * RTE_MEMPOOL_EVENT_READY (after populating a mempool) > * RTE_MEMPOOL_EVENT_DESTROY (before freeing a mempool) > Provide a unit test for the new API. > The new API is internal, because it is primarily demanded by PMDs that > may need to deal with any mempools and do not control their creation, > while an application, on the other hand, knows which mempools it creates > and doesn't care about internal mempools PMDs might create. > > Signed-off-by: Dmitry Kozlyuk > Acked-by: Matan Azrad With below review notes processed Reviewed-by: Andrew Rybchenko [snip] > diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c > index c5f859ae71..51c0ba2931 100644 > --- a/lib/mempool/rte_mempool.c > +++ b/lib/mempool/rte_mempool.c [snip] > @@ -360,6 +372,10 @@ rte_mempool_populate_iova(struct rte_mempool *mp, char *vaddr, > STAILQ_INSERT_TAIL(&mp->mem_list, memhdr, next); > mp->nb_mem_chunks++; > > + /* Report the mempool as ready only when fully populated. */ > + if (mp->populated_size >= mp->size) > + mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_READY, mp); > + > rte_mempool_trace_populate_iova(mp, vaddr, iova, len, free_cb, opaque); > return i; > > @@ -722,6 +738,7 @@ rte_mempool_free(struct rte_mempool *mp) > } > rte_mcfg_tailq_write_unlock(); > > + mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_DESTROY, mp); > rte_mempool_trace_free(mp); > rte_mempool_free_memchunks(mp); > rte_mempool_ops_free(mp); > @@ -1343,3 +1360,123 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *), > > rte_mcfg_mempool_read_unlock(); > } > + > +struct mempool_callback { It sounds like it is a mempool callback itself. Consider: mempool_event_callback_data. I think this way it will be consistent. > + rte_mempool_event_callback *func; > + void *user_data; > +}; > + > +static void > +mempool_event_callback_invoke(enum rte_mempool_event event, > + struct rte_mempool *mp) > +{ > + struct mempool_callback_list *list; > + struct rte_tailq_entry *te; > + void *tmp_te; > + > + rte_mcfg_tailq_read_lock(); > + list = RTE_TAILQ_CAST(callback_tailq.head, mempool_callback_list); > + RTE_TAILQ_FOREACH_SAFE(te, list, next, tmp_te) { > + struct mempool_callback *cb = te->data; > + rte_mcfg_tailq_read_unlock(); > + cb->func(event, mp, cb->user_data); > + rte_mcfg_tailq_read_lock(); > + } > + rte_mcfg_tailq_read_unlock(); > +} > + > +int > +rte_mempool_event_callback_register(rte_mempool_event_callback *func, > + void *user_data) > +{ > + struct mempool_callback_list *list; > + struct rte_tailq_entry *te = NULL; > + struct mempool_callback *cb; > + void *tmp_te; > + int ret; > + > + if (func == NULL) { > + rte_errno = EINVAL; > + return -rte_errno; > + } > + > + rte_mcfg_mempool_read_lock(); > + rte_mcfg_tailq_write_lock(); > + > + list = RTE_TAILQ_CAST(callback_tailq.head, mempool_callback_list); > + RTE_TAILQ_FOREACH_SAFE(te, list, next, tmp_te) { > + struct mempool_callback *cb = > + (struct mempool_callback *)te->data; > + if (cb->func == func && cb->user_data == user_data) { > + ret = -EEXIST; > + goto exit; > + } > + } > + > + te = rte_zmalloc("MEMPOOL_TAILQ_ENTRY", sizeof(*te), 0); > + if (te == NULL) { > + RTE_LOG(ERR, MEMPOOL, > + "Cannot allocate event callback tailq entry!\n"); > + ret = -ENOMEM; > + goto exit; > + } > + > + cb = rte_malloc("MEMPOOL_EVENT_CALLBACK", sizeof(*cb), 0); > + if (cb == NULL) { > + RTE_LOG(ERR, MEMPOOL, > + "Cannot allocate event callback!\n"); > + rte_free(te); > + ret = -ENOMEM; > + goto exit; > + } > + > + cb->func = func; > + cb->user_data = user_data; > + te->data = cb; > + TAILQ_INSERT_TAIL(list, te, next); > + ret = 0; > + > +exit: > + rte_mcfg_tailq_write_unlock(); > + rte_mcfg_mempool_read_unlock(); > + rte_errno = -ret; > + return ret; > +} > + > +int > +rte_mempool_event_callback_unregister(rte_mempool_event_callback *func, > + void *user_data) > +{ > + struct mempool_callback_list *list; > + struct rte_tailq_entry *te = NULL; > + struct mempool_callback *cb; > + int ret; > + > + if (rte_eal_process_type() != RTE_PROC_PRIMARY) { > + rte_errno = EPERM; > + return -1; The function should behave consistencly. Below you return negative error. Here just -1. I think it would be more constent to return -rte_errno here. > + } > + > + rte_mcfg_mempool_read_lock(); > + rte_mcfg_tailq_write_lock(); > + ret = -ENOENT; > + list = RTE_TAILQ_CAST(callback_tailq.head, mempool_callback_list); > + TAILQ_FOREACH(te, list, next) { > + cb = (struct mempool_callback *)te->data; > + if (cb->func == func && cb->user_data == user_data) > + break; > + } > + if (te != NULL) { Here we rely on the fact that TAILQ_FOREACH() exists with te==NULL in the case of no such entry. I'd suggest to avoid the assumption. I.e. do below two lines above before break and have not the if condition her at all. > + TAILQ_REMOVE(list, te, next); > + ret = 0; > + } > + rte_mcfg_tailq_write_unlock(); > + rte_mcfg_mempool_read_unlock(); > + > + if (ret == 0) { > + rte_free(te); > + rte_free(cb); > + } > + rte_errno = -ret; > + return ret; > +} > diff --git a/lib/mempool/rte_mempool.h b/lib/mempool/rte_mempool.h > index f57ecbd6fc..663123042f 100644 > --- a/lib/mempool/rte_mempool.h > +++ b/lib/mempool/rte_mempool.h > @@ -1774,6 +1774,67 @@ void rte_mempool_walk(void (*func)(struct rte_mempool *, void *arg), > int > rte_mempool_get_page_size(struct rte_mempool *mp, size_t *pg_sz); > > +/** > + * Mempool event type. > + * @internal Shouldn't internal go first? > + */ > +enum rte_mempool_event { > + /** Occurs after a mempool is successfully populated. */ successfully -> fully ? > + RTE_MEMPOOL_EVENT_READY = 0, > + /** Occurs before destruction of a mempool begins. */ > + RTE_MEMPOOL_EVENT_DESTROY = 1, > +}; [snip]