From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wi0-f171.google.com (mail-wi0-f171.google.com [209.85.212.171]) by dpdk.org (Postfix) with ESMTP id 76F2F2A9 for ; Tue, 6 Jan 2015 20:56:09 +0100 (CET) Received: by mail-wi0-f171.google.com with SMTP id bs8so6079491wib.4 for ; Tue, 06 Jan 2015 11:56:09 -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=Rko5FNzCIPSzcWashs0X4NCRiHvxKjNJTHJ37tPq5vM=; b=blvCrhrVEaDmRhGT2ZyhmJOa3ry0/RXSIBkOQRRnSOv8VUxI4CrvDOUiSx8neZf96G cSJHw2+Vv0lmRk1ZaY1NIPRD0JHDlMupHBWfTwMgOkugGVnCArQUb8vLwpACA4f1p3c+ TZ4stbt4He4ZF6g2myM+PSpJd9zhEpTzQuwIojh9pBCn6EIAWBQkWMYn09/3bSRNDmi+ Lqab2zAtwgs0TD25ocGpD0C/4CAjUniXXYDq8E+1tzQyF5jRCrnV4UGiCrDOpvw9f8aa sNnnwesIirN8yBhyIvyvofOcsjV5SOxMRGoNuee+vNuolBE5nndQYh38qp5pCnCpkNoj 0ZKw== X-Gm-Message-State: ALoCoQlLz4nuQhy+H2LkxftXrjGIkr6qx+5xjRv9pqk9a22DPWRp2fMeEs9HFLW7Md8Ir12h+7l8 X-Received: by 10.194.57.84 with SMTP id g20mr199278217wjq.122.1420574169249; Tue, 06 Jan 2015 11:56:09 -0800 (PST) Received: from [10.0.0.3] (bzq-79-179-0-199.red.bezeqint.net. [79.179.0.199]) by mx.google.com with ESMTPSA id x16sm15156673wia.15.2015.01.06.11.56.08 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 06 Jan 2015 11:56:08 -0800 (PST) Message-ID: <54AC3DD7.60604@cloudius-systems.com> Date: Tue, 06 Jan 2015 21:56:07 +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> <54AA62F7.6080108@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: Tue, 06 Jan 2015 19:56:09 -0000 On 01/06/15 03:56, Ouyang, Changchun wrote: >> -----Original Message----- >> From: Vlad Zolotarov [mailto:vladz@cloudius-systems.com] >> Sent: Monday, January 5, 2015 6:10 PM >> To: Ouyang, Changchun;dev@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH v4 4/6] ether: Check VMDq RSS mode >> >> >> 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. > Then what's your suggestion for the better log message? I can consider refine it if you have better one. Just like I've suggested before - u may break with appropriate error message right when u see the problem (in a "else" clause). This way the code will be both more readable and more robust and won't break if anybody decides to change the not-RSS-specific logic u r relying on. >>>>> Thanks >>>>> Changchun >>>>> >>>>>