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 D486AA034C; Fri, 25 Feb 2022 12:20:13 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C4F7E4115C; Fri, 25 Feb 2022 12:20:13 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id EE98E4068B for ; Fri, 25 Feb 2022 12:20:11 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1645788012; x=1677324012; h=message-id:date:subject:to:cc:references:from: in-reply-to:content-transfer-encoding:mime-version; bh=w55OpSqHoWzn2Lutb27Bj5j+hgPvkjNv+JBRAuqLOQY=; b=PGD6K01lfxkplrCKaqP4JDJ3mku4cK9pQQHdiw56bJsdCQnRGr1N8Yln R8CxYL8uUDurtBPZTewXWlA66X4l0EnwyWwI0NUiGbIUA7eyDkjSUCubs B2SdU534F5prYwCVxbdUoJE/U0gB82rEruUwTs6riwcPeAT9xOp0XgugN 4VCqfVld7zvBcqg3vkIGuhrgdEyXCjYYHsUMD7UIHC1A+jjdSzzw+SUvZ xRs6kcn5JOwwRxIheWujxY7XBDm8P189YJzR7yqXvTYaFAmVhKTp4vKgu n8BFsjZ2+EQv7XQFRNsA6+G43VDGucDEbpJ6bZ/VllArK52D4UUEkiiWh w==; X-IronPort-AV: E=McAfee;i="6200,9189,10268"; a="235986137" X-IronPort-AV: E=Sophos;i="5.90,136,1643702400"; d="scan'208";a="235986137" Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 25 Feb 2022 03:19:53 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.90,136,1643702400"; d="scan'208";a="791333814" Received: from orsmsx603.amr.corp.intel.com ([10.22.229.16]) by fmsmga005.fm.intel.com with ESMTP; 25 Feb 2022 03:19:53 -0800 Received: from orsmsx602.amr.corp.intel.com (10.22.229.15) by ORSMSX603.amr.corp.intel.com (10.22.229.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21; Fri, 25 Feb 2022 03:19:52 -0800 Received: from ORSEDG601.ED.cps.intel.com (10.7.248.6) by orsmsx602.amr.corp.intel.com (10.22.229.15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2308.21 via Frontend Transport; Fri, 25 Feb 2022 03:19:52 -0800 Received: from NAM12-MW2-obe.outbound.protection.outlook.com (104.47.66.48) by edgegateway.intel.com (134.134.137.102) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2308.20; Fri, 25 Feb 2022 03:19:52 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=NRMywLCJMmpEcDskk8fGvu4VRnbLplVhptXvkRJ87RNC/Kgtu8L2HpJ5JxpIlbciITkNCCfu+UcZh2cIwcU2Snq8eDuK661VQZpCOdmd/BxA7jP594nRBLugvpl/VQgN4mD+Puv5w2BZsjGUoWeSeWynudYlqKCc3/OWN5G2/uCrduSRMaQ/W5WvIMavCXXzUF/1L+/0/KhOQ60YNcFomjgqPtG6oP5BDasoBSw0hxwhHeJf58WTHcSCgXQnbflZFgjjGd1lrWo8ektbVklvT4w/u9B/DNhsh8U5rjmbUArkQcCVFza+p/QxiWfP/z45aT9dvEvK5/VN8iIYVllsjQ== 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=9on6q69ulS159cO2V5KD6X3/ViBbtVPVNzGIggH8n/s=; b=h7Pjowckv5vuF1+KVJ2pDTmtmYQ7tghxrzP/cviDDq/wP8iIzDQ4AbtcKmusKqhhAiVS3sEGH5wnT0lYN5Ge0QPKSM9FDJaHWw1zW4CtaWuvhuwbrMRHEoYFGG1WT3z9Rh9zDn3s29P8aPZx4ggSKZH/D6Ua/UQ2e/C6iZSjQl1TraFOIJp/dxLi2DTaincQjIU6Vcwe6Dd8Q9dtDoV8YIog2aKqRheMK1hCyscKbT0VnxHGLG43OV2wbXA/vQ6HxCKt5KX+r2EYT/kJzPTjz19dj6xzJeWzTbRWreNkaz2NFurgAoqZVwfLg2FSsDlMccOyAhQ7vwTEkii+nz9r3w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; Received: from PH0PR11MB5093.namprd11.prod.outlook.com (2603:10b6:510:3e::23) by MWHPR1101MB2271.namprd11.prod.outlook.com (2603:10b6:301:52::23) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5017.24; Fri, 25 Feb 2022 11:19:50 +0000 Received: from PH0PR11MB5093.namprd11.prod.outlook.com ([fe80::384b:3fcf:f896:53dd]) by PH0PR11MB5093.namprd11.prod.outlook.com ([fe80::384b:3fcf:f896:53dd%6]) with mapi id 15.20.5017.026; Fri, 25 Feb 2022 11:19:50 +0000 Message-ID: Date: Fri, 25 Feb 2022 11:19:44 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0 Thunderbird/91.6.0 Subject: Re: [PATCH v1] power: add wakeup log Content-Language: en-US To: "Li, Miao" , "Hunt, David" , "dev@dpdk.org" CC: "Wang, Yinan" , "stephen@networkplumber.org" References: <20220222135227.631275-1-miao.li@intel.com> <075cf990-f0b8-8a8c-94c4-420f185e9e94@intel.com> From: "Burakov, Anatoly" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit X-ClientProxiedBy: LO4P123CA0311.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:197::10) To PH0PR11MB5093.namprd11.prod.outlook.com (2603:10b6:510:3e::23) MIME-Version: 1.0 X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id: 15410c13-5176-4efc-a5bc-08d9f850bcfb X-MS-TrafficTypeDiagnostic: MWHPR1101MB2271:EE_ X-Microsoft-Antispam-PRVS: X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: UFSXgMerTyNDQVT3yUbUgMvM9M2CgbmLwX8BrAkCA0CHme/W02c6vnjqDTfiWjMZYQ+l2XnrfLDPI8RJQzU2o4b0OdbRZ0dzPNxRh9WK7DRRGOANpFUvN8OAHufKlPUaW74Va1QgfDQHTnhxqKW1YbdtuNQuyMu6un6APuu4h+wapL7FDU6pTXMI7wTzfrwJxuh0F6fZwrAb24oCQ1X+lODPtCOdiqfQ7vnU4Xk1bamY+GZUU4IunT0BwFONiSu7MAZREQhGm8ZVUZRDarD+LRNa0TIdX4ZLSxcA5UoQ/KcE4g2jIY0/rRm4IJcvFm6NxO6pd9jUSItSz14eZJEiNVjpPtz7rofjpu3/3O1vdJSYZpHZOl1mV9E5LSoYDYm5h+pAQdx6NzhfDaOHqRR6lPc/OPZBCe8PbE6va/u+5ojwHhAp1d8EBoNvEoKJxg05wN+SPTg/eo5SXYF7Cf6HRMeV3G/6VSBAiL2+cH385TfQIaRUq8u9JoVZ8lxy3wSBdo4mR8rLaR+CEpj4IrGyFYajUYR88Y1lD1jKM0X8VhYP6sekiLQliTTGc2eDzSr+dLwU+nmg7RXHE2I0uRODC+kc2DuJKKGMZbscILZi+V6elDsoCrdd1fk8Qsavzl8AynDGEfKsBlIGOV1BFKppqJzkQ6li6g6BEdNJIQabZRnOofoE3/CNyVTPd3e/xkiuM187ocaY/aB5EGUYKCC51TITStLYJVIdQYXKmQy4ofs= X-Forefront-Antispam-Report: CIP:255.255.255.255; CTRY:; LANG:en; SCL:1; SRV:; IPV:NLI; SFV:NSPM; H:PH0PR11MB5093.namprd11.prod.outlook.com; PTR:; CAT:NONE; SFS:(13230001)(366004)(53546011)(66946007)(66556008)(6486002)(66476007)(6506007)(31696002)(4326008)(8676002)(2616005)(2906002)(82960400001)(8936002)(26005)(6512007)(316002)(5660300002)(86362001)(6666004)(508600001)(186003)(110136005)(54906003)(31686004)(36756003)(38100700002)(83380400001)(43740500002)(45980500001); DIR:OUT; SFP:1102; X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MkE4Y3ZKUWIvVXh4T0JVMEJQUVBUT1UwWnluUHpVVWkyM2RmZG92Z3F5bTg3?= =?utf-8?B?cU5GZDRCTkh6d3pXYWFXT3M1R0MzY2cwSXFKSFNqM09NZTc2NXBJNTVmSFl3?= =?utf-8?B?UTFCcjhiMk1TQ0pkWDAvOWFUL3Z1S2Z6ZjFMTFBaV0hJWXJhL0ljZ1RlZ01Q?= =?utf-8?B?emFvMkh1T0xVNjZacFIwMTQwRW5sd0xQVkVmcGtnZkxWaHdrUFZtRkF3VXRW?= =?utf-8?B?eUJVSkVqRU13czJNSWZLSm1LNWdRQmsyNGdEdXY1VUNBamdSQkRzWXRjbW9J?= =?utf-8?B?YXI3S21JWWJZdkorUXFSdURsdVVQZVllUDhLQ3BYU1h0ZWYzQzVnQ3I4MTUx?= =?utf-8?B?MEVIY0lFY3Z0ZzdScDd1YUlYSGFtdWk4UVpwTU80VGpWZW9hamwzTkorUVJW?= =?utf-8?B?dzh1TGFRc0RMZWZuRzcydXBqLzJ6QkNqcHVFamdRWkFsQTBuNWM4SW9vS2I1?= =?utf-8?B?VkhMajVPbG80M0lvOTFjT1BXeGk0L0N2MElrd2UwU2orbmR0Rm5TMHZkUGYx?= =?utf-8?B?bUYvR1c4ekxuSkxpNHlXNE9PRHdRVEpaWHZScjRTOTBCMDYwaEY2S3NvbTF4?= =?utf-8?B?bzJ4Yk9QSTdyN1RYZW9DcENKcXltVzBoUGpRQVZWNmwzR3FEN21CMUUydmpM?= =?utf-8?B?dlJZd0VDdFd1cWxrOTBtTksxbEdqd3Q3NDRtWCt5VU55aEFhdVc5NTlMb0pk?= =?utf-8?B?bTdpMmdObjhKRnhHZXZOK25MTlc2UWl5SjA5aTFHODJ4ZmwwbmVodnRWL3lh?= =?utf-8?B?U0dTUmVKZU9CdGRBWjJqTndObzJDcjZHZCs2UVdxblFYcU1Nb1VwRU5qYUJZ?= =?utf-8?B?VXlHVWx1dnJOVGZ0T1ZIQzhTa21UMWppektjUzV4L2pUVHRKdnk3TEZUZ0Vn?= =?utf-8?B?MXNWOUxwZnYycjBKNVYxWEJKMEdFa20yT202RTMzNHZvYXV1S2xsc0JnYVEw?= =?utf-8?B?TXJUSi82a283R3NNV25SN2dCV0hBcHdabnI1SHoyejVzajJpUWF1aWNsOU9l?= =?utf-8?B?WFA3UlNzR0lWT2c1YXU4RityQjhYYjFFd1BpRmFTYmJUNFdaT2lESzZ2eVp2?= =?utf-8?B?VUhFQTd0RnQ1akZJd3ZBUVZWU0FDaFF4Z0VESlNsM2FDeDlXU1ZPSjdBOC9Y?= =?utf-8?B?SHBtVGJPSE5yc1plc1J3SnQxUFR3MnpVOEo4cWtXZEtYUHhid08yTzM0ZTdS?= =?utf-8?B?SjNQTVFsaDhxbkh2VnFrODMwU2RoNDFMSzErdmtLb2RaRDBnOWFwQ0VNdlFR?= =?utf-8?B?VC9YTmpFazRrS2kxcDJmczhZUmdHQlAyTzIwY0UvNnRwNXhrbVp3SWpkQnNE?= =?utf-8?B?bXRDVmVMalNYVTdWZHEyMk5xNUxVZ1RuS1dEOC8xcjVaY09QNWRqdENGbFdh?= =?utf-8?B?cndydVhWeFRxa01kZ3FLTWc3UDJ2d1poeFB4cnVScE1pbzZPaVEyUVhvQ2dt?= =?utf-8?B?SjFSaUNEcWovMHl6cWFZdFlWQ3dyenVhT0hZMGJNUGFSUmNEY0s5YUJkeVky?= =?utf-8?B?aWI0WVZFRkIvTDlMdGpsRlhQZmYzTmtzTjd0cmo1SkVkYVRnbC9CM1FwbzBF?= =?utf-8?B?M1Ryd3IxS1Z1VnNrMjE3SG93VnduZDB4bmMxMUJDRi9IamlOcERTTG9SaDBq?= =?utf-8?B?QmF0OEJEeVFTZHdEMGszUGNtWlNubG5QcUpGenp3dGtXd0dOSlA5ZkI5QXdF?= =?utf-8?B?dis1WnEzMUpWYS9DTDRRZHcrR3oyUVVEWDROQlhQUzNRbGo4VmRGNytkSDNY?= =?utf-8?B?TUh5TlNDOENWcGFrbXZCZnJVN1U5WnZrRWVjOXRESDRmd2RLeTk3WnVONlIx?= =?utf-8?B?bHpZTHJDSnk3WHBKbllnTmhBMGx2eFY1dm53cFRnQld6SkJDMm55ZVNxNFMr?= =?utf-8?B?dXc0SlFMRjRMbVVPQkpqU0svUUxUWVhxYm8wcVFYRCsyMlJXcnZWTk9aY2xH?= =?utf-8?B?eU85ZW1RTWlhV2REY0YrbE01TjFwbGRJNUJ4dGc1YkZDMHpnSjNlWFVJRytU?= =?utf-8?B?M01BK3ZtdmRSWUdGVmxrbFowVHJSdmlqeVU4d1hsY2xnVmtlMGI5b2NrTjA0?= =?utf-8?B?S3JHa0FBTEdDSXo1TWkyTW56QWVoeVpSZVplWUY2VXJ6VG42U05UeHJ6VFhF?= =?utf-8?B?UjhHVFVqdzMwaURhYkpiU3dNNlJOL1JCd01aTmtYNkJWbGpqOWVjZnFzaDky?= =?utf-8?B?dlAwN0xQTWRIWFhWY1hkaThaK08wRllya1dVcE1DbVRNaXpHbnJjVXREZ21p?= =?utf-8?B?Nzlkc1d5aUZxU29vMlp0anl5ODdnPT0=?= X-MS-Exchange-CrossTenant-Network-Message-Id: 15410c13-5176-4efc-a5bc-08d9f850bcfb X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5093.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 Feb 2022 11:19:50.5065 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: 0Jb8zHKZHWeMJh+Lg+/dGOt94PMnwRhwiSoj9h83vYvBE4u2S27dP4N1lcypD72lYmeQa9C4mf+iftPWk/RjjxG63i9VKRTLKXEjZ4a2Lm0= X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR1101MB2271 X-OriginatorOrg: intel.com 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 On 24-Feb-22 2:38 AM, Li, Miao wrote: > Hi, > >> -----Original Message----- >> From: Hunt, David >> Sent: Wednesday, February 23, 2022 12:32 AM >> To: Li, Miao ; dev@dpdk.org >> Cc: Wang, Yinan ; stephen@networkplumber.org >> Subject: Re: [PATCH v1] power: add wakeup log >> >> >> On 22/2/2022 1:52 PM, Miao Li wrote: >>> This patch adds a log in rte_power_monitor to show the core has been >>> waked up. >>> >>> Signed-off-by: Miao Li >>> --- >>> lib/eal/x86/rte_power_intrinsics.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/lib/eal/x86/rte_power_intrinsics.c >> b/lib/eal/x86/rte_power_intrinsics.c >>> index f749da9b85..dd63e2b6eb 100644 >>> --- a/lib/eal/x86/rte_power_intrinsics.c >>> +++ b/lib/eal/x86/rte_power_intrinsics.c >>> @@ -128,6 +128,14 @@ rte_power_monitor(const struct >> rte_power_monitor_cond *pmc, >>> : "D"(0), /* enter C0.2 */ >>> "a"(tsc_l), "d"(tsc_h)); >>> >>> + cur_value = __get_umwait_val(pmc->addr, pmc->size); >>> + >>> + /* check if core has been waked up by changing monitoring value */ >>> + if (pmc->fn(cur_value, pmc->opaque) != 0) >>> + RTE_LOG(INFO, EAL, >>> + "lcore %u is waked up from value change\n", >>> + rte_lcore_id()); >>> + >>> end: >>> /* erase sleep address */ >>> rte_spinlock_lock(&s->lock); >> >> >> Hi Li, >> >> If I'm not mistaken, a similar patch was added to a previous DPDK >> release and then removed because of the enormous performance impact. >> >> This looks to be something similar, and it's adding a log message to a >> low-level function. Also, as mentioned before, the intention in the >> future is to call this function much more agressively, so there would be >> hundreds of thousands of messages every second. >> >> We cannot add an RTE_LOG here. Please rework and put the log in the test >> case instead. > > I add a judgment before the output. When no packet arriver, the log will not be printed. When a lot of packets arriver, the rte_power_monitor will not be called. So I think the performance impact is small. > >> >> Also, regarding the wording, I would suggest  "lcore %u awoke due to >> monitor address value change\n" > > I will change the log content in next version. > >> >> Rgds, >> >> Dave. >> > > Thanks, > Miao > > Hi Miao, I have to agree with Dave here, I don't think this patch should be merged, as there are several problems with it. First of all, the result might be inaccurate in practice, because there is a race condition between reading value first and second time - UMONITOR protects us from that before we sleep, but not after. So, the information we get with `__get_umwait_val()` and calling a callback might be out of date by the time we reach that code. So, this code is *provably* vulnerable to race conditions. More importantly, I do not see how this is a useful addition to DPDK. I have to ask: what is the problem the patch is trying to solve? Because if the problem being solved is validating full path from l3fwd-power down to `rte_power_monitor`, then it could be solved in other ways that are more agreeable. For example, I believe we have logging inside l3fwd-power, so we can validate that it wakes up correctly. We *don't* have logging inside rte_power for these particular code paths, but IMO it would be unnecessary because l3fwd-power already effectively does that anyway. We *don't* have logging inside `rte_power_monitor`, but we can still validate it with unit tests, if the concern here is validating that the functionality works correctly. So, we can verify `rte_power_monitor` works with unit tests. We *could* introspect `rte_power` callback code with more logging if that's required (although IMO unnecessary, given that l3fwd-power already does that), and we can validate that l3fwd-power wakes up correctly with existing logging. So, with the addition of unit tests, the entire stack would be validated. Thus, to me, it seems like the patch is adding a (conditional) log message to a low level function, supported by an unnecessary and racy read/callback call for the sole purpose of checking information that does not get used anywhere else in the function other than in a log message, to solve a problem that could be solved in another way (with unit tests). Is there anything that I'm missing here? -- Thanks, Anatoly