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 4D574A0A0A; Thu, 3 Jun 2021 09:26:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D2D0F40689; Thu, 3 Jun 2021 09:26:08 +0200 (CEST) Received: from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com [66.111.4.26]) by mails.dpdk.org (Postfix) with ESMTP id A27574067A for ; Thu, 3 Jun 2021 09:26:06 +0200 (CEST) Received: from compute3.internal (compute3.nyi.internal [10.202.2.43]) by mailout.nyi.internal (Postfix) with ESMTP id 249365C0117; Thu, 3 Jun 2021 03:26:06 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute3.internal (MEProxy); Thu, 03 Jun 2021 03:26:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm1; bh= O1eNvbwdeiQuyeVkFM2cg7RHG+LBiCxTNIo+itJAm8M=; b=NaVIQdxLcSR+MJQR +TpnTlYxVadStArsLDoAQ7HbMHITqcwsjMCETeE0K93diw72jMJP5Xc+fE/IEqbX CxIIWwlD4KX8fJ2sZhE4w7c0Kk80rqKahvXkiDLjZsmzC9/kpRxW22//pFeDnmMv K7Gg2Iwt4bC5SVT6gLKpQRXM1ew81WcQ8H0ElqE0VERx8KPyNSYiHvP3pG4pgfnO 0lcRBZv8IsHOYsYltY2rKsPMwgnn+8oZl9ejfwuePeCu3C51Vn5XVErIx+PJgpzk RBW64qTe5g0mwimrcCMx5+5WOLv+3Q6Br5bXfy/0W8nnDKpRPVS5e0cplVSL+cu2 umjS3Q== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=O1eNvbwdeiQuyeVkFM2cg7RHG+LBiCxTNIo+itJAm 8M=; b=iG0gMNOpVgWUnJaSYi1uIFVmIzkPyaJERgYI1WZ0i/yBJqJhe1+6fza8a K51/K/u6rQYw/7ARUhobimGGD5ndIr8KUO5qsuhuUmiAOtw6W8HANXlCwVdXMeoP CcecxnMcI63nVZtye4nrDT1fDS6qOpTyWfWpCb85NxA7xFanFQ1EGT/5ks7uEGe3 AlFzvB0R7REZ60FR7MRMCaRA5FGaIFjcbqPk1SarHaKduxxoJDILyUaoZ7Pcor9A 9Dq9Epj5RB0qaXzvkuvLJGLs8Z9iyPNE1yZ6+Ayg5lfdXEBmAHQ88BQ6BFFXdMpq 9aR8ADDwfURJkAzSgG949Yd9o0/Sw== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduledrvdelkedguddukecutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefhvffufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedugefgvdefudfftdefgeelgffhueekgfffhfeujedtteeutdej ueeiiedvffegheenucevlhhushhtvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfh hrohhmpehthhhomhgrshesmhhonhhjrghlohhnrdhnvght X-ME-Proxy: Received: by mail.messagingengine.com (Postfix) with ESMTPA; Thu, 3 Jun 2021 03:26:04 -0400 (EDT) From: Thomas Monjalon To: Andrew Rybchenko Cc: dev@dpdk.org, Elena Agostini , david.marchand@redhat.com Date: Thu, 03 Jun 2021 09:26:02 +0200 Message-ID: <2561723.R1Na977jkj@thomas> In-Reply-To: <38e4c145-e561-0707-a3b1-17a8f6cee75e@oktetlabs.ru> References: <20210602203531.2288645-1-thomas@monjalon.net> <38e4c145-e561-0707-a3b1-17a8f6cee75e@oktetlabs.ru> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH] gpudev: introduce memory API 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" 03/06/2021 09:06, Andrew Rybchenko: > On 6/2/21 11:35 PM, Thomas Monjalon wrote: > > From: Elena Agostini > > > > The new library gpudev is for dealing with GPU from a DPDK application > > in a vendor-agnostic way. > > > > As a first step, the features are focused on memory management. > > A function allows to allocate memory inside the GPU, > > while another one allows to use main (CPU) memory from the GPU. > > > > The infrastructure is prepared to welcome drivers in drivers/gpu/ > > as the upcoming NVIDIA one, implementing the gpudev API. > > Other additions planned for next revisions: > > - C implementation file > > - guide documentation > > - unit tests > > - integration in testpmd to enable Rx/Tx to/from GPU memory. > > > > The next step should focus on GPU processing task control. > > > > Signed-off-by: Elena Agostini > > Signed-off-by: Thomas Monjalon > > > LGTM as an RFC. It is definitely to a patch to apply > since implementation is missing. See my notes below. Yes sorry I forgot the RFC tag when sending. [...] > > +typedef int (*gpu_malloc_t)(struct rte_gpu_dev *dev, size_t size, void **ptr); > > +typedef int (*gpu_free_t)(struct rte_gpu_dev *dev, void *ptr); > > Not that important but I always prefer to typedef > function prototypes w/o pointer and use pointer in > the structure below. I.e. > > typedef int (gpu_malloc_t)(struct rte_gpu_dev *dev, size_t size, void > **ptr); > > It allows to specify that corresponding callback > must comply to the prototype and produce build > error otherwise (and do not rely on warnings), e.g. > > static gpu_malloc_t mlx5_gpu_malloc; > static int > mlx5_gpu_malloc(struct rte_gpu_dev *dev, size_t size, void **ptr) > { > ... > } > > May be a new library should go this way. I agree. > > > + > > +struct rte_gpu_dev { > > + /* Backing device. */ > > + struct rte_device *device; > > + /* GPU info structure. */ > > + struct rte_gpu_info info; > > + /* Counter of processes using the device. */ > > + uint16_t process_cnt; > > + /* If device is currently used or not. */ > > + enum rte_gpu_state state; > > + /* FUNCTION: Allocate memory on the GPU. */ > > + gpu_malloc_t gpu_malloc; > > + /* FUNCTION: Allocate memory on the CPU visible from the GPU. */ > > + gpu_malloc_t gpu_malloc_visible; > > + /* FUNCTION: Free allocated memory on the GPU. */ > > + gpu_free_t gpu_free; > > Don't we need a callback to get dev_info? Yes it's my miss. [...] > > +__rte_experimental > > +int rte_gpu_dev_info_get(uint16_t gpu_id, struct rte_gpu_info **info); > > Hm, I think it is better to have 'struct rte_gpu_info *info'. > Why should it allocate and return memory to be freed by caller? No you're right, I overlooked it. [...] > > + * Allocate a chunk of memory on the GPU. > > Looking a below function it is required to clarify here if > the memory is visible or invisible to GPU (or both allowed). This function allocates on the GPU so it is visible by the GPU. I feel I misunderstand your question. > > + * > > + * @param gpu_id > > + * GPU ID to allocate memory. > > + * @param size > > + * Number of bytes to allocate. > > Is behaviour defined if zero size is requested? > IMHO, it would be good to define. OK > > + * @param ptr > > + * Pointer to store the address of the allocated memory. > > + * > > + * @return > > + * 0 on success, -1 otherwise. > > Don't we want to differentiate various errors using > negative errno as it is done in many DPDK libraries? Yes I think so, I was just too much lazy to do it in this RFC. > > + */ > > +__rte_experimental > > +int rte_gpu_malloc(uint16_t gpu_id, size_t size, void **ptr); > > May be *malloc() should return a pointer and "negative" > values used to report various errnos? I don't understand what you mean by negative values if it is a pointer. We could return a pointer and use rte_errno. > The problem with the approach that comparison vs NULL will > not work in this case and we need special macro or small > inline function to check error condition. > > Returned pointer is definitely more convenient, but above > not may result in bugs. I don't know what is better. [...] > > + * Deallocate a chunk of memory allocated with rte_gpu_malloc*. > > + * > > + * @param gpu_id > > + * Reference GPU ID. > > + * @param ptr > > + * Pointer to the memory area to be deallocated. > > I think it should be NOP in the case of NULL pointer and it > should be documented. If not, it must be documented as well. OK for NOP.