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 F0E35A034F; Sun, 3 Oct 2021 20:16:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8666B41258; Sun, 3 Oct 2021 20:16:23 +0200 (CEST) Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by mails.dpdk.org (Postfix) with ESMTP id 9EC4C41251 for ; Sun, 3 Oct 2021 20:16:22 +0200 (CEST) Received: by mail-lf1-f41.google.com with SMTP id u18so62186567lfd.12 for ; Sun, 03 Oct 2021 11:16:22 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=k2GpWjVOMyxM5RSYdlH7JUgleIoTzEq1ykZSJZpVuz0=; b=h8RQYmSBPS6zrGmGM8vOKoBJqpMSmLPUo3ATBEbqQMyiDIIcrkGl5b/lbHpD2CP2M0 1XZZ9j5ObASGncgf/Jx+PgIHdF2H2DzNS8xYHFBb4ZCAPmU84U5WEdW15QTjNoqRiLBA cgqTdvwrRfZbkYx+fj0KGw76etlg4Dw+u0cUMQoIhYiJMYCOkBthzLTYYK2wT8Qiqrlk ZUWdprXK3g7m7G1/iTFyO0t41KgirxEhuEmQqlcNOXcGWNVTFTjSMif9M/Rmw8RYC+4y DwIY+Mu61ZoJzP/pLAou/p+jgeM0zgmT8mmEqx3GpOGoUCVn7azkUL4yIbk1LMdBXYOx nXCQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=k2GpWjVOMyxM5RSYdlH7JUgleIoTzEq1ykZSJZpVuz0=; b=nKCEx/VrvjW6VN7JZJhAP6zatCFKczzsAvq1bztDxKWy3Fdt2IZ779g9RYRNiDbuaX 0QbrMHtcL3bH+T2up+mewcvidXQGTkH5l5/SJ4v+i3ETJGcFSdyQGqc05CM1IaCqkIDr Pv3LD3/yU0VhSpS7kKz1qWwRuhtmo3HE3kNZrCUdyd9fzj29L4hUBNcLXGG7pFVKQHNC olLUJZlstFTgvGV7E2V+AvgYN+DQt57Bf9p3jPTi4n6qH8HVgEL1ffv//oYnOGaIq4Vj PHEdVmTtECd6h8KC30+KdPUDG1YbJORptSfw7uoq6R7+SwpRtkOcABqCcFaLXKZ5zHmo Wfhw== X-Gm-Message-State: AOAM532cUFAxe6adNrzUC2rnsqXpf8cETxiRaTM8nFeLVh1nTnJ4gVSj Rz+HZo3/jMAOrsn0dHNYLPk= X-Google-Smtp-Source: ABdhPJz9NRsZmjMk+sq0OwLAQuT4DM8buSzqowvID/cdxacEvRPN3ipj0oKnnN/iTouPfTUuU2a0bg== X-Received: by 2002:a2e:9a98:: with SMTP id p24mr10956520lji.55.1633284982113; Sun, 03 Oct 2021 11:16:22 -0700 (PDT) Received: from sovereign (broadband-37-110-65-23.ip.moscow.rt.ru. [37.110.65.23]) by smtp.gmail.com with ESMTPSA id c21sm1386710lfi.192.2021.10.03.11.16.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 03 Oct 2021 11:16:20 -0700 (PDT) Date: Sun, 3 Oct 2021 21:16:19 +0300 From: Dmitry Kozlyuk To: Harman Kalra Cc: , Anatoly Burakov Message-ID: <20211003211619.47a5834e@sovereign> In-Reply-To: <20210903124102.47425-7-hkalra@marvell.com> References: <20210826145726.102081-1-hkalra@marvell.com> <20210903124102.47425-1-hkalra@marvell.com> <20210903124102.47425-7-hkalra@marvell.com> X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v1 6/7] eal/interrupts: make interrupt handle structure opaque 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" 2021-09-03 18:11 (UTC+0530), Harman Kalra: > [...] > @@ -31,11 +54,40 @@ struct rte_intr_handle *rte_intr_handle_instance_alloc(int size, > } > > for (i = 0; i < size; i++) { > + if (from_hugepage) > + intr_handle[i].efds = rte_zmalloc(NULL, > + RTE_MAX_RXTX_INTR_VEC_ID * sizeof(uint32_t), 0); > + else > + intr_handle[i].efds = calloc(1, > + RTE_MAX_RXTX_INTR_VEC_ID * sizeof(uint32_t)); > + if (!intr_handle[i].efds) { > + RTE_LOG(ERR, EAL, "Fail to allocate event fd list\n"); > + rte_errno = ENOMEM; > + goto fail; > + } > + > + if (from_hugepage) > + intr_handle[i].elist = rte_zmalloc(NULL, > + RTE_MAX_RXTX_INTR_VEC_ID * > + sizeof(struct rte_epoll_event), 0); > + else > + intr_handle[i].elist = calloc(1, > + RTE_MAX_RXTX_INTR_VEC_ID * > + sizeof(struct rte_epoll_event)); > + if (!intr_handle[i].elist) { > + RTE_LOG(ERR, EAL, "fail to allocate event fd list\n"); > + rte_errno = ENOMEM; > + goto fail; > + } > intr_handle[i].nb_intr = RTE_MAX_RXTX_INTR_VEC_ID; > intr_handle[i].alloc_from_hugepage = from_hugepage; > } > > return intr_handle; > +fail: > + free(intr_handle->efds); > + free(intr_handle); > + return NULL; This is incorrect if "from_hugepage" is set. > } > > struct rte_intr_handle *rte_intr_handle_instance_index_get( > @@ -73,12 +125,48 @@ int rte_intr_handle_instance_index_set(struct rte_intr_handle *intr_handle, > } > > intr_handle[index].fd = src->fd; > - intr_handle[index].vfio_dev_fd = src->vfio_dev_fd; > + intr_handle[index].dev_fd = src->dev_fd; > + > intr_handle[index].type = src->type; > intr_handle[index].max_intr = src->max_intr; > intr_handle[index].nb_efd = src->nb_efd; > intr_handle[index].efd_counter_size = src->efd_counter_size; > > + if (intr_handle[index].nb_intr != src->nb_intr) { > + if (src->alloc_from_hugepage) > + intr_handle[index].efds = > + rte_realloc(intr_handle[index].efds, > + src->nb_intr * > + sizeof(uint32_t), 0); > + else > + intr_handle[index].efds = > + realloc(intr_handle[index].efds, > + src->nb_intr * sizeof(uint32_t)); > + if (intr_handle[index].efds == NULL) { > + RTE_LOG(ERR, EAL, "Failed to realloc the efds list"); > + rte_errno = ENOMEM; > + goto fail; > + } > + > + if (src->alloc_from_hugepage) > + intr_handle[index].elist = > + rte_realloc(intr_handle[index].elist, > + src->nb_intr * > + sizeof(struct rte_epoll_event), 0); > + else > + intr_handle[index].elist = > + realloc(intr_handle[index].elist, > + src->nb_intr * > + sizeof(struct rte_epoll_event)); > + if (intr_handle[index].elist == NULL) { > + RTE_LOG(ERR, EAL, "Failed to realloc the event list"); > + rte_errno = ENOMEM; > + goto fail; > + } > + > + intr_handle[index].nb_intr = src->nb_intr; > + } > + This implementation leaves "intr_handle" in an invalid state and leaks memory on error paths. > memcpy(intr_handle[index].efds, src->efds, src->nb_intr); > memcpy(intr_handle[index].elist, src->elist, src->nb_intr); > > @@ -87,6 +175,45 @@ int rte_intr_handle_instance_index_set(struct rte_intr_handle *intr_handle, > return rte_errno; > } > > +int rte_intr_handle_event_list_update(struct rte_intr_handle *intr_handle, > + int size) > +{ > + if (intr_handle == NULL) { > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > + rte_errno = ENOTSUP; > + goto fail; > + } > + > + if (size == 0) { > + RTE_LOG(ERR, EAL, "Size can't be zero\n"); > + rte_errno = EINVAL; > + goto fail; > + } > + > + intr_handle->efds = realloc(intr_handle->efds, > + size * sizeof(uint32_t)); > + if (intr_handle->efds == NULL) { > + RTE_LOG(ERR, EAL, "Failed to realloc the efds list"); > + rte_errno = ENOMEM; > + goto fail; > + } > + > + intr_handle->elist = realloc(intr_handle->elist, > + size * sizeof(struct rte_epoll_event)); > + if (intr_handle->elist == NULL) { > + RTE_LOG(ERR, EAL, "Failed to realloc the event list"); > + rte_errno = ENOMEM; > + goto fail; > + } > + > + intr_handle->nb_intr = size; > + > + return 0; > +fail: > + return rte_errno; > +} > + > + Same here. > [...] > diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h > index afc3262967..7dfb849eea 100644 > --- a/lib/eal/include/rte_interrupts.h > +++ b/lib/eal/include/rte_interrupts.h > @@ -25,9 +25,29 @@ extern "C" { > /** Interrupt handle */ > struct rte_intr_handle; > > -#define RTE_INTR_HANDLE_DEFAULT_SIZE 1 > +#define RTE_MAX_RXTX_INTR_VEC_ID 512 > +#define RTE_INTR_VEC_ZERO_OFFSET 0 > +#define RTE_INTR_VEC_RXTX_OFFSET 1 > + > +/** > + * The interrupt source type, e.g. UIO, VFIO, ALARM etc. > + */ > +enum rte_intr_handle_type { > + RTE_INTR_HANDLE_UNKNOWN = 0, /**< generic unknown handle */ > + RTE_INTR_HANDLE_UIO, /**< uio device handle */ > + RTE_INTR_HANDLE_UIO_INTX, /**< uio generic handle */ > + RTE_INTR_HANDLE_VFIO_LEGACY, /**< vfio device handle (legacy) */ > + RTE_INTR_HANDLE_VFIO_MSI, /**< vfio device handle (MSI) */ > + RTE_INTR_HANDLE_VFIO_MSIX, /**< vfio device handle (MSIX) */ > + RTE_INTR_HANDLE_ALARM, /**< alarm handle */ > + RTE_INTR_HANDLE_EXT, /**< external handler */ > + RTE_INTR_HANDLE_VDEV, /**< virtual device */ > + RTE_INTR_HANDLE_DEV_EVENT, /**< device event handle */ > + RTE_INTR_HANDLE_VFIO_REQ, /**< VFIO request handle */ > + RTE_INTR_HANDLE_MAX /**< count of elements */ Enums shouldn't have a _MAX member, can we remove it? > +}; > > -#include "rte_eal_interrupts.h" > +#define RTE_INTR_HANDLE_DEFAULT_SIZE 1 I find this constant more cluttering call sites than helpful. If a handle is allocated with a calloc-like function, plain 1 reads just fine.