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 C3239A0C44; Mon, 12 Apr 2021 10:27:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 29DBC140F88; Mon, 12 Apr 2021 10:27:34 +0200 (CEST) Received: from NAM11-DM6-obe.outbound.protection.outlook.com (mail-dm6nam11on2045.outbound.protection.outlook.com [40.107.223.45]) by mails.dpdk.org (Postfix) with ESMTP id 18CFF4014E; Mon, 12 Apr 2021 10:27:32 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=bzrYV5iDs/2DhsVdanLrY8NewSVZVCljKHQkCh52wTwv7iuv98zrFeEv70nClzmqLSyiK9DPz0mO+l6d2WZ0TUH3Yhi0grXyB+FFizYyNl21NhuybZOM2UJqXCNN0EfbDHNrgKmWNs0Sw00+fAk1T60YFYUB6pHiuh9gtcnMt2W0xW9nPEEBZ+RrG4HqNgfD6U1XpE0oaa/X0x/X/1cNc6VT7Ht7I8JLP+tfPkV4ifOL5dT9AhCImXDypvpgkc+aA2i39y5G3TojBrKG5e/ZbHjeKUDtY7m9oNd9t8fQi+KkWbVsuC321YTNlRW9AeZylVB8jM7n903HnHeQI6Ybvw== 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-SenderADCheck; bh=ebzFxvgYwvuuep7Nhs9ybtXvgI2a4HgTIfb0LcJRLsM=; b=fo32K4cPeQjr+ySmn8vJ651zd+1pMDyoSIFMYd7PlR6jC7rNIz8veU85lBVZupO6fd9FmyjqHK56l82UgsxstK8bqr5PYUtGuT5HfTASda1mO0uyZxo1kp5HWuupUfQ4j48ZBaFBBOv7FvFJZLnfBvKsuBzlHX6KjCeR0F2bIFBDJz/8H9narC3QyXevsoAsbhJGwdGb96B4jHyMO+6QKfnm3VaFESUl29xuq77pOieqAEBigLpmNxu7l89fsx/XJwsuMD+K01N+2iBwArV5ZBkzcvQl5ZIB16ba/VMYW7jBAF9NRH6vhNwkc5NI0y1UJsiZMBMh2FrR384tPeG2Mw== 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=ebzFxvgYwvuuep7Nhs9ybtXvgI2a4HgTIfb0LcJRLsM=; b=Y3hUGdN+X9+X+r6SMtRCQpQnY9Z0erIF7cszoGc8xDzA9fvrU4UKIIEBu9roeRP2cuubapJMsWyGhEgXOBt94lICKQe99NYBtRtn2aj0XZ0X3HMDcAv8kyM2C81D4wZRD0YsyBlc2pzGZJHUrTfs25/TikgK61jO2j24Y7xvSdweN6uRc1AFYGl1/ASA+YSMaSI7mHSCNY/hyIy9HLQAr18I76emp1q0jhAhlyuQu/E7S6t+omF0IHPTfFj7xx6IG9dZNKk99b01GiC1F4pCBJMvI7chFUO6QcLZGbYOX7Hqp7LzysJl3SbCGgxY9zXjEdrYXjeYsArWCcOTZ3IX/g== Received: from DM6PR12MB3753.namprd12.prod.outlook.com (2603:10b6:5:1c7::18) by DM6PR12MB2921.namprd12.prod.outlook.com (2603:10b6:5:182::17) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4020.20; Mon, 12 Apr 2021 08:27:30 +0000 Received: from DM6PR12MB3753.namprd12.prod.outlook.com ([fe80::20f7:11fc:9d46:9258]) by DM6PR12MB3753.namprd12.prod.outlook.com ([fe80::20f7:11fc:9d46:9258%6]) with mapi id 15.20.4020.022; Mon, 12 Apr 2021 08:27:30 +0000 From: Slava Ovsiienko To: Feifei Wang , Matan Azrad , Shahaf Shuler , Yongseok Koh CC: "dev@dpdk.org" , "nd@arm.com" , "stable@dpdk.org" , Ruifeng Wang Thread-Topic: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache Thread-Index: AQHXG8cWFxZYWyFpJUm7yYF997hX4qqwqlRA Date: Mon, 12 Apr 2021 08:27:30 +0000 Message-ID: References: <20210318071840.359957-1-feifei.wang2@arm.com> <20210318071840.359957-4-feifei.wang2@arm.com> In-Reply-To: <20210318071840.359957-4-feifei.wang2@arm.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=nvidia.com; x-originating-ip: [95.164.10.10] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 41e1e896-7c84-4a25-457d-08d8fd8cd063 x-ms-traffictypediagnostic: DM6PR12MB2921: x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:7691; x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 0kedTB1n5xpKsXuhLvaFETunF8cKhVZdHxOGW03Ls4mwLzJLloqs27rHBXO8ekGK2L9Hlqid/SrJBXXFyvJOWgGOhaGeOUwEfAupGkVQ2FXz1FHRMiF+GSDKUIWvxizoik9fqERdmMdIh92mWbzrr/Buj6vxHK1Vizar06JLhimJsmr3g05x9J9xyrFUEOBLBEQVUGcJ+tVQ3tm8GOPR5JQZhrFqC7X/JZYfnCAiDC9nXtk1QOB1rs6ny4xMYQNB3AQO8y8PmV3agIqvfcINIKst/WZOd15Rp9RvhFxihtpRDIpjivsVDZ2QqoDYDzYvGnuynvVzMlnchcmDbivzn9CudNKZwJ8LcveszREORDjtgQauuKHiP8IbfBZAlGPoOL6frUnvjAI9nI/gPh6irFAnU89gpQj1EEtYAEpOF5pAj7gxzvktQ6a/L5o7ECVBCzmg0fKgi+urYQpdc7f3BLUeEzpEwJFXRQlHb/ruzSUialXrgw7uBcZ5jIOLjZhK2wz0M9AWxRGY/aDD5KS2EiNRgq52DjswBu19NcUagYLYlv0z8m1zTD+u2HqkTBcYKmzZZFAQ8sUZWgItfGTlSU592+KISu+PD1jgLVgS5jQ= x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM6PR12MB3753.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(136003)(366004)(376002)(39860400002)(346002)(396003)(8936002)(478600001)(83380400001)(8676002)(86362001)(110136005)(4326008)(9686003)(71200400001)(55016002)(53546011)(7696005)(2906002)(66556008)(33656002)(6506007)(64756008)(66946007)(66446008)(316002)(52536014)(54906003)(186003)(76116006)(66476007)(38100700002)(26005)(5660300002); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata: =?us-ascii?Q?glSka57luHXXVUwYI77Wc3XwzfdnvNDJXVpEBmKMqPuC7THYZ08IozYeuGhF?= =?us-ascii?Q?3bIw7IeWKz+eVo78cIM18Ae59xuQS9mFJ6M78ZFhgb/chK4H2aHeGLV2Bk5x?= =?us-ascii?Q?cJ6qCdh0JSa5RK+hsm9HyPN3YKAk7/Rpmaf5nUCJmCaQRdGjNpm4hlIgO4aq?= =?us-ascii?Q?eUno3E37elc+sXm1S8d5kRcANeDo87YA5AfTXDy30fghXHRC+egWS4szMg84?= =?us-ascii?Q?UEl5Eh/6um2sujlGwr6I3++owWTFVyoSPBcuc27OobHcDVQl3yGeL5Rw8UFp?= =?us-ascii?Q?f4vLnIAoDNEi9bYUMlF0D8bRRZLOfslhFbmOg1/67aCM0Ae0TOESZifP0adj?= =?us-ascii?Q?sQyBxmF0VAQ4cdjsNgxosoS5jVYauUZnqer/077lkVPuWMCK+qjKzdOImMg/?= =?us-ascii?Q?h7z4XYhLwYPkqFzrpulpPTk3uMPJlKqDX6D7SnD6KoIQP/GCPQq6WQhqO6Yo?= =?us-ascii?Q?WZdH518cIXVnz+HcR4SYYeyyTkrUcIFWM9cM15UgGrpRjl4w9nUIeXo5zJv4?= =?us-ascii?Q?2zaYg9KgGTrNJ1xSRQiHoDz7im3FGxd4vbX/3CIRnKb3/2jdHJ/8zc6CWX5Q?= =?us-ascii?Q?qaVjIJmg8IkkrD4E+Zm06PrTTC3PohabJaOVqXe40z+optc9yjNqeWER/IfH?= =?us-ascii?Q?Pfwm+TQDiKOIvQHG6rAEm4O84xBd3Gv2rkRVhmNTAu1WWZhs99vl3W6Vr2hW?= =?us-ascii?Q?g9XD7FQ8s38mvpEVxxB0mbbJT8ZQAp7qFwvSVmVw7q6X9L4Q9AXVHvCfQ/Nx?= =?us-ascii?Q?dHyH9aBJSnsOJzbZANSJ/IdGNFzxV2U6mZneIw+JcU6uoeZRLThGOhKj/h1H?= =?us-ascii?Q?DbJfF+bmx31jOQZJzhVzkuzOHDl7ffRph5+VOMCcU5jyQOpCPQL6WPkhoJzE?= =?us-ascii?Q?RrYF0x7HzThPIeHzzdbt4EU6KUAORNcwts9tFnWGfl0wyts6fuwpqA9UWZsj?= =?us-ascii?Q?GwRjQWbHvLEfhT7hRsbQhOkucffSEOGtfp3NSRplXTuupg8A6AkmVXufHxN0?= =?us-ascii?Q?9VZEW57g675+5VDbVmaC8jC/0oS3LghdOQEP+axiAmKRED/hWKUKciyfJEHW?= =?us-ascii?Q?r5+U8WfkqRh+0ninXdxtZJ44LJ5TzDmpdargOrsXF1daUqQKzXKOy83SY7am?= =?us-ascii?Q?TE73d53n5+7DFjztxv2iApOXBoCiYUiFymVDqB9UesR98QNZo1ZgceFWKB4B?= =?us-ascii?Q?S+qH0jAkTmmvlej+YcuUmyBssQrmSOlvmfmNwTrVOFf0DoFrFpJOI8/Y22Kp?= =?us-ascii?Q?Xr1OmZQUfUCbVIw6cEKW/dmIUjoVPJyjauSyswl2C1CSm0ZEJG26fO0SIkLU?= =?us-ascii?Q?iF4=3D?= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Nvidia.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3753.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 41e1e896-7c84-4a25-457d-08d8fd8cd063 X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Apr 2021 08:27:30.5001 (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: QihCT8fP9qP7kU/y22IPLD3VkUDUlvEeqIKzdSoBejNNesBI4mRe68kwMgoG4vcyQG5018sUvbGh3qurbEMLgQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB2921 Subject: Re: [dpdk-dev] [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache 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, Feifei Sorry, I do not follow what this patch fixes. Do we have some issue/bug wit= h MR cache in practice? Each Tx queue has its own dedicated "local" cache for MRs to convert buffer= address in mbufs being transmitted to LKeys (HW-related entity handle) and the "global" cache for all MR regis= tered on the device. AFAIK, how conversion happens in datapath: - check the local queue cache flush request - lookup in local cache - if not found: - acquire lock for global cache read access - lookup in global cache - release lock for global cache How cache update on memory freeing/unregistering happens: - acquire lock for global cache write access=20 - [a] remove relevant MRs from the global cache - [b] set local caches flush request=20 - free global cache lock If I understand correctly, your patch swaps [a] and [b], and local caches flush is requested earlier. What problem does it solve? It is not supposed there are in datapath some mbufs referencing to the memory being freed. Application must ensure this and must not alloca= te new mbufs from this memory regions being freed. Hence, the lookups for thes= e MRs in caches should not occur. For other side, the cache flush has negative effect - the local cache is getting empty and can't provide translation for other valid (not being remo= ved) MRs, and the translation has to look up in the global cache, that is locked now for rebuilding, this causes the delays in datapatch on acquiring global cac= he lock. So, I see some potential performance impact.=20 With best regards, Slava > -----Original Message----- > From: Feifei Wang > Sent: Thursday, March 18, 2021 9:19 > To: Matan Azrad ; Shahaf Shuler > ; Slava Ovsiienko ; > Yongseok Koh > Cc: dev@dpdk.org; nd@arm.com; Feifei Wang ; > stable@dpdk.org; Ruifeng Wang > Subject: [PATCH v1 3/4] net/mlx5: fix rebuild bug for Memory Region cache >=20 > 'dev_gen' is a variable to inform other cores to flush their local cache = when > global cache is rebuilt. >=20 > However, if 'dev_gen' is updated after global cache is rebuilt, other cor= es > may load a wrong memory region lkey value from old local cache. >=20 > Timeslot main core worker core > 1 rebuild global cache > 2 load unchanged dev_gen > 3 update dev_gen > 4 look up old local cache >=20 > From the example above, we can see that though global cache is rebuilt, d= ue > to that dev_gen is not updated, the worker core may look up old cache tab= le > and receive a wrong memory region lkey value. >=20 > To fix this, updating 'dev_gen' should be moved before rebuilding global > cache to inform worker cores to flush their local cache when global cache > start rebuilding. And wmb can ensure the sequence of this process. >=20 > Fixes: 974f1e7ef146 ("net/mlx5: add new memory region support") > Cc: stable@dpdk.org >=20 > Suggested-by: Ruifeng Wang > Signed-off-by: Feifei Wang > Reviewed-by: Ruifeng Wang > --- > drivers/net/mlx5/mlx5_mr.c | 37 +++++++++++++++++-------------------- > 1 file changed, 17 insertions(+), 20 deletions(-) >=20 > diff --git a/drivers/net/mlx5/mlx5_mr.c b/drivers/net/mlx5/mlx5_mr.c inde= x > da4e91fc2..7ce1d3e64 100644 > --- a/drivers/net/mlx5/mlx5_mr.c > +++ b/drivers/net/mlx5/mlx5_mr.c > @@ -103,20 +103,18 @@ mlx5_mr_mem_event_free_cb(struct > mlx5_dev_ctx_shared *sh, > rebuild =3D 1; > } > if (rebuild) { > - mlx5_mr_rebuild_cache(&sh->share_cache); > + ++sh->share_cache.dev_gen; > + DEBUG("broadcasting local cache flush, gen=3D%d", > + sh->share_cache.dev_gen); > + > /* > * Flush local caches by propagating invalidation across cores. > - * rte_smp_wmb() is enough to synchronize this event. If > one of > - * freed memsegs is seen by other core, that means the > memseg > - * has been allocated by allocator, which will come after this > - * free call. Therefore, this store instruction (incrementing > - * generation below) will be guaranteed to be seen by other > core > - * before the core sees the newly allocated memory. > + * rte_smp_wmb() is to keep the order that dev_gen > updated before > + * rebuilding global cache. Therefore, other core can flush > their > + * local cache on time. > */ > - ++sh->share_cache.dev_gen; > - DEBUG("broadcasting local cache flush, gen=3D%d", > - sh->share_cache.dev_gen); > rte_smp_wmb(); > + mlx5_mr_rebuild_cache(&sh->share_cache); > } > rte_rwlock_write_unlock(&sh->share_cache.rwlock); > } > @@ -407,20 +405,19 @@ mlx5_dma_unmap(struct rte_pci_device *pdev, > void *addr, > mlx5_mr_free(mr, sh->share_cache.dereg_mr_cb); > DEBUG("port %u remove MR(%p) from list", dev->data->port_id, > (void *)mr); > - mlx5_mr_rebuild_cache(&sh->share_cache); > + > + ++sh->share_cache.dev_gen; > + DEBUG("broadcasting local cache flush, gen=3D%d", > + sh->share_cache.dev_gen); > + > /* > * Flush local caches by propagating invalidation across cores. > - * rte_smp_wmb() is enough to synchronize this event. If one of > - * freed memsegs is seen by other core, that means the memseg > - * has been allocated by allocator, which will come after this > - * free call. Therefore, this store instruction (incrementing > - * generation below) will be guaranteed to be seen by other core > - * before the core sees the newly allocated memory. > + * rte_smp_wmb() is to keep the order that dev_gen updated > before > + * rebuilding global cache. Therefore, other core can flush their > + * local cache on time. > */ > - ++sh->share_cache.dev_gen; > - DEBUG("broadcasting local cache flush, gen=3D%d", > - sh->share_cache.dev_gen); > rte_smp_wmb(); > + mlx5_mr_rebuild_cache(&sh->share_cache); > rte_rwlock_read_unlock(&sh->share_cache.rwlock); > return 0; > } > -- > 2.25.1