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 EF5AFA034F; Wed, 10 Nov 2021 16:24:26 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BEC064068B; Wed, 10 Nov 2021 16:24:26 +0100 (CET) Received: from NAM11-BN8-obe.outbound.protection.outlook.com (mail-bn8nam11on2088.outbound.protection.outlook.com [40.107.236.88]) by mails.dpdk.org (Postfix) with ESMTP id 4029C40683 for ; Wed, 10 Nov 2021 16:24:25 +0100 (CET) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dAxjxKW7zbk65aZjqH/iVhGqJuiFE9wXxSqEpaXq2buf0Pkdv7htZ+W4x5QDHWk+/CLhQc1J5RfRDBMrXKP4iCaxVwI47MOsb1Myp1tVF9D/tAlI1qACv+or7An2CURWbuvS1pBPct4Q2YEAgQ4JDEMsJmZ7gS44EM2kCwA+eZwdnCp2eANKFe+RHAodR1Xu5QkymG8+7yHEcIS5eiG3d+SsvuZLfKRh1u2wIciPomi+sofo2eXRGLBqoBGobOXcdtOl3vIkVSxQgWIPsk7FSkMIz6lCCHFweo/i0lcC34wbBU0zw6p76bEDq8l+v+/iL7hRFlUaO5m8GP93h5TV8w== 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=/U2F39agcyfdUb7fyQTZ4U/R2qLS8AMnz9bLV57dkwU=; b=QVp89LiZMZw0Uuq8IEe4py2UiEwfC10twujhDveGtx7g9D/5bCb7EhwRAmhGd69tA3fyZHXmbklzZK6HhDdU8MNiCkCXshsYasLqkcimhH0hi2GlwQyV7dHcUFotO0nl5jh/OoF5MT+Wsu/nUG+D671XhfE5FK0nMRR68y/tjUzALa/AV0ik+GM0n68GwS0VU2J2BQg5ZGnPMd48y/5+CnrJunsxaggXQ5qCL3kmJrj3Pt29GHWD9ilbchEHCnTi9sMXRjGDF1ZHQ/iSOBF1T0DhHXfp5w1dugEMgnbmIRqikGJUED0/6rKJuiHWnKl4lrMr8NVTNh6+CGYoQdvPCg== 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=/U2F39agcyfdUb7fyQTZ4U/R2qLS8AMnz9bLV57dkwU=; b=fTZC2m1D/0o3iKiBnGzPaE0Pk57+pq5MWjmm+J1aNy1Maq8DpQysqnjlptB4dIzEGrSniK08GtfwpAMAy6x4UAZIsSllgBxUTwIXlOwFmjBhrkAN+z7tLGGu9dRSFXf3FyYrDXlWEeluHO2G51N0hDKzdf1R4CjumJAfhEonM+GQEy5VMSyVV4w69pd/UR06B27nlbYSbEZUlkmAfgEhGkwXx8k1X3/LRTSLvTgF1iqaoaTVaCzsEUO9mRqFSJ6aPzhzfgbFWQSJZI70AeUg3H7ITWuFm5XMQlH6TsC8CVAq3YuOuwONXJ8i2zK+pwWMUyqPmvI924oyrXa0tw7xnw== Received: from DM5PR12MB1755.namprd12.prod.outlook.com (2603:10b6:3:107::9) by DM6PR12MB2937.namprd12.prod.outlook.com (2603:10b6:5:181::11) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4669.15; Wed, 10 Nov 2021 15:24:23 +0000 Received: from DM5PR12MB1755.namprd12.prod.outlook.com ([fe80::f0d0:7b3c:2e7f:cf1e]) by DM5PR12MB1755.namprd12.prod.outlook.com ([fe80::f0d0:7b3c:2e7f:cf1e%10]) with mapi id 15.20.4669.016; Wed, 10 Nov 2021 15:24:22 +0000 From: Bing Zhao To: NBU-Contact-Thomas Monjalon , "Yigit, Ferruh" , "Ananyev, Konstantin" CC: "andrew.rybchenko@oktetlabs.ru" , "dev@dpdk.org" Subject: RE: [PATCH 2/2] ethdev: fix the race condition for fp ops reset Thread-Topic: [PATCH 2/2] ethdev: fix the race condition for fp ops reset Thread-Index: AQHXx+jaMkEdAq/hoUmW9xWY0Ot3oavgdVaAgBx7FgCAAADAgIAABbOAgAAHP4A= Date: Wed, 10 Nov 2021 15:24:22 +0000 Message-ID: References: <20211022211407.315068-1-bingz@nvidia.com> <16525cf6-bf10-e637-3f03-f4d415ff6bb1@intel.com> <9516865.9GKQoxltXr@thomas> In-Reply-To: <9516865.9GKQoxltXr@thomas> Accept-Language: en-US, zh-CN Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: monjalon.net; dkim=none (message not signed) header.d=none;monjalon.net; dmarc=none action=none header.from=nvidia.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 89ad1592-59c2-414c-e0a9-08d9a45e2c6a x-ms-traffictypediagnostic: DM6PR12MB2937: x-ld-processed: 43083d15-7273-40c1-b7db-39efd9ccc17a,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:8882; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 3bFxQH8IDR5jK/2jg7Ui3h89kv7ZtIelJ9jxaxGt+MVwKHrqaoDYMU+xTXmLdMMiNdWgUaKVN/x2VaYhd2y9LSObXCPUrcnbTzLu6ETY/xRKRovWnxS6TbqeMB8IxXcdMb7wJ9EEOR2BTihnhcLO5c2z6hCFsiv0fMUhl+nre0l07VYhAd4/pLBm1qVIlP064hU96JU3Ty1ExpgHx3juCmKPNK7Tn0B5Epbu44NCSOrbjQtRsB2O8T1ClKIHv0vMKW/BE7KcV0d/9QFbwcppTMOxZ8Pz/ElDm/ZlUnh0ER3Db7MwuPnKzUdbPBLEFRN1is3Obu+TIS26Zh3jnRt1QlIE+NbZij4qHDJmOWUeFRmXZ1dnzdJ393xt4AmcVNzAMMqU8BG/brLOiY/L8d/lP0pIWSIBD9QcQbHa+i/lxUxLC19ECYxSZnlfX7UoIEFQ6BiH3GeVRg3/OFWf4kc5K/+78jfUCs0rxydnEonUq7X3h4MzDii54ItwfZqH+JZscYDs2f7REkG3Z+2gClTrLFMSty/PubAx4fy8AdtlgiFrvkwNKKzWJ2zlHi3hzqf/zwhK8FRiOuF2lGpPDwKSgaS2Z//T1iqG8DqWts6oA2EMWo9cR+UuofYJTQJcQNAsBAOVTGI/cXLsrxXvbW2gTW0f7ax9z6bQWAXds/a4WMGsqCAeLhcyv17qZ92Obxm9V5smEicWvY9CA+Ovz34FbA== x-forefront-antispam-report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:DM5PR12MB1755.namprd12.prod.outlook.com; PTR:; CAT:NONE; SFS:(4636009)(366004)(64756008)(55016002)(9686003)(316002)(66556008)(5660300002)(7696005)(2906002)(4326008)(76116006)(52536014)(38100700002)(66946007)(38070700005)(86362001)(508600001)(53546011)(6506007)(26005)(33656002)(110136005)(8676002)(122000001)(8936002)(83380400001)(71200400001)(66446008)(54906003)(186003)(66476007); DIR:OUT; SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?Ia1IgFjvmQZd+2siTPRjFUdJQZmm+bLz/zM7okBdWxlLU7NqvhC2qw1Y6wE3?= =?us-ascii?Q?O3TYrTKSTCgmJ8X3u4aOPKR9M9XWoncUXrswEXVnov1wfpI+vbNScu8zYHZj?= =?us-ascii?Q?Bu9Fsd57pHL9qQ21agXQwDRayidx5s5S6MU5jbEfJUZ0aglAe9ZHh6HQkn3O?= =?us-ascii?Q?hOSUlAQo1sLqgosnAuiLw4dm5NNVnOtr2eqktX3XqwgaNIzLKBoiYNVTwiyX?= =?us-ascii?Q?rCEuYOc6BBlCE+WZBRnSJCLL/ir3R13wkwJYUByhSqy2IHdS1AQ5cgQw9SwW?= =?us-ascii?Q?+8cEyR6p+9OmZjGS49KIrPp7lKu8xNhFJKXBwcUy4G6pskXoFVfcaPe91qQe?= =?us-ascii?Q?QFdUzYIXVpRQJ4M+J618K1wYUvrUL+QVbd/Lz2zuErzKZbnL59yzdaltzBEc?= =?us-ascii?Q?xPMVpsRjT5n0gzraWFnoH3RtBrgrCDTK/n8eau5r6ByW74N8Y5/bdvnQYeXd?= =?us-ascii?Q?OdzRBS+ysCl3q+PLd872+RvivhGdJON6UNanDjKMAPwT0OZ8t/WbUTk9NbjF?= =?us-ascii?Q?BWfiue6IK7e38jaiVbs1rkagbMRSZNpw/NF3Q9Db4fRajEYwJP/IfmaswWxe?= =?us-ascii?Q?cx2U7gqBSKpFC1lvef5soxjLTEO1dRP3gLOrJGfLvSnIsIcEiyzHzJM2Iqhe?= =?us-ascii?Q?Ht3lPQr4KjU4e1WQPq4xNS6yqaddPsyI87KSzx8//sHMJpFpCwZFgIHzZxz8?= =?us-ascii?Q?aNK3AVNy0J+nyCYQfek/Pon9xA5nQ0cZLysQDquIBzfHHL6EOi1gEw46FIHF?= =?us-ascii?Q?vRaxmUh6O++e2Hy/krz/wmDQLOEMIgmightQncX/r0NGJ/NGyf24T0t0PCn/?= =?us-ascii?Q?D5VS39ceTQ8ENJKJ5D6VTufBZl6vI1PPlNnOUW6qE9tCzD8EMlVcbksFoL5W?= =?us-ascii?Q?tH+Dv+i91wkWKSYaNWSP7oR/piOBNWi5i1/MyXEuKJr1x+9l6jVGNmEgWmVj?= =?us-ascii?Q?NXazx2xWswvHeM4BJa5ErteyAA5Wv0cq6JnElel9yu4HhNczL4OxWymCgint?= =?us-ascii?Q?0aNUxfMqSti0+YlUprM/ZELegVTmBLL+LAXjaVpS8ML/aFxZhS4lp4mNq7B+?= =?us-ascii?Q?R2CflRD2BT0r7B+KZR3dOC6tO6PDsCFYY4dGDvCB527JVXgOOIP7twJmQrnC?= =?us-ascii?Q?LNq9gF2MQbXNFR8y+teAZh+ctSr1WUtZE9VBKOJZmJey467sM5xscUH1tzMZ?= =?us-ascii?Q?6WDPdpHmFAKleQr/7bq8CS1nvQAinJBj68bqetO6DkI3BuMzlItUWWW7tjws?= =?us-ascii?Q?lZIdtHAq7VPagb63pO7BW0if4/31TOrV5jbNnb49d4wPMsaKXPyvGVZEUY48?= =?us-ascii?Q?cxylbibB8Ad/BdHmq4jQYHaF8w13o6BiYtea/Q7cBIeARmFu7HdaP8RDCy4x?= =?us-ascii?Q?fYmTXbJnl8OrSmr9lzI+HLlh9oNPc81+aMzk27hHEQU6RrOZ4jgMdQgLMnKC?= =?us-ascii?Q?HoX8KFRnwGTFzmuYgOjEmIx/W5L1AjqSy2XOl70X25yKhX+1H0UpVcd7gd5a?= =?us-ascii?Q?KB0nhhlTo5L0womPOvy1Jr69yIyTrgJOzw8FOKyKr/FejZt8oZffOCaIg4YG?= =?us-ascii?Q?QUFcPYIPOrBpNIHPPUVMS6Iw/gsKbvsxrPd3G4195/lROyGrtNT5r+YtVjii?= =?us-ascii?Q?OKbEbirWXElqJQehyH4K6Ak=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: DM5PR12MB1755.namprd12.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 89ad1592-59c2-414c-e0a9-08d9a45e2c6a X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Nov 2021 15:24:22.7099 (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: fbRPVHMFTTBDkLN5I53WjTnsf2Cm/rz/VTuXVDmbR9O/VwKv6/sLuVLSUKZa76l3BXIA2sTP9UzPgjwPRQVkjw== X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR12MB2937 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 Yes +1 Let the application handle it once there is an issue. BR. Bing > -----Original Message----- > From: Thomas Monjalon > Sent: Wednesday, November 10, 2021 10:58 PM > To: Yigit, Ferruh ; Bing Zhao > ; Ananyev, Konstantin > > Cc: andrew.rybchenko@oktetlabs.ru; dev@dpdk.org > Subject: Re: [PATCH 2/2] ethdev: fix the race condition for fp ops > reset >=20 > External email: Use caution opening links or attachments >=20 >=20 > 10/11/2021 15:37, Ananyev, Konstantin: > > > > Hi Ferruh, > > > > > >> 22/10/2021 23:14, Bing Zhao: > > > >>> In the function "eth_dev_fp_ops_reset", a structure > assignment > > > >>> operation is used to reset one queue's callback functions, > etc., > > > >>> but it is not thread safe. > > > >>> > > > >>> The structure assignment is not atomic, a lot of > instructions > > > >>> will be generated. Right now, since not all the fields are > > > >>> needed, the fields in the "dummy_ops" which is not set > > > >>> explicitly will be 0s based on the specification and > compiler > > > >>> behavior. In order to make "fpo" has the same content with > > > >>> "dummy_ops", some clearing to 0 operation is needed. > > > >>> > > > >>> By checking the object instructions (e.g. with GCC 4.8.5) > > > >>> 0x0000000000a58317 <+35>: mov %rsi,%rdi > > > >>> 0x0000000000a5831a <+38>: mov %rdx,%rcx > > > >>> =3D> 0x0000000000a5831d <+41>: rep stos %rax,%es:(%rdi) > > > >>> 0x0000000000a58320 <+44>: mov -0x38(%rsp),%rax > > > >>> 0x0000000000a58325 <+49>: lea -0xe0(%rip),%rdx > > > >>> // # 0xa5824c > > > >>> > > > >>> It shows that "rep stos" will clear the "fpo" structure > before > > > >>> assigning new values. > > > >>> > > > >>> In the other thread, if some data path Tx / Rx functions are > > > >>> still running, there is a risk to get 0 instead of the > correct > > > >>> dummy content. > > > >>> 1. qd =3D p->rxq.data[queue_id] > > > >>> 2. (void **)&p->rxq.clbk[queue_id] "data" and "clbk" may > be > > > >>> observed with NULL (0) in other threads. > > > >>> Even it is temporary, the accessing to a NULL pointer will > cause > > > >>> a crash. Using "memcpy" could get rid of this. > > > >>> > > > >>> Fixes: c87d435a4d79 ("ethdev: copy fast-path API into > separate > > > >>> structure") > > > >>> Cc: konstantin.ananyev@intel.com > > > >>> > > > >>> Signed-off-by: Bing Zhao > > > >>> --- > > > >>> --- a/lib/ethdev/ethdev_private.c > > > >>> +++ b/lib/ethdev/ethdev_private.c > > > >>> @@ -206,7 +206,7 @@ eth_dev_fp_ops_reset(struct > rte_eth_fp_ops *fpo) > > > >>> .txq =3D {.data =3D dummy_data, .clbk =3D > dummy_data,}, > > > >>> }; > > > >>> > > > >>> - *fpo =3D dummy_ops; > > > >>> + rte_memcpy(fpo, &dummy_ops, sizeof(struct > > > >>> + rte_eth_fp_ops)); > > > >> > > > >> That's not trivial. > > > >> Please add a comment to briefly explain that memcpy avoids > zeroing of a simple assignment. > > > >> > > > > > > > > I think that patch is based on two totally wrong assumptions: > > > > 1) ethdev data-path and control-path API is MT-safe. > > > > With current design it is not. > > > > When calling rx/tx_burst it is caller responsibility to > make sure that given port is > > > > already properly configured and started. Also it is user > responsibility to guarantee > > > > that none other thread doing dev_stop for the same port > simultaneously. > > > > And visa-versa when calling dev_stop(), it is user > responsibility to ensure that > > > > none other thread doing rx/tx_burst for given port > simultaneously. > > > > If your app doesn't follow these principles, then it is a > bug that needs to be fixed. > > > > 2) rte_memcpy() provides some sort of atomicity and it is safe > to use it on its own > > > > in MT environment. That's totally wrong. > > > > In both cases compiler has total freedom to perform copy > in any order it likes > > > > (let say it can first read whole source data in some > temporary buffer (SIMD register), > > > > and then right it in one go, or it can do the same trick > with 'rep stos' as above). > > > > Moreover CPU itself can reorder instructions. > > > > So if you need this copy to be atomic you need to use > some sort of > > > > sync primitives along with it (mutex, rwlock, rcu, etc.). > > > > But as I said above right now ethdev API is not MT-safe, > so it is not required. > > > > > > > > To summarise - there is no point to mae these changes, and > patch > > > > comment is wrong and misleading. > > > > > > Can we mark this patch as rejected now? > > > > I believe so. > > > > > Patch seems trying to cover a wrong application usage, and it > should > > > be addressed in the application level. >=20 > Yes >=20