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 E45DEA0C4C; Mon, 4 Oct 2021 12:37:27 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5E94D41318; Mon, 4 Oct 2021 12:37:27 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0a-0016f401.pphosted.com [67.231.148.174]) by mails.dpdk.org (Postfix) with ESMTP id 21FF041315 for ; Mon, 4 Oct 2021 12:37:24 +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 1943sX4X011679; Mon, 4 Oct 2021 03:37:21 -0700 Received: from nam11-dm6-obe.outbound.protection.outlook.com (mail-dm6nam11lp2169.outbound.protection.outlook.com [104.47.57.169]) by mx0a-0016f401.pphosted.com with ESMTP id 3bfqpts7pu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Mon, 04 Oct 2021 03:37:21 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Q6heivA1sxSGLroKQjTP9gDwLZcM225G0PUzHbbziZoDtwd/s35PXpZQUqQjbNwe8IAmZvQ26s8d77tOwokWLa8f6U8txGZhG6g/Kb7rAE/QsvoDC57z7zoxr/5/4VTQZG5+k1RVW+hoPqoO/GwSqd1pT+f/CgzrhObGGvX4yVmdn8jwEBO4GyndMm3XvLLLLxdmZFtmXdhdjIiPgCrkQGLYLJlu0OP91tn9W5usnzikGoVM+HhmKw2hO5maO2wGn/hVvvsdtzOmym1lZ1kVwjvi34E6OI6Hsoo4d94AMYJBHwHHdDIQWxdVhYxAlDGGzs+MhFBOKOwUiFR9CYQaGQ== 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=iSYpVSLU10sFOpTofl4Vm4IFSjX+3QFCQy91RD5Kwwo=; b=MC9kXsf8pGLlpA59bVh5Ipj0QsP5JifhTWin3xoXgA6E6xZN6oDJXNhBpqTXfO/KADCfsY3unwJvNgwlnUxY8gXL+NJr6eJLu26uC91jeU51W6wxXuX2px74R6OvUxCwAyHlVlZh+LxyCCWWTaxCYkEGoa0LjW4Ed7ndzKflF/FfDWSPzVaxEc4u0DPJRCZ5hYqtijvz6stT6QQP+MDWKjELnk9HR2aEHtWAzk+sP9ouKCJUP387Rg+8eVZkhC0rnngNevcuR6SywjpxzaB02oWc2sXDHWzxZT9xsGeKM934N5FGHlafQWyuAxfGksBEgGf9Va/FN3DcMH1FYPHduA== 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=iSYpVSLU10sFOpTofl4Vm4IFSjX+3QFCQy91RD5Kwwo=; b=UF3LhXad+TmFgG80N7oT8Cxo85kH/PTJgCoDBV+jDAvPkKaqHW9sd/Y4SA1a3D73aYYR9N730Sm0eFc/Bc+52x//P/GUSieBAoguEJDpz5CNEnHV+DBas5sGYLhShGLj2xVG6MsXnuzzB2P574dmPj8j5oECZDnQ9Pzk9AW6A2M= Received: from BN9PR18MB4204.namprd18.prod.outlook.com (2603:10b6:408:119::18) by BN6PR18MB1329.namprd18.prod.outlook.com (2603:10b6:404:12b::10) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4566.19; Mon, 4 Oct 2021 10:37:18 +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 10:37:18 +0000 From: Harman Kalra To: Dmitry Kozlyuk CC: "dev@dpdk.org" , Ray Kinsella , David Marchand Thread-Topic: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement get set APIs Thread-Index: AQHXoMEXRUmGnDOfBEuyfrELGqzD26vBwOCAgAEPZ9A= Date: Mon, 4 Oct 2021 10:37:18 +0000 Message-ID: References: <20210826145726.102081-1-hkalra@marvell.com> <20210903124102.47425-1-hkalra@marvell.com> <20210903124102.47425-3-hkalra@marvell.com> <20211003210505.1c852bd4@sovereign> In-Reply-To: <20211003210505.1c852bd4@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: beca6748-51a8-4b82-0f3e-08d98722f0ab x-ms-traffictypediagnostic: BN6PR18MB1329: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:113; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: QXPxeLLyH0QstYCwSgqaD2fro+aCS4gl522pwnF7W4kIPmmxHn4Q5iiT9pW2mUr3cgb1gd2w0zkWcx/8GIcb0fvvXpUa+tbV0m62Cb9dKxzRo9vXvn67EtJCom//8etEqsA4QNDX7sr5m+S6ZWuEsNO2WQmNYKfvgNnB7KkOdjBh5XNuEW9Q2/rbG6hNy+m7++7gCOxv8lIgPxfbGsvWRnl5rLe2VcOE0E2LL4kKWkWdZ50EhdbT2g3U19vacZwx0autdcGru7l3RtydPPjId8tj/xavzHMjzFUCIu0PZJFL2S3IrWEXTItBXkwlO4HcjI6HI/UvytRn0TzmdK7tJLQeub49ozv9CGvPwYeZXrcjB4tDyQA0ZIyDiE3YBharMCtTOaYTcBwl6v8EFg6iDgxC2zeXMFIndJVY0Kfk9QpNSxXMizFuU5duhFP8wmCqsgKVYq6dLupCtuxfOvOKU2UYdFosCJsxzbtlwwgUKaRI2nCMUuaIyFq2RmcOoh5bUxkGM5GKzWo7uiSZqNETPnBnq2InppD23SoyTXLhDIdwxXeIOC+Lj8Snn3E8PUOvnOUipNjS24xRVg+6OEYBB9yufmeJ9AQKqqW6+PQ1FDZxVxAW2Lc3UQx4gWvpHFFbh2ZB5kHD9moCjwtfA1PmyLI4NkcGszhmafGQqwK31RCbO+S5RAyhoOrw50mtUlzd7mjzzvw8nZZsB7kVKFBpIg== 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)(4326008)(186003)(26005)(83380400001)(66946007)(54906003)(316002)(6506007)(8936002)(38070700005)(66556008)(122000001)(86362001)(508600001)(53546011)(33656002)(71200400001)(66476007)(38100700002)(55016002)(76116006)(6916009)(2906002)(8676002)(52536014)(7696005)(5660300002)(66446008)(64756008)(9686003); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?j+e/KdWmf+j3rKYIjeRqx5zIEQjEvRr8v27OBWCPwd9gYgKD39m+mtF2q/eJ?= =?us-ascii?Q?zT2VOnGObPqR1MDs1lg7f9fFZDtLQAltm3q1dz05dbm3rkjyUyPtLCiiOe80?= =?us-ascii?Q?xPEYqmuF9Nf+MIKyTxPGH+qcDKxYZiJaxvOILZfT9zZtYCo3Zu0TW0WnSa6Q?= =?us-ascii?Q?TbYwqKvGJot+VG0cPx1w27Grny8PHtsOU3H4xBPELs5LN6dZYr2E72naLVJX?= =?us-ascii?Q?piAKOQvOgRaUMs0Habg7YjWU0JtnooXeV1gWsbGCI3FhQlWs8LKtuSH+Rg1S?= =?us-ascii?Q?3izxJoS9S8M1D+cakfnDPPfj2P7vAQnVszBA9dFMnjFwAHxqExVmZN/OuTAT?= =?us-ascii?Q?0ECHVrPT6HvBWPEkGOveyzaVKD3tGN75wFxZM2zDNIpG7oL0K5jDEw6vmh5+?= =?us-ascii?Q?usAwLBrabTS0DNVRtgepHzbbRw+A+Tr17GGhillA14EHnQOt7kAsYTDXrhLt?= =?us-ascii?Q?Q5bBQqA+mdW6IQ6Bplfie64yCbb31KmnG831OqrjgZKAYB4QQqYzfS9lwdIY?= =?us-ascii?Q?vrpOanp3DUzfKTnw8Mt+DTPk3MIzdCGoyOB1rPO/Kv2edze/MwEXVfF+Xmx/?= =?us-ascii?Q?FozYSYaBy0yKghYhVZq/v7TZOTcbmgdwChRsbdIup3+fzCu/7wPYindI+U4b?= =?us-ascii?Q?nyMKisnQRoqdfX7QzDnZyt7qHIh7a3YWLLWAQSneZJm2mviP70CLfgYnURlc?= =?us-ascii?Q?9o4wdGOQr++mOvnz7MFb75CA0XOsIo7bgYeRVGR/7/mVb9VsrJSPEi2rNXzk?= =?us-ascii?Q?ifHBJ4PdX1magbneJb5kuuSLclqrxie6uPdurUSkfg59XXa8/oyBcSzayhzK?= =?us-ascii?Q?Yx3H4w9bbkdVj61xeTcEENzar+lZenlJeT/XT+hpAyhDl7/aonJtZ0V+sJcd?= =?us-ascii?Q?E4om2hmCOg97GuW60APNsiwta3Nsc+2uBabZlrCfjJFCWFOG5ngT/m0z8IhY?= =?us-ascii?Q?15R1P4d6vKJ27YprPaFW4rrg5d+bTsiS1ApvMjrFA0EyYHYSr8nEaxdjATpV?= =?us-ascii?Q?mcPpZjWs/1b6CKDHxG76nM5LK2phDIkk9b/zF6aG6Fv1Oo3J3JPY0ufAiPVj?= =?us-ascii?Q?TFyHe76trIRSgOduV/pKazg4W/YZW3TfTWmKFmb+IsfTGVTsQdljSBio6uR8?= =?us-ascii?Q?9rGsmtsLz6E9VQl6qDTIQoz4xtkY/qWskQ+jbnmrj1j6VokfpIGY5mU4cOkm?= =?us-ascii?Q?SAvOSfEldbFbn+fBy3t52+CLtw4+CoJxn4Le2UNKXRoDfHONjFEF/PGYK4K/?= =?us-ascii?Q?e6WlJ6RRZlx6FSUfo3gef2g6lzQ+hBaQLdOznqEZjdO8q6SqclgRMbR92iuL?= =?us-ascii?Q?3e2aYrOry53KBhJmllpH5hnJ?= 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: beca6748-51a8-4b82-0f3e-08d98722f0ab X-MS-Exchange-CrossTenant-originalarrivaltime: 04 Oct 2021 10:37:18.5548 (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: BLEcRqxcTG4XGkuDhb8E+mXWY0GzSQiY5DXHDfQjdalfjMNugPsdQmDu/Hy/9IwqHyp128ua3hbhkxGarMIyTw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR18MB1329 X-Proofpoint-GUID: w3MsYurjFtTDl6uL9mWpJiZ79afmoaw1 X-Proofpoint-ORIG-GUID: w3MsYurjFtTDl6uL9mWpJiZ79afmoaw1 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_02,2021-10-04_01,2020-04-07_01 Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v1 2/7] eal/interrupts: implement get set APIs 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, Thanks for reviewing the series. Please find my comments inline. > -----Original Message----- > From: Dmitry Kozlyuk > Sent: Sunday, October 3, 2021 11:35 PM > To: Harman Kalra > Cc: dev@dpdk.org; Ray Kinsella > Subject: [EXT] Re: [dpdk-dev] [PATCH v1 2/7] eal/interrupts: implement ge= t > set APIs >=20 > External Email >=20 > ---------------------------------------------------------------------- > 2021-09-03 18:10 (UTC+0530), Harman Kalra: > > [...] > > diff --git a/lib/eal/common/eal_common_interrupts.c > > b/lib/eal/common/eal_common_interrupts.c > > new file mode 100644 > > index 0000000000..2e4fed96f0 > > --- /dev/null > > +++ b/lib/eal/common/eal_common_interrupts.c > > @@ -0,0 +1,506 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(C) 2021 Marvell. > > + */ > > + > > +#include > > +#include > > + > > +#include > > +#include > > +#include > > + > > +#include > > + > > + > > +struct rte_intr_handle *rte_intr_handle_instance_alloc(int size, > > + bool from_hugepage) >=20 > Since the purpose of the series is to reduce future ABI breakages, how ab= out > making the second parameter "flags" to have some spare bits? > (If not removing it completely per David's suggestion.) >=20 Having second parameter "flags" is a good suggestion, I will include i= t. > > +{ > > + struct rte_intr_handle *intr_handle; > > + int i; > > + > > + if (from_hugepage) > > + intr_handle =3D rte_zmalloc(NULL, > > + size * sizeof(struct rte_intr_handle), > > + 0); > > + else > > + intr_handle =3D calloc(1, size * sizeof(struct rte_intr_handle)); > > + if (!intr_handle) { > > + RTE_LOG(ERR, EAL, "Fail to allocate intr_handle\n"); > > + rte_errno =3D ENOMEM; > > + return NULL; > > + } > > + > > + for (i =3D 0; i < size; i++) { > > + intr_handle[i].nb_intr =3D RTE_MAX_RXTX_INTR_VEC_ID; > > + intr_handle[i].alloc_from_hugepage =3D from_hugepage; > > + } > > + > > + return intr_handle; > > +} > > + > > +struct rte_intr_handle *rte_intr_handle_instance_index_get( > > + struct rte_intr_handle *intr_handle, int > index) >=20 > If rte_intr_handle_instance_alloc() returns a pointer to an array, this f= unction > is useless since the user can simply manipulate a pointer. User wont be able to manipulate the pointer as he is not aware of size= of struct rte_intr_handle. He will observe "dereferencing pointer to incomplete type" compilation erro= r. > If we want to make a distinction between a single struct rte_intr_handle = and > a commonly allocated bunch of such (but why?), then they should be > represented by distinct types. Do you mean, we should have separate APIs for single allocation and ba= tch allocation? As get API will be useful only in case of batch allocation. Currently interrupt autote= sts and ifpga_rawdev driver makes batch allocation.=20 I think common API for single and batch is fine, get API is required for re= turning a particular intr_handle instance. But one problem I see in current implementation is there should be upper li= mit check for index in get/set API, which I will fix. >=20 > > +{ > > + if (intr_handle =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOMEM; >=20 > Why it's sometimes ENOMEM and sometimes ENOTSUP when the handle is > not allocated? I will fix and make it symmetrical across. >=20 > > + return NULL; > > + } > > + > > + return &intr_handle[index]; > > +} > > + > > +int rte_intr_handle_instance_index_set(struct rte_intr_handle > *intr_handle, > > + const struct rte_intr_handle *src, > > + int index) >=20 > See above regarding the "index" parameter. If it can be removed, a better > name for this function would be rte_intr_handle_copy(). I think get API is required. >=20 > > +{ > > + if (intr_handle =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOTSUP; > > + goto fail; > > + } > > + > > + if (src =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Source interrupt instance > unallocated\n"); > > + rte_errno =3D EINVAL; > > + goto fail; > > + } > > + > > + if (index < 0) { > > + RTE_LOG(ERR, EAL, "Index cany be negative"); > > + rte_errno =3D EINVAL; > > + goto fail; > > + } >=20 > How about making this parameter "size_t"? You mean index ? It can be size_t. >=20 > > + > > + intr_handle[index].fd =3D src->fd; > > + intr_handle[index].vfio_dev_fd =3D src->vfio_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; > > + > > + memcpy(intr_handle[index].efds, src->efds, src->nb_intr); > > + memcpy(intr_handle[index].elist, src->elist, src->nb_intr); > > + > > + return 0; > > +fail: > > + return rte_errno; >=20 > Should be (-rte_errno) per documentation. > Please check all functions in this file that return an "int" status. Sure will fix it across APIs. >=20 > > [...] > > +int rte_intr_handle_dev_fd_get(const struct rte_intr_handle > > +*intr_handle) { > > + if (intr_handle =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOTSUP; > > + goto fail; > > + } > > + > > + return intr_handle->vfio_dev_fd; > > +fail: > > + return rte_errno; > > +} >=20 > Returning a errno value instead of an FD is very error-prone. > Probably returning (-1) is both safe and convenient? Ack >=20 > > + > > +int rte_intr_handle_max_intr_set(struct rte_intr_handle *intr_handle, > > + int max_intr) > > +{ > > + if (intr_handle =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOTSUP; > > + goto fail; > > + } > > + > > + if (max_intr > intr_handle->nb_intr) { > > + RTE_LOG(ERR, EAL, "Max_intr=3D%d greater than > > +PLT_MAX_RXTX_INTR_VEC_ID=3D%d", >=20 > Seems like this common/cnxk name leaked here by mistake? Thanks for catching this. >=20 > > + max_intr, intr_handle->nb_intr); > > + rte_errno =3D ERANGE; > > + goto fail; > > + } > > + > > + intr_handle->max_intr =3D max_intr; > > + > > + return 0; > > +fail: > > + return rte_errno; > > +} > > + > > +int rte_intr_handle_max_intr_get(const struct rte_intr_handle > > +*intr_handle) { > > + if (intr_handle =3D=3D NULL) { > > + RTE_LOG(ERR, EAL, "Interrupt instance unallocated\n"); > > + rte_errno =3D ENOTSUP; > > + goto fail; > > + } > > + > > + return intr_handle->max_intr; > > +fail: > > + return rte_errno; > > +} >=20 > Should be negative per documentation and to avoid returning a positive > value that cannot be distinguished from a successful return. > Please also check other functions in this file returning an "int" result = (not > status). Will fix it.