From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-we0-f178.google.com (mail-we0-f178.google.com [74.125.82.178]) by dpdk.org (Postfix) with ESMTP id E7B0B5A3E for ; Mon, 5 Jan 2015 11:10:01 +0100 (CET) Received: by mail-we0-f178.google.com with SMTP id p10so7569638wes.37 for ; Mon, 05 Jan 2015 02:10:01 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:date:from:user-agent:mime-version:to :subject:references:in-reply-to:content-type :content-transfer-encoding; bh=EAqLludrlavuQy+Ko07crPVJT3HJZf7ecLQkAvycLts=; b=he4+d9vbidQTS7OOJ21MJFKoZY5Av0y+pdgu6T64PS4j7g9HxK4AZ0o9KThy5NTH4q qDHspjrPB5LL0wZD6fHKElavtUcuqjjHnzHu3JDpUV98OJKALNdye+61V4/d82vHq3AQ gwDUGVar3SsQO1w1oqv4Xl0kJkMt9WxW6I5eOYamOSpmdn1qnxxFfosRWGo1f5OdTksa AO+I7R7Zd4ib4qbXyM5u1s1ax0m4/Ryok+MIHyIVhH4zbcZ2D8/0eoV3exjZX1njJ5xh I5YgOIJO+oDetioXjQpiulY+xaMqHR2n1wEpdlOIRIVAKrx2MY1wjhzkjGVe6RPZ2Pus gAVA== X-Gm-Message-State: ALoCoQl+Mb+VmzYeSex7O62XQYI+EClOqo1RUUnPNCgx4aTcQQiWPpyKMhbsMaWg5nnaDHsaVDrM X-Received: by 10.194.110.233 with SMTP id id9mr39459480wjb.22.1420452601780; Mon, 05 Jan 2015 02:10:01 -0800 (PST) Received: from [10.0.0.165] (system.cloudius-systems.com. [84.94.198.183]) by mx.google.com with ESMTPSA id a1sm25935790wjx.28.2015.01.05.02.10.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 05 Jan 2015 02:10:01 -0800 (PST) Message-ID: <54AA62F7.6080108@cloudius-systems.com> Date: Mon, 05 Jan 2015 12:09:59 +0200 From: Vlad Zolotarov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: "Ouyang, Changchun" , "dev@dpdk.org" References: <1419398584-19520-1-git-send-email-changchun.ouyang@intel.com> <1420355937-18484-1-git-send-email-changchun.ouyang@intel.com> <1420355937-18484-5-git-send-email-changchun.ouyang@intel.com> <54A8FD9A.8040701@cloudius-systems.com> <54A90BC8.8020307@cloudius-systems.com> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 05 Jan 2015 10:10:02 -0000 On 01/05/15 03:00, Ouyang, Changchun wrote: > >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Sunday, January 4, 2015 5:46 PM >> To: Ouyang, Changchun; dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode >> >> >> On 01/04/15 10:58, Ouyang, Changchun wrote: >>>> -----Original Message----- >>>> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >>>> Sent: Sunday, January 4, 2015 4:45 PM >>>> To: Ouyang, Changchun; dev@dpdk.org >>>> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode >>>> >>>> >>>> On 01/04/15 09:18, Ouyang Changchun wrote: >>>>> Check mq mode for VMDq RSS, handle it correctly instead of returning >>>>> an error; Also remove the limitation of per pool queue number has >>>>> max value of 1, because the per pool queue number could be 2 or 4 if >>>>> it is VMDq RSS mode; >>>>> >>>>> The number of rxq specified in config will determine the mq mode for >>>> VMDq RSS. >>>>> Signed-off-by: Changchun Ouyang >>>>> --- >>>>> lib/librte_ether/rte_ethdev.c | 39 >>>> ++++++++++++++++++++++++++++++++++----- >>>>> 1 file changed, 34 insertions(+), 5 deletions(-) >>>>> >>>>> diff --git a/lib/librte_ether/rte_ethdev.c >>>>> b/lib/librte_ether/rte_ethdev.c index 95f2ceb..59ff325 100644 >>>>> --- a/lib/librte_ether/rte_ethdev.c >>>>> +++ b/lib/librte_ether/rte_ethdev.c >>>>> @@ -510,8 +510,7 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, >>>>> uint16_t nb_rx_q, uint16_t nb_tx_q, >>>>> >>>>> if (RTE_ETH_DEV_SRIOV(dev).active != 0) { >>>>> /* check multi-queue mode */ >>>>> - if ((dev_conf->rxmode.mq_mode == ETH_MQ_RX_RSS) || >>>>> - (dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB) || >>>>> + if ((dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB) || >>>>> (dev_conf->rxmode.mq_mode == ETH_MQ_RX_DCB_RSS) >>>> || >>>>> (dev_conf->txmode.mq_mode == ETH_MQ_TX_DCB)) { >>>>> /* SRIOV only works in VMDq enable mode */ @@ - >>>> 525,7 +524,6 @@ >>>>> rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t nb_rx_q, >>>> uint16_t nb_tx_q, >>>>> } >>>>> >>>>> switch (dev_conf->rxmode.mq_mode) { >>>>> - case ETH_MQ_RX_VMDQ_RSS: >>>>> case ETH_MQ_RX_VMDQ_DCB: >>>>> case ETH_MQ_RX_VMDQ_DCB_RSS: >>>>> /* DCB/RSS VMDQ in SRIOV mode, not implement >>>> yet */ @@ -534,6 >>>>> +532,39 @@ rte_eth_dev_check_mq_mode(uint8_t port_id, uint16_t >>>> nb_rx_q, uint16_t nb_tx_q, >>>>> "unsupported VMDQ mq_mode >>>> rx %u\n", >>>>> port_id, dev_conf- >>>>> rxmode.mq_mode); >>>>> return (-EINVAL); >>>>> + case ETH_MQ_RX_RSS: >>>>> + PMD_DEBUG_TRACE("ethdev port_id=%" PRIu8 >>>>> + " SRIOV active, " >>>>> + "Rx mq mode is changed from:" >>>>> + "mq_mode %u into VMDQ >>>> mq_mode %u\n", >>>>> + port_id, >>>>> + dev_conf->rxmode.mq_mode, >>>>> + dev->data- >>>>> dev_conf.rxmode.mq_mode); >>>>> + case ETH_MQ_RX_VMDQ_RSS: >>>>> + dev->data->dev_conf.rxmode.mq_mode = >>>> ETH_MQ_RX_VMDQ_RSS; >>>>> + if (nb_rx_q < >>>> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool) { >> Missed that before: shouldn't it be "<=" here? > Agree with you, need <= here, I will fix it in v5 > >>>>> + switch (nb_rx_q) { >>>>> + case 1: >>>>> + case 2: >>>>> + RTE_ETH_DEV_SRIOV(dev).active = >>>>> + ETH_64_POOLS; >>>>> + break; >>>>> + case 4: >>>>> + RTE_ETH_DEV_SRIOV(dev).active = >>>>> + ETH_32_POOLS; >>>>> + break; >>>>> + default: >>>>> + PMD_DEBUG_TRACE("ethdev >>>> port_id=%d" >>>>> + " SRIOV active, " >>>>> + "queue number invalid\n", >>>>> + port_id); >>>>> + return -EINVAL; >>>>> + } >>>>> + RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool = >>>> nb_rx_q; >>>>> + RTE_ETH_DEV_SRIOV(dev).def_pool_q_idx = >>>>> + dev->pci_dev->max_vfs * nb_rx_q; >>>>> + } >>>> Don't u need to return an error in the "else" here? >>> Actually it has such a check after these code snippet, and it does >>> return error for the else case, Because it is original logic, I don't change any >> code around it, so it doesn't display here, you can check the codes. >> >> I see. The flow is a bit confusing since the switch-case above will end up >> executing a "default" clause which will set >> RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 and then the error message >> in the check u are referring will be a bit confusing. > ' set RTE_ETH_DEV_SRIOV(dev).nb_q_per_pool to 1 ' is original code, which is for vmdq only case, or single queue case. > It is in default clause, and not in VMDQ_RSS clause. > I think my new code is ok here. The original code is ok and your current code will work. The only problem with your new code is that in case on an error like I've described above the error message will be confusing. > >>> Thanks >>> Changchun >>> >>>