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 F1021A0A0C; Thu, 1 Jul 2021 11:01:23 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B7B4340141; Thu, 1 Jul 2021 11:01:23 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by mails.dpdk.org (Postfix) with ESMTP id 9BC2B40040 for ; Thu, 1 Jul 2021 11:01:21 +0200 (CEST) X-IronPort-AV: E=McAfee;i="6200,9189,10031"; a="188883568" X-IronPort-AV: E=Sophos;i="5.83,313,1616482800"; d="scan'208";a="188883568" Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga107.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jul 2021 02:01:20 -0700 X-IronPort-AV: E=Sophos;i="5.83,313,1616482800"; d="scan'208";a="409081195" Received: from dhunt5-mobl5.ger.corp.intel.com (HELO [10.252.2.63]) ([10.252.2.63]) by orsmga003-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 01 Jul 2021 02:01:19 -0700 From: David Hunt To: Anatoly Burakov , dev@dpdk.org Cc: konstantin.ananyev@intel.com, ciara.loftus@intel.com References: <8f5d030a77aa2f0e95e9680cb911b4e8db30c879.1624981670.git.anatoly.burakov@intel.com> <04557f14-b238-3daa-2753-b3437e59e640@intel.com> Message-ID: Date: Thu, 1 Jul 2021 10:01:17 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <04557f14-b238-3daa-2753-b3437e59e640@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [dpdk-dev] [PATCH v5 5/7] power: support callbacks for multiple Rx queues 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" On 30/6/2021 10:52 AM, David Hunt wrote: > Hi Anatoly, > > On 29/6/2021 4:48 PM, Anatoly Burakov wrote: >> Currently, there is a hard limitation on the PMD power management >> support that only allows it to support a single queue per lcore. This is >> not ideal as most DPDK use cases will poll multiple queues per core. >> >> The PMD power management mechanism relies on ethdev Rx callbacks, so it >> is very difficult to implement such support because callbacks are >> effectively stateless and have no visibility into what the other ethdev >> devices are doing. This places limitations on what we can do within the >> framework of Rx callbacks, but the basics of this implementation are as >> follows: >> >> - Replace per-queue structures with per-lcore ones, so that any device >>    polled from the same lcore can share data >> - Any queue that is going to be polled from a specific lcore has to be >>    added to the list of queues to poll, so that the callback is aware of >>    other queues being polled by the same lcore >> - Both the empty poll counter and the actual power saving mechanism is >>    shared between all queues polled on a particular lcore, and is only >>    activated when all queues in the list were polled and were determined >>    to have no traffic. >> - The limitation on UMWAIT-based polling is not removed because UMWAIT >>    is incapable of monitoring more than one address. >> >> Also, while we're at it, update and improve the docs. >> >> Signed-off-by: Anatoly Burakov >> --- >> >> Notes: >>      v5: >>      - Remove the "power save queue" API and replace it with >> mechanism suggested by >>        Konstantin >>           v3: >>      - Move the list of supported NICs to NIC feature table >>           v2: >>      - Use a TAILQ for queues instead of a static array >>      - Address feedback from Konstantin >>      - Add additional checks for stopped queues >> >>   doc/guides/nics/features.rst           |  10 + >>   doc/guides/prog_guide/power_man.rst    |  65 ++-- >>   doc/guides/rel_notes/release_21_08.rst |   3 + >>   lib/power/rte_power_pmd_mgmt.c         | 431 ++++++++++++++++++------- >>   4 files changed, 373 insertions(+), 136 deletions(-) >> > > --snip-- > >>   int >>   rte_power_ethdev_pmgmt_queue_enable(unsigned int lcore_id, uint16_t >> port_id, >>           uint16_t queue_id, enum rte_power_pmd_mgmt_type mode) >>   { >> -    struct pmd_queue_cfg *queue_cfg; >> +    const union queue qdata = {.portid = port_id, .qid = queue_id}; >> +    struct pmd_core_cfg *lcore_cfg; >> +    struct queue_list_entry *queue_cfg; >>       struct rte_eth_dev_info info; >>       rte_rx_callback_fn clb; >>       int ret; >> @@ -202,9 +401,19 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned int >> lcore_id, uint16_t port_id, >>           goto end; >>       } >>   -    queue_cfg = &port_cfg[port_id][queue_id]; >> +    lcore_cfg = &lcore_cfgs[lcore_id]; >>   -    if (queue_cfg->pwr_mgmt_state != PMD_MGMT_DISABLED) { >> +    /* check if other queues are stopped as well */ >> +    ret = cfg_queues_stopped(lcore_cfg); >> +    if (ret != 1) { >> +        /* error means invalid queue, 0 means queue wasn't stopped */ >> +        ret = ret < 0 ? -EINVAL : -EBUSY; >> +        goto end; >> +    } >> + >> +    /* if callback was already enabled, check current callback type */ >> +    if (lcore_cfg->pwr_mgmt_state != PMD_MGMT_DISABLED && >> +            lcore_cfg->cb_mode != mode) { >>           ret = -EINVAL; >>           goto end; >>       } >> @@ -214,53 +423,20 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned >> int lcore_id, uint16_t port_id, >>         switch (mode) { >>       case RTE_POWER_MGMT_TYPE_MONITOR: >> -    { >> -        struct rte_power_monitor_cond dummy; >> - >> -        /* check if rte_power_monitor is supported */ >> -        if (!global_data.intrinsics_support.power_monitor) { >> -            RTE_LOG(DEBUG, POWER, "Monitoring intrinsics are not >> supported\n"); >> -            ret = -ENOTSUP; >> +        /* check if we can add a new queue */ >> +        ret = check_monitor(lcore_cfg, &qdata); >> +        if (ret < 0) >>               goto end; >> -        } >>   -        /* check if the device supports the necessary PMD API */ >> -        if (rte_eth_get_monitor_addr(port_id, queue_id, >> -                &dummy) == -ENOTSUP) { >> -            RTE_LOG(DEBUG, POWER, "The device does not support >> rte_eth_get_monitor_addr\n"); >> -            ret = -ENOTSUP; >> -            goto end; >> -        } >>           clb = clb_umwait; >>           break; >> -    } >>       case RTE_POWER_MGMT_TYPE_SCALE: >> -    { >> -        enum power_management_env env; >> -        /* only PSTATE and ACPI modes are supported */ >> -        if (!rte_power_check_env_supported(PM_ENV_ACPI_CPUFREQ) && >> -                !rte_power_check_env_supported( >> -                    PM_ENV_PSTATE_CPUFREQ)) { >> -            RTE_LOG(DEBUG, POWER, "Neither ACPI nor PSTATE modes are >> supported\n"); >> -            ret = -ENOTSUP; >> +        /* check if we can add a new queue */ >> +        ret = check_scale(lcore_id); >> +        if (ret < 0) >>               goto end; >> -        } >> -        /* ensure we could initialize the power library */ >> -        if (rte_power_init(lcore_id)) { >> -            ret = -EINVAL; >> -            goto end; >> -        } >> -        /* ensure we initialized the correct env */ >> -        env = rte_power_get_env(); >> -        if (env != PM_ENV_ACPI_CPUFREQ && >> -                env != PM_ENV_PSTATE_CPUFREQ) { >> -            RTE_LOG(DEBUG, POWER, "Neither ACPI nor PSTATE modes >> were initialized\n"); >> -            ret = -ENOTSUP; >> -            goto end; >> -        } >>           clb = clb_scale_freq; >>           break; >> -    } >>       case RTE_POWER_MGMT_TYPE_PAUSE: >>           /* figure out various time-to-tsc conversions */ >>           if (global_data.tsc_per_us == 0) >> @@ -273,13 +449,23 @@ rte_power_ethdev_pmgmt_queue_enable(unsigned >> int lcore_id, uint16_t port_id, >>           ret = -EINVAL; >>           goto end; >>       } >> +    /* add this queue to the list */ >> +    ret = queue_list_add(lcore_cfg, &qdata); >> +    if (ret < 0) { >> +        RTE_LOG(DEBUG, POWER, "Failed to add queue to list: %s\n", >> +                strerror(-ret)); >> +        goto end; >> +    } >> +    /* new queue is always added last */ >> +    queue_cfg = TAILQ_LAST(&lcore_cfgs->head, queue_list_head); > > > Need to ensure that queue_cfg gets set here, otherwise we'll get a > segfault below. > Or, looking at this again, shouldn't "lcore_cfgs" be "lcore_cfg"? > > >>         /* 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, NULL); >> +    if (lcore_cfg->n_queues == 1) { >> +        lcore_cfg->cb_mode = mode; >> +        lcore_cfg->pwr_mgmt_state = PMD_MGMT_ENABLED; >> +    } >> +    queue_cfg->cb = rte_eth_add_rx_callback(port_id, queue_id, >> +            clb, queue_cfg); > --snip-- >