From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 02ADFA04DB; Thu, 15 Oct 2020 12:32:04 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id CBDD31BF0B; Thu, 15 Oct 2020 12:32:02 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by dpdk.org (Postfix) with ESMTP id 3F3BC1BEE0 for ; Thu, 15 Oct 2020 12:32:00 +0200 (CEST) IronPort-SDR: xnAXpw0u30Jpmuer3z7ijR8MDhLQ7l+LIAYz2dpRo11oJmAo+6NetSx1mTE9gHRl0nbk9ZXr3g nN/nw9B377Mw== X-IronPort-AV: E=McAfee;i="6000,8403,9774"; a="145627467" X-IronPort-AV: E=Sophos;i="5.77,378,1596524400"; d="scan'208";a="145627467" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2020 03:31:58 -0700 IronPort-SDR: nDmzN/l+uhM5+vDytZ7PmZUuDcacpoYWZlpBXSX4s0iwGacXC4LBswdkV7iBR54n/UvIU5an4s 4HnE/5oTIQcQ== X-IronPort-AV: E=Sophos;i="5.77,378,1596524400"; d="scan'208";a="521792296" Received: from orozen-mobl.ger.corp.intel.com (HELO [10.213.243.220]) ([10.213.243.220]) by fmsmga005-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Oct 2020 03:31:56 -0700 To: "Ananyev, Konstantin" , "dev@dpdk.org" Cc: "Ma, Liang J" , "Hunt, David" , Ray Kinsella , Neil Horman , "jerinjacobk@gmail.com" , "Richardson, Bruce" , "thomas@monjalon.net" , "McDaniel, Timothy" , "Eads, Gage" , "Macnamara, Chris" References: <532f45c5d79b4c30a919553d322bb66e91534466.1602258833.git.anatoly.burakov@intel.com> From: "Burakov, Anatoly" Message-ID: <30a97833-d556-26bc-f814-69198280c05d@intel.com> Date: Thu, 15 Oct 2020 11:31:54 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v6 05/10] power: add PMD power management API and callback X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 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" On 14-Oct-20 7:41 PM, Ananyev, Konstantin wrote: >> From: Liang Ma >> >> Add a simple on/off switch that will enable saving power when no >> packets are arriving. It is based on counting the number of empty >> polls and, when the number reaches a certain threshold, entering an >> architecture-defined optimized power state that will either wait >> until a TSC timestamp expires, or when packets arrive. >> >> This API mandates a core-to-single-queue mapping (that is, multiple >> queued per device are supported, but they have to be polled on different >> cores). >> >> This design is using PMD RX callbacks. >> >> 1. UMWAIT/UMONITOR: >> >> When a certain threshold of empty polls is reached, the core will go >> into a power optimized sleep while waiting on an address of next RX >> descriptor to be written to. >> >> 2. Pause instruction >> >> Instead of move the core into deeper C state, this method uses the >> pause instruction to avoid busy polling. >> >> 3. Frequency scaling >> Reuse existing DPDK power library to scale up/down core frequency >> depending on traffic volume. >> >> Signed-off-by: Liang Ma >> Signed-off-by: Anatoly Burakov >> --- >> >> Notes: >> v6: >> - Added wakeup mechanism for UMWAIT >> - Removed memory allocation (everything is now allocated statically) >> - Fixed various typos and comments >> - Check for invalid queue ID >> - Moved release notes to this patch >> >> v5: >> - Make error checking more robust >> - Prevent initializing scaling if ACPI or PSTATE env wasn't set >> - Prevent initializing UMWAIT path if PMD doesn't support get_wake_addr >> - Add some debug logging >> - Replace x86-specific code path to generic path using the intrinsic check >> > > > I think you need to check state here, and _disable() have to set state with lock grabbed. > Otherwise this lock wouldn't protect you from race conditions. > As an example: > > CP@T0: > rte_spinlock_lock(&queue_cfg->umwait_lock); > if (queue_cfg->wait_addr != NULL) //wait_addr == NULL, fallthrough > rte_spinlock_unlock(&queue_cfg->umwait_lock); > > DP@T1: > rte_spinlock_lock(&queue_cfg->umwait_lock); > queue_cfg->wait_addr = target_addr; > monitor_sync(...); // DP was put to sleep > > CP@T2: > queue_cfg->cur_cb = NULL; > queue_cfg->pwr_mgmt_state = PMD_MGMT_DISABLED; > ret = 0; > > rte_power_pmd_mgmt_queue_disable() finished with success, > but DP core wasn't wokenup. > > To be more specific: > clb_umwait(...) { > ... > lock(&qcfg->lck); > if (qcfg->state == ENABLED) { > qcfg->wake_addr = addr; > monitor_sync(addr, ...,&qcfg->lck); > } > unlock(&qcfg->lck); > ... > } > > _disable(...) { > ... > lock(&qcfg->lck); > qcfg->state = DISABLED; > if (qcfg->wake_addr != NULL) > monitor_wakeup(qcfg->wake_addr); > unlock(&qcfg->lock); > ... > } True, didn't think of that. Will fix. >> + >> +if (!i.power_monitor) { >> +RTE_LOG(DEBUG, POWER, "Monitoring intrinsics are not supported\n"); >> +ret = -ENOTSUP; >> +goto end; >> +} >> + >> +/* check if the device supports the necessary PMD API */ >> +if (rte_eth_get_wake_addr(port_id, queue_id, >> +&dummy_addr, &dummy_expected, >> +&dummy_mask, &dummy_sz) == -ENOTSUP) { >> +RTE_LOG(DEBUG, POWER, "The device does not support rte_eth_rxq_ring_addr_get\n"); >> +ret = -ENOTSUP; >> +goto end; >> +} >> +/* initialize UMWAIT spinlock */ >> +rte_spinlock_init(&queue_cfg->umwait_lock); > > I think don't need to do that. > It supposed to be in valid state (otherwise you are probably in trouble anyway). This is mostly for initialization, for when we first run the callback. I suppose we could do it in an RTE_INIT() function or just leave it be since the spinlocks are part of a statically allocated structure and will default to 0 anyway (although it wouldn't be proper usage of the API as that would be relying on implementation detail). > >> + >> +/* initialize data before enabling the callback */ >> +queue_cfg->empty_poll_stats = 0; >> +queue_cfg->cb_mode = mode; >> +queue_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED; >> + >> +queue_cfg->cur_cb = rte_eth_add_rx_callback(port_id, queue_id, >> +clb_umwait, NULL); > > Would be a bit cleaner/nicer to move add_rx_callback out of switch() {} > As you have to do it always anyway. > Same thought for disable() and remove_rx_callback(). The functions are different for each, so we can't move them out of switch (unless you're suggesting to unify the callback to handle all three modes?). -- Thanks, Anatoly