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 11266A0C4D; Mon, 4 Oct 2021 16:09:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 8B5984130B; Mon, 4 Oct 2021 16:09:21 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by mails.dpdk.org (Postfix) with ESMTP id 675AB412D2 for ; Mon, 4 Oct 2021 16:09:19 +0200 (CEST) Received: from pps.filterd (m0045849.ppops.net [127.0.0.1]) by mx0a-0016f401.pphosted.com (8.16.1.2/8.16.1.2) with SMTP id 194DkskM032685; Mon, 4 Oct 2021 07:09:18 -0700 Received: from nam04-dm6-obe.outbound.protection.outlook.com (mail-dm6nam08lp2049.outbound.protection.outlook.com [104.47.73.49]) by mx0a-0016f401.pphosted.com with ESMTP id 3bfqptswbv-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 04 Oct 2021 07:09:18 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=f7ioaSEVCqjj0Q17tn7Z5oPTF795V/3/ueaL6EOlS9dRcTRbRjbQhDPeiZP0gvj/jBc8lnEtqF1ji2g9N1Is2RkJgYIbfVi50kZfiTYT8rzYuZczWICODaIj3t/VyE+EDa+xj5Uos8FG0BCOQUL574RN+IP3oWbbWGBI2ohrzMT/rZgdtjluebSSldaJM32+SXCtuZk8HNDx6xkKg4WJ1V9KJPK6l9r1T414e7PZJIt42yY1f5ANcRvprnteEYUBvecrIjpx/eiOAeRwQpm3c/ur3/pnetS5Ke41u4wr8r/3DlzvEbZ8W8RpgHLiwapucyMbvZfCHvX3QlOlK8jBjQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Q3V6mfgXaA0/BBszLOvkN9eEiVGvshYSATDuLBpKTq4=; b=DmY7A/xCz+GUvoLSi1hvcy566xOVTpy61aY32JHeMSMdwgTXso+YRLbRo5br5MTJYQkhtqfZjeWT2LlsX5s+pfxDERfNDykVx2vd+ZTWJlcho+ZbhVWfP0J3+CML1hOTdFogM1fEIx/uUocZV/hgh+6Th1ZcAtSjr3YuP1cB/KTDMvVYRDAaLyZRfaCn15UX0SIoOUWRkwVF2VnFyBU2XVrZPrV39zZIgTKw1eCJ/TZ2CXfO2KJLHmzrcKIPWvn1io50U4aYaefrQcVq170dyUkiDcNEVmcBYPulma3N8gMhyle80CTivA7H9pRSwWmSksJdJ7Rojiujp3GQcSjphQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=marvell.com; dmarc=pass action=none header.from=marvell.com; dkim=pass header.d=marvell.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.onmicrosoft.com; s=selector1-marvell-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Q3V6mfgXaA0/BBszLOvkN9eEiVGvshYSATDuLBpKTq4=; b=kgHOYUuCTzkfHHdxtVsusgdIbh21BtOhN5lOtA/NZzO3sAV9k/NIc2U+R8hy0msNczp/0iZ9Ncj9RAcsfEcCqAbNFG6U/1FPn3Pn7EWg7rw7WhpLai8L3Hz9BiMAtYpH/G8O59RjcItHroOrqJ2YaaBe32x0orYEzGGEK0Bw8XM= Received: from BN9PR18MB4204.namprd18.prod.outlook.com (2603:10b6:408:119::18) by BN6PR18MB1537.namprd18.prod.outlook.com (2603:10b6:404:12d::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4566.14; Mon, 4 Oct 2021 14:09:16 +0000 Received: from BN9PR18MB4204.namprd18.prod.outlook.com ([fe80::29f4:8e3d:264f:26b1]) by BN9PR18MB4204.namprd18.prod.outlook.com ([fe80::29f4:8e3d:264f:26b1%7]) with mapi id 15.20.4566.022; Mon, 4 Oct 2021 14:09:16 +0000 From: Harman Kalra To: Dmitry Kozlyuk CC: "dev@dpdk.org" , Anatoly Burakov Thread-Topic: [EXT] Re: [dpdk-dev] [PATCH v1 6/7] eal/interrupts: make interrupt handle structure opaque Thread-Index: AQHXoMEpkYHbeZq6FUyYPvFnP1S7/KvBxAOAgAETMLA= Date: Mon, 4 Oct 2021 14:09:15 +0000 Message-ID: References: <20210826145726.102081-1-hkalra@marvell.com> <20210903124102.47425-1-hkalra@marvell.com> <20210903124102.47425-7-hkalra@marvell.com> <20211003211619.47a5834e@sovereign> In-Reply-To: <20211003211619.47a5834e@sovereign> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 9d61bde9-6e38-46c4-898f-08d987408cea x-ms-traffictypediagnostic: BN6PR18MB1537: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:454; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: HTzu2qajjTKIENyXT9FGU+VyRkv60IiUiBNIvmtGvgBSgCx1vVe7WzmCYzR5sW+j6CZUy9ALntlakwV//kEpTEHz+52+SQH4DdPK3zs/+gD+lExV2K+DUdWNEmD6QHCsD1ALEFeoZNaJDxNT4d4N3Iyy/qorupmaq2dP6vMqr28qV44Nrlsg0vbZj3HqJlyBt1l3v90M//IL9lrefvhDbUnDkJUbd7B/3SjeRy2TW9ty5uNKFt48Y459MVrv7EEV+/sLQg8vnve6CdKM5UnpZ7soTjSsfY1RI3fecXddeNykN/LAKZ1qgkUTzzSR1+YelYUEKx2LtPa7WWnblQNCNPPp2I4DaAeqaL4TIbCHAsfVw/H7yRDirWGvVFjsotpcgTc7zFBYYlLU8ObFn+He/PQePg64cQsXQJyGHO4Qxgm0KQCuhDqDI409wHwfiImWzw3lsBu7iS9JyWPw1DSbjtngr198p5Tgf2hSyZLDDX6pCbdmiWrU/I2g7UzN3ixyFRNnQyVDe5WLxYk1jY3OmRNBC/rvYuv3rE10fWNDfWHgfmI4EJw2+5RtHa65J1V/CQFRq8CNmwND8g/M6saBNkrfbhlQ1JfpOAvJZrnvvaRrO9AadMMIQBKuxpt+sNj/riXPd7JypGpBv+RgXM28PMo83CtiDPSBr96Put7/Aj/N9V4ONdbAd76d/x2uxC0dsJiFbbHOB0pbEsrm2JN7ow== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:BN9PR18MB4204.namprd18.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(86362001)(33656002)(55016002)(4326008)(26005)(6916009)(5660300002)(71200400001)(66556008)(66946007)(76116006)(66476007)(66446008)(64756008)(186003)(53546011)(6506007)(508600001)(83380400001)(2906002)(9686003)(38070700005)(7696005)(122000001)(316002)(8676002)(54906003)(8936002)(52536014)(38100700002); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?efX1YNPdvDczy7yiONK+igUtC+OOnLzV38FFqgZho9TKUtSDlQ9Cxw/MmgMp?= =?us-ascii?Q?ieCqOjJNl+YXexdDLyr3VKskdS6F+Hjf8Q1ilqLXPIOl24EZKZnjUNf3OWDM?= =?us-ascii?Q?dwpV/A8A36yxUCkb5OotRdiHwN6wk4iAN29VncGwPN+SMP1BRGqF+06aorAj?= =?us-ascii?Q?LLAeUUvWQHXwzuEx+pUyS3Oq2SxMDPr15x0tfHUYe4rbR5cdfcP6XzOMHP0Z?= =?us-ascii?Q?ShvdDhzg9x4f7XOV38aXbvdTKNirA7YXOv/XVwM4YyhWGCvcrKc7i4Ah0Q1p?= =?us-ascii?Q?sL/wTtEjr2OVDC5GCU68BgT9f1SmONj5RW/rWMpHoNP2Mw8AyosLD3SjyuNG?= =?us-ascii?Q?Biutep+HHt7kc/f8XnO/wTc+Ch79uRfa0Xl6XljLspG+86E8bRZ9mTBODmC9?= =?us-ascii?Q?eYjx/MaOc98GF8Z99Kdk6XfPPCpgvCzLSu9umuGkBmoecYBNCuTsp5+6QxBX?= =?us-ascii?Q?A9HgL2GpWYeTL5XLVtaofXWpmdpLBbwF6BV488pHfRiXkiz/LBGGWUC3FkYE?= =?us-ascii?Q?tM2OOFQqxyR+Z1tqlvUG2CpAHlgu8ced4GTQfc3zbiZCTKl6HoTJnEWbzstt?= =?us-ascii?Q?+TB+m98fNQdnXCyOBN2Gsyu/NUmi8/zx1VPiPTVeYCD0Jtu/hdDoeixQVtfp?= =?us-ascii?Q?hAmmhccSiM0RUOF2f5fGMsmHTNsDcXX0MnwRgVhvjrQp71HYPobWid+fzS91?= =?us-ascii?Q?O+wsiMoDKxMII+KURvCoyFHapGsnRoYw79YjiMxDXMtWllG/Ds+HenGZZHT+?= =?us-ascii?Q?ftIB1mOdeE8nTPGDeJQJ8Zht4RRMxEK8ZV4JdLUFbouKLo4g2379R0jJ8I3V?= =?us-ascii?Q?a7zSm87W2mkooEeHISgKVt5MNZ4Gw7LsJ2A7FWQqimXGDA8k6bUjk3ojr8ZS?= =?us-ascii?Q?56Yh1djbXkzW8qGWkIVGEnnhflmiM9Ci9GqiQ3nAjludr783/R/KppPIbf6C?= =?us-ascii?Q?mQjfp7u+AG6ezqphTV+266hKNFNlX+YEY4POuo727ClyrHjlnohwolumsEKv?= =?us-ascii?Q?soB5qQZewBGQ2YWmG0hMzJHYMb43EjN2CpPdE7O0nWeow6YzPlhjBkk+62nv?= =?us-ascii?Q?l2ywDpsdDjbw0TMOjJf2VPqQohHbAldB7Vv2S+l6zDJOt57alPTxpj5FnTOY?= =?us-ascii?Q?40LPDYFDHYm37VxVV/n0exEGkM7Syu8llaWKFYh+ms3PSRrYLPR8t3jlDfFo?= =?us-ascii?Q?cEtSEIbulr2BZ4LyVrT5gPeBKTt1VDUi3V2RBYsA5woq4mSXI6y1iu+23ghQ?= =?us-ascii?Q?/7xT+hwmAni5k1J1nQXqULtv+OIguSZ3IB/N7BqwmhlRxZaSkfZMsBiGO1OJ?= =?us-ascii?Q?nXxcItktQ7j/Pkgvt9CLItez?= x-ms-exchange-transport-forked: True Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: marvell.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: BN9PR18MB4204.namprd18.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 9d61bde9-6e38-46c4-898f-08d987408cea X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Oct 2021 14:09:15.9820 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: ipjBoP4SxFhnr798DB0JXYtOCzgfsZr+lI9k1JdrG8hDQlMhlQpOQx67k7glc/zvWu2GbsAtFTgPqgvwwSdg0g== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR18MB1537 X-Proofpoint-GUID: KMKg2y80TU0i3kXTTBgNSU-ZenDsB4MU X-Proofpoint-ORIG-GUID: KMKg2y80TU0i3kXTTBgNSU-ZenDsB4MU X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.182.1,Aquarius:18.0.790,Hydra:6.0.391,FMLib:17.0.607.475 definitions=2021-10-04_04,2021-10-04_01,2020-04-07_01 Subject: Re: [dpdk-dev] [EXT] Re: [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" Hi Dmitry, Please find my comments inline. > -----Original Message----- > From: Dmitry Kozlyuk > Sent: Sunday, October 3, 2021 11:46 PM > To: Harman Kalra > Cc: dev@dpdk.org; Anatoly Burakov > Subject: [EXT] Re: [dpdk-dev] [PATCH v1 6/7] eal/interrupts: make interru= pt > handle structure opaque >=20 > External Email >=20 > ---------------------------------------------------------------------- > 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 =3D 0; i < size; i++) { > > + if (from_hugepage) > > + intr_handle[i].efds =3D rte_zmalloc(NULL, > > + RTE_MAX_RXTX_INTR_VEC_ID * > sizeof(uint32_t), 0); > > + else > > + intr_handle[i].efds =3D 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 =3D ENOMEM; > > + goto fail; > > + } > > + > > + if (from_hugepage) > > + intr_handle[i].elist =3D rte_zmalloc(NULL, > > + RTE_MAX_RXTX_INTR_VEC_ID * > > + sizeof(struct rte_epoll_event), 0); > > + else > > + intr_handle[i].elist =3D 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 =3D ENOMEM; > > + goto fail; > > + } > > intr_handle[i].nb_intr =3D RTE_MAX_RXTX_INTR_VEC_ID; > > intr_handle[i].alloc_from_hugepage =3D from_hugepage; > > } > > > > return intr_handle; > > +fail: > > + free(intr_handle->efds); > > + free(intr_handle); > > + return NULL; >=20 > This is incorrect if "from_hugepage" is set. Ack, will fix it. >=20 > > } > > > > 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 =3D src->fd; > > - intr_handle[index].vfio_dev_fd =3D src->vfio_dev_fd; > > + intr_handle[index].dev_fd =3D src->dev_fd; > > + > > intr_handle[index].type =3D src->type; > > intr_handle[index].max_intr =3D src->max_intr; > > intr_handle[index].nb_efd =3D src->nb_efd; > > intr_handle[index].efd_counter_size =3D src->efd_counter_size; > > > > + if (intr_handle[index].nb_intr !=3D src->nb_intr) { > > + if (src->alloc_from_hugepage) > > + intr_handle[index].efds =3D > > + rte_realloc(intr_handle[index].efds, > > + src->nb_intr * > > + sizeof(uint32_t), 0); > > + else > > + intr_handle[index].efds =3D > > + realloc(intr_handle[index].efds, > > + src->nb_intr * sizeof(uint32_t)); > > + if (intr_handle[index].efds =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Failed to realloc the efds list"); > > + rte_errno =3D ENOMEM; > > + goto fail; > > + } > > + > > + if (src->alloc_from_hugepage) > > + intr_handle[index].elist =3D > > + rte_realloc(intr_handle[index].elist, > > + src->nb_intr * > > + sizeof(struct rte_epoll_event), 0); > > + else > > + intr_handle[index].elist =3D > > + realloc(intr_handle[index].elist, > > + src->nb_intr * > > + sizeof(struct rte_epoll_event)); > > + if (intr_handle[index].elist =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Failed to realloc the event list"); > > + rte_errno =3D ENOMEM; > > + goto fail; > > + } > > + > > + intr_handle[index].nb_intr =3D src->nb_intr; > > + } > > + >=20 > This implementation leaves "intr_handle" in an invalid state and leaks > memory on error paths. Yes, I will get the reallocated pointer in a tmp variable and will upd= ate (intr_handle[index].elist/efds only after all error paths are cleared. >=20 > > 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 =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOTSUP; > > + goto fail; > > + } > > + > > + if (size =3D=3D 0) { > > + RTE_LOG(ERR, EAL, "Size can't be zero\n"); > > + rte_errno =3D EINVAL; > > + goto fail; > > + } > > + > > + intr_handle->efds =3D realloc(intr_handle->efds, > > + size * sizeof(uint32_t)); > > + if (intr_handle->efds =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Failed to realloc the efds list"); > > + rte_errno =3D ENOMEM; > > + goto fail; > > + } > > + > > + intr_handle->elist =3D realloc(intr_handle->elist, > > + size * sizeof(struct rte_epoll_event)); > > + if (intr_handle->elist =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Failed to realloc the event list"); > > + rte_errno =3D ENOMEM; > > + goto fail; > > + } > > + > > + intr_handle->nb_intr =3D size; > > + > > + return 0; > > +fail: > > + return rte_errno; > > +} > > + > > + >=20 > Same here. Ack >=20 > > [...] > > 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 =3D 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 */ >=20 > Enums shouldn't have a _MAX member, can we remove it? I don't see RTE_INTR_HANDLE_MAX used at any place, I will remove it. >=20 > > +}; > > > > -#include "rte_eal_interrupts.h" > > +#define RTE_INTR_HANDLE_DEFAULT_SIZE 1 >=20 > 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. Now since we are thinking of restricting rte_intr_handle_instance_alloc() t= o single intr_handle allocation, I will remove this macro. Thanks Harman