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 84BDC45B40; Tue, 15 Oct 2024 08:41:41 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 4D9514027C; Tue, 15 Oct 2024 08:41:41 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id DC96440270 for ; Tue, 15 Oct 2024 08:41:38 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id A292112ED1 for ; Tue, 15 Oct 2024 08:41:38 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 9520112ED0; Tue, 15 Oct 2024 08:41:38 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 4.0.0 (2022-12-13) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.2 required=5.0 tests=ALL_TRUSTED,AWL, T_SCC_BODY_TEXT_LINE autolearn=disabled version=4.0.0 X-Spam-Score: -1.2 Received: from [192.168.1.85] (h-62-63-215-114.A163.priv.bahnhof.se [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id CF15A12F2C; Tue, 15 Oct 2024 08:41:36 +0200 (CEST) Message-ID: Date: Tue, 15 Oct 2024 08:41:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v11 1/7] eal: add static per-lcore memory allocation facility To: =?UTF-8?Q?Morten_Br=C3=B8rup?= , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , dev@dpdk.org, thomas@monjalon.net Cc: Stephen Hemminger , Konstantin Ananyev , David Marchand , Jerin Jacob , Luka Jankovic , Konstantin Ananyev , Chengwen Feng References: <20241011081901.816211-2-mattias.ronnblom@ericsson.com> <20241014074348.821962-1-mattias.ronnblom@ericsson.com> <20241014074348.821962-2-mattias.ronnblom@ericsson.com> <98CBD80474FA8B44BF855DF32C47DC35E9F7D1@smartserver.smartshare.dk> Content-Language: en-US From: =?UTF-8?Q?Mattias_R=C3=B6nnblom?= In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9F7D1@smartserver.smartshare.dk> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 On 2024-10-14 10:17, Morten Brørup wrote: >> From: Mattias Rönnblom [mailto:mattias.ronnblom@ericsson.com] >> Sent: Monday, 14 October 2024 09.44 > > >> +struct lcore_var_buffer { >> + char data[RTE_MAX_LCORE_VAR * RTE_MAX_LCORE]; >> + struct lcore_var_buffer *prev; >> +}; > > In relation to Jerin's request for using hugepages when available, the "data" field should be a pointer to the memory allocated from either the heap or through rte_malloc. You would also need to add a flag to indicate which it is, so the correct deallocation function can be used to free it on cleanup. > The typing (glibc heap or DPDK heap) could be in the buffers themselves, no? > > Here's another (nice to have) idea, which does not need to be part of this series, but can be implemented in a separate patch: > If you move "offset" into this structure, new lcore variables can be allocated from any buffer, instead of only the most recently allocated buffer. > There might even be gains by picking the "optimal" buffer to allocate different size variables from. > > If the max lcore variable size is much greater than the actual variable sizes, the amount of fragmentation (i.e., the space at the end) will be very small. I don't think we should use huge pages for this facility, since they don't support demand paging. The day we have a DPDK heap which support lcore-affinitized allocations, then potentially eal_common_lcore_var.c could use that, provided it's available (and there is a proper way to check [or get notified] if it is available or not). >> + >> +static struct lcore_var_buffer *current_buffer; >> + >> +/* initialized to trigger buffer allocation on first allocation */ >> +static size_t offset = RTE_MAX_LCORE_VAR; > > >> +void * >> +rte_lcore_var_alloc(size_t size, size_t align) >> +{ >> + /* Having the per-lcore buffer size aligned on cache lines >> + * assures as well as having the base pointer aligned on cache >> + * size assures that aligned offsets also translate to alipgned >> + * pointers across all values. >> + */ >> + RTE_BUILD_BUG_ON(RTE_MAX_LCORE_VAR % RTE_CACHE_LINE_SIZE != 0); >> + RTE_ASSERT(align <= RTE_CACHE_LINE_SIZE); >> + RTE_ASSERT(size <= RTE_MAX_LCORE_VAR); > > This is very slow path, please RTE_VERIFY instead of RTE_ASSERT in this function. > Sure. (I think I rejected that before, but now I don't agree with my old self.) > >> +/** >> + * Get pointer to lcore variable instance with the specified lcore id. >> + * >> + * @param lcore_id >> + * The lcore id specifying which of the @c RTE_MAX_LCORE value >> + * instances should be accessed. The lcore id need not be valid >> + * (e.g., may be @ref LCORE_ID_ANY), but in such a case, the pointer >> + * is also not valid (and thus should not be dereferenced). >> + * @param handle >> + * The lcore variable handle. >> + */ >> +#define RTE_LCORE_VAR_LCORE_VALUE(lcore_id, handle) \ >> + ((typeof(handle))rte_lcore_var_lcore_ptr(lcore_id, handle)) > > Please remove the _VALUE suffix. > You changed your mind? I'm missing the rationale here. >> + >> +/** >> + * Get pointer to lcore variable instance of the current thread. >> + * >> + * May only be used by EAL threads and registered non-EAL threads. >> + */ >> +#define RTE_LCORE_VAR_VALUE(handle) \ >> + RTE_LCORE_VAR_LCORE_VALUE(rte_lcore_id(), handle) > > Please remove the _VALUE suffix. > >> + >> +/** >> + * Iterate over each lcore id's value for an lcore variable. >> + * >> + * @param lcore_id >> + * An unsigned int variable successively set to the >> + * lcore id of every valid lcore id (up to @c RTE_MAX_LCORE). >> + * @param value >> + * A pointer variable successively set to point to lcore variable >> + * value instance of the current lcore id being processed. >> + * @param handle >> + * The lcore variable handle. >> + */ >> +#define RTE_LCORE_VAR_FOREACH_VALUE(lcore_id, value, handle) > > Please remove the _VALUE suffix. > >> \ >> + for ((lcore_id) = \ >> + (((value) = RTE_LCORE_VAR_LCORE_VALUE(0, handle)), 0); >> \ >> + (lcore_id) < RTE_MAX_LCORE; \ >> + (lcore_id)++, (value) = RTE_LCORE_VAR_LCORE_VALUE(lcore_id, >> \ >