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 EAFFFA0C4E; Fri, 15 Oct 2021 15:07:47 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7A51E411CB; Fri, 15 Oct 2021 15:07:47 +0200 (CEST) Received: from AZHDRRW-EX02.NVIDIA.COM (azhdrrw-ex02.nvidia.com [20.64.145.131]) by mails.dpdk.org (Postfix) with ESMTP id 16F00410F1 for ; Fri, 15 Oct 2021 15:07:45 +0200 (CEST) Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.169) by mxs.oss.nvidia.com (10.13.234.37) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.858.15; Fri, 15 Oct 2021 06:07:43 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=SqSupCYaM2sq+5pPlp1zVy9aD1BEE398K/ZnCCaoGagm2f6frRq0PiwALPVLDV3yOgQHLD7pRMn0+m2VJSS/FcwZrQi+XijTg71plvUQGKu4kbGj6xInS4jyZpRB3ZYPSBaPDt/NY/eTe8QHsxUZu7mnep9tnixIb46aCZ//XPQapF0wVGrBRkdXCosqrc9ZtCEn8MxCCx8wRhYYL23I4zjVS2nXH5E2CibuZtcmoWhezac/ekrIdtB9N/HEDFZToVKxYH+LBuEqhdBwpN9zeW/FxXqIUomG+uCGaQYRMWpszdJH799RcTsX8LuJYp6Fwqxdb8qrpzGNOMzowGtjWw== 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=Rv9f7BYDkKZ6jhv3g2Jc08V4grg0dez8op7XR4EFabQ=; b=fFnYN/N7SucqV9Bl3uo2SDJXDITpEnGIqoLQszHUKQgt1HpR2y92YBifDz3P+Uhyc3W9Kz0XK5mI9AZ98P259soTbQ9SFv+n/hP13k6AZkt8jwGx0hGDV2NgAWkjfiJaStl2hYagvUqTPijN1ZYpx/KaiEm3UBE8ZyYc8HKOEjwqhT+/Z++H/PQc8AwYNpQFjczfBLuKNZYMqDOzHI/w7Tza5Nn+km2jT4MCT5U+NPXo1DCiVPW9Oolm7GTQ5fDS/prIrqEnTTFmKtyGa8d6ApLpOKvcOKWyXpzgt30TPu8tVr3OgLrRHCx9uTYSicFuLwzG1yyaNqJgjxHD8nCbUQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=nvidia.com; dmarc=pass action=none header.from=nvidia.com; dkim=pass header.d=nvidia.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Nvidia.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=Rv9f7BYDkKZ6jhv3g2Jc08V4grg0dez8op7XR4EFabQ=; b=omfz9SL8N+VG3HewwkMD5rsisQmhV3+jbf+MJH2F9JxNnBNN7o45XcA2pyI2kRoDOa0KO75/hjUqBy69nb7amNaeAfzJABaxJDq7hwsBx3g1LwM17ii57jiTXNt4T7sbs/MlDCJafWDhgO5OqmOcZnY2sqe6fzVrVYb5wwCuc/S68DWaM/HiSZqVrFhNO0reaYeP1lKweCRS9s4k3TB2q/NrQ7blm/aHd2/8jDp1hsbtW/TdrxmeYa5N8E4aEh2Q6AXXDAYAWmV8VXTHp7fBfrl7WHKQn9JcJ8bh6QormrlVWxO+1yLY7KauVuxpNxqWLv+jjGYM0qoEojOSOsh2uA== Received: from CH0PR12MB5091.namprd12.prod.outlook.com (2603:10b6:610:be::10) by CH0PR12MB5315.namprd12.prod.outlook.com (2603:10b6:610:d6::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4587.20; Fri, 15 Oct 2021 13:07:42 +0000 Received: from CH0PR12MB5091.namprd12.prod.outlook.com ([fe80::59bf:640f:7140:ab97]) by CH0PR12MB5091.namprd12.prod.outlook.com ([fe80::59bf:640f:7140:ab97%9]) with mapi id 15.20.4608.017; Fri, 15 Oct 2021 13:07:42 +0000 From: Dmitry Kozlyuk To: Olivier Matz CC: "dev@dpdk.org" , Andrew Rybchenko , Matan Azrad , Ray Kinsella , Anatoly Burakov Thread-Topic: [PATCH v4 1/4] mempool: add event callbacks Thread-Index: AQHXwb3h50GU9ZpxBEKK7yBeG2S6PqvT/0Xw Date: Fri, 15 Oct 2021 13:07:42 +0000 Message-ID: References: <20211012000409.2751908-1-dkozlyuk@nvidia.com> <20211013110131.2909604-1-dkozlyuk@nvidia.com> <20211013110131.2909604-2-dkozlyuk@nvidia.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: 6wind.com; dkim=none (message not signed) header.d=none;6wind.com; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1a133d19-8af4-4538-a3e9-08d98fdcc5db x-ms-traffictypediagnostic: CH0PR12MB5315: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: KmkSvHTk+qhskMwGmyei1dbMItGbEVe4q8c0bv4n/DGsrhr6WzQ8BLvpOPeIoZjNN7VWtrPmQsdRteU8JCAE3Bpup8heWH8fKy+l+YY8WLfei5ryf0JlN6Yj/SZ5zMMoQXAKFNxXxA0WyEGTuNIz/nLWuAJNL9dF8M3BKaVBRWukwIBgPr9fHXJm2BoWP92YrH06uVvBlNivyXGIYjUi9kcr28HCd7ewf6ENcSBhgHv/TQQZbYK4PNOU064mIUUaej3em/y79We5SQkSh869hguz61aYnJtF49zNk43lHpFr+b46x5G0zCV2N4if+O9tK/npEZkfZhExDqyLULn1vmNUYr7kgDvWHiIj2Mr8ZnhY0VSPJ9eeBc7m7u4u2+CXjfo2hLILqFaqrmkNXrjHDXTq7wXAOPWIFCxUOzf2r4sJuIh34un+vv71rOjnDmRZN+AdsZRQXpYp9IjWJdk4RaAO0ds5a8BGW4KCpoRo4+OPaxNocV2NSGTcU0Vt/19wntFuG6DEBW5wjpygDIP8bYJUqOUq7nOK68dMvx7lcazskmp+CnYZ7befCOrG0NpC2gbek/LjEcdsrGRCNm7rJ4Zr0KlKjauwbqP2unKw9GPHRISITNGNvswzBRAnGqEma+LKQoPG9yWw93faFWEGTnLz+CCFlLctZMAecygMr5DLKF5pq7TeABAx6JbGnR4jOQ+l4pV6e7jaApJttzuuBbUS64fru2mlmios9IzA7d7HVPVtsUa9ELZWR4sT0lDmpHLTtEsdLHCuf+vgsbr6mFEuFoJgtRmNyRfcG4QfOapi4CzeRFHFo3vKejTfspA3 x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:CH0PR12MB5091.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(122000001)(38100700002)(6862004)(2906002)(86362001)(7696005)(186003)(33656002)(8936002)(8676002)(26005)(107886003)(4326008)(316002)(71200400001)(38070700005)(66446008)(6506007)(52536014)(66476007)(76116006)(66946007)(83380400001)(54906003)(966005)(9686003)(66556008)(55016002)(64756008)(5660300002)(508600001)(21314003); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?uwydt/nY91XVyQqba7DqIyiBpPiDpKTrBrV15KW9LuN4T4PR0RYR8cCbuKvy?= =?us-ascii?Q?6ABIOz4rkW1sMOcnaUV84Q8/cWY1jltDVbmhzLF5plSkViGoC8mkImTPt56W?= =?us-ascii?Q?uP56dG9NEuQZlIIKTTTI7/ZfOa3YyDlNAdQOYWYh68eSWVOeq9KOVFTsykx6?= =?us-ascii?Q?8IJ9eHeD5zAyVrcoZwEsmloXLZAzpB4XKBJrG7mBjvpfh5Aw7q2jeoFiUItT?= =?us-ascii?Q?jLCynzMe18MxtuUV9uS/cWuW5VaL/qiow4TGFt9QRPRRfSDHKul6UwXQOMrq?= =?us-ascii?Q?NFiHY0v60+iH/yKAWnrF1Bf6IkW9auWOV9rv79gvg5tmKutQc2xGs2e5QNz1?= =?us-ascii?Q?DKl1LbfA1s67zQ40z37Y3c+LJ8as7KTro2j+gYcijL3RSUzrJlcOv+Yjvriu?= =?us-ascii?Q?Ogi2YBDwNugprpqH4UrR1MrP/B0SYRn672cjPxDQ27QLKakmOGIGNezxwriX?= =?us-ascii?Q?lwkyzs3F0QRq9JFk9Uu0ETWpHGQDgFogIQzxsYC8MjGjkOJDHb3NIHg8vMLb?= =?us-ascii?Q?Mj212Fw2OmTRYer0i3A9c5aLUdTQkdxMlhhPSOuwnAlVFhrWST4yU+Gdfgjx?= =?us-ascii?Q?KPKWluwLMggewhpzyIPUwbueU3EmEcS7+Pkh54AKOLONWheLS22j2Hwfrlrf?= =?us-ascii?Q?nss8v/xjL2dOJbBkwlllzaDSv0th+lg9twIzcCl5tV/1epwzriGwqnxgSqwo?= =?us-ascii?Q?9vHuruS0Qhb2+bIZiGTEu9MRVikaNw7gvWg0ydG1gMaipIGyrJGV4L92KXMH?= =?us-ascii?Q?5I1D5D3kx2qnPgfwvElFUoczpkUN80ppMerbj2porJEV9ljpD14BGYeQDBB+?= =?us-ascii?Q?cedxJqx66Clgny+hCD7SycMQps/knxU1o9K05DN0wq1wIEXxpH4FkOKumdsR?= =?us-ascii?Q?V0RXRBeVwkM5ssFgmAct2V8s/Es06k1ZRQRVH3aNT2rElL9cc9sisPDieyJl?= =?us-ascii?Q?0/ssXS8d3Uuu9JFgeuXdKgEU3QfFt/Meu2nw+f0uuBiAA8XBsgMKx46Zn2Kk?= =?us-ascii?Q?P7cb06N65OSBKpnCuc/QFZG75L86JQLxHh40PdYIFErnlmHWAaEs0ImG7njC?= =?us-ascii?Q?D3urtKHYmkkmTHgUMt2otV0EsZXctYqoUL8jrNz9c44bh6PW9lO/S9n8T8Mk?= =?us-ascii?Q?f1H3yEHwXgqa5vv8c6hYmZwjPWa/kIIsfE91KmZfSvdpYSSBrZzwgyM1bB6w?= =?us-ascii?Q?DETZX0ydHWa1hZwmnA3BsHfW88g4ogb3MfJE8eFhA2m+CXzuAN/vHsUb29OZ?= =?us-ascii?Q?qbJVi/lciAUV0GPGmCk7gb8FWROTadEne+lfT0AUbaGkmLt+C1T1/OYIH1+M?= =?us-ascii?Q?w7kC4V+0wEZ9R4GeYdSKtqg3?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CH0PR12MB5091.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1a133d19-8af4-4538-a3e9-08d98fdcc5db X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Oct 2021 13:07:42.3474 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 43083d15-7273-40c1-b7db-39efd9ccc17a X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: xKSPPQqwWuvWw6Cc4KQA+hjGRqwQ/jT7AYDIzhVqZwoWwqugh/AGhC+/yBwBMijmm/d5q3i61owPU/q0m8d3Gg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR12MB5315 Subject: Re: [dpdk-dev] [PATCH v4 1/4] mempool: add event callbacks 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" [...] > > @@ -360,6 +372,10 @@ rte_mempool_populate_iova(struct rte_mempool *mp, > char *vaddr, > > STAILQ_INSERT_TAIL(&mp->mem_list, memhdr, next); > > mp->nb_mem_chunks++; > > > > + /* Report the mempool as ready only when fully populated. */ > > + if (mp->populated_size >=3D mp->size) > > + mempool_event_callback_invoke(RTE_MEMPOOL_EVENT_READY, > mp); > > + >=20 > One small comment here. I think it does not happen today, but in the > future, something that could happen is: > - create empty mempool > - populate mempool > - use mempool > - populate mempool with more objects > - use mempool >=20 > I've seen one usage there: https://www.youtube.com/watch?v=3DSzQFn9tm4Sw >=20 > In that case, it would require a POPULATE event instead of a > MEMPOOL_CREATE event. That's a troublesome case. Technically it can be only one POPULATE event called on each population, even an incomplete one, and the callback can check populated_size vs size. However, I'd keep the READY event indeed, because it's the common case, and the first consumer of the feature, mlx5 PMD, can handle corner cases. It is also a little more efficient. POPULATE can be added later. > Enhancing the documentation to better explain when the callback is > invoked looks enough to me for the moment. Per Andrew's suggestion, it will say: "Occurs after a mempool is fully populated". I hope this is clear enough. > [...] > > +static void > > +mempool_event_callback_invoke(enum rte_mempool_event event, > > + struct rte_mempool *mp) > > +{ > > + struct mempool_callback_list *list; > > + struct rte_tailq_entry *te; > > + void *tmp_te; > > + > > + rte_mcfg_tailq_read_lock(); > > + list =3D RTE_TAILQ_CAST(callback_tailq.head, mempool_callback_lis= t); > > + RTE_TAILQ_FOREACH_SAFE(te, list, next, tmp_te) { > > + struct mempool_callback *cb =3D te->data; > > + rte_mcfg_tailq_read_unlock(); > > + cb->func(event, mp, cb->user_data); > > + rte_mcfg_tailq_read_lock(); >=20 > I think it is dangerous to unlock the list before invoking the callback. > During that time, another thread can remove the next mempool callback, an= d > the next iteration will access to a freed element, causing an undefined > behavior. >=20 > Is it a problem to keep the lock held during the callback invocation? >=20 > I see that you have a test for this, and that you wrote a comment in the > documentation: >=20 > * rte_mempool_event_callback_register() may be called from within the > callback, > * but the callbacks registered this way will not be invoked for the same > event. > * rte_mempool_event_callback_unregister() may only be safely called > * to remove the running callback. >=20 > But is there a use-case for this? > If no, I'll tend to say that we can document that it is not allowed to > create, free or list mempools or register cb from the callback. There is no use-case, but I'd argue for releasing the lock. This lock is taken by rte_xxx_create() functions in many libraries, so the restriction is wider and, worse, it is not strictly limited. > [...] > > +int > > +rte_mempool_event_callback_unregister(rte_mempool_event_callback *func= , > > + void *user_data) > > +{ > > + struct mempool_callback_list *list; > > + struct rte_tailq_entry *te =3D NULL; > > + struct mempool_callback *cb; > > + int ret; > > + > > + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) { > > + rte_errno =3D EPERM; > > + return -1; > > + } >=20 > The help of the register function says > * Callbacks will be invoked in the process that creates the mempool. BTW, this is another bug, it should be "populates", not "creates". > So registration is allowed from primary or secondary process. Can't a > secondary process destroys the callback it has loaded? >=20 > > + > > + rte_mcfg_mempool_read_lock(); > > + rte_mcfg_tailq_write_lock(); >=20 > I don't understand why there are 2 locks here. >=20 > After looking at the code, I think the locking model is already > incorrect in current mempool code: >=20 > rte_mcfg_tailq_write_lock() is used in create and free to protect the > access to the mempool tailq >=20 > rte_mcfg_mempool_write_lock() is used in create(), to protect from > concurrent creation (with same name for instance), but I doubt it > is absolutly needed, because memzone_reserve is already protected. >=20 > rte_mcfg_mempool_read_lock() is used in dump functions, but to me > it should use rte_mcfg_tailq_read_lock() instead. > Currently, doing a dump and a free concurrently can cause a crash > because they are not using the same lock. >=20 > In your case, I suggest to use only one lock to protect the callback > list. I think it can be rte_mcfg_tailq_*_lock(). Thanks, I will double-check the locking.