From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 6FF0E567E; Tue, 15 Aug 2017 19:40:20 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 5C1947C846; Tue, 15 Aug 2017 17:40:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 5C1947C846 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=ktraynor@redhat.com Received: from ktraynor.remote.csb (ovpn-116-250.ams2.redhat.com [10.36.116.250]) by smtp.corp.redhat.com (Postfix) with ESMTP id 30C0C5C66F; Tue, 15 Aug 2017 17:40:16 +0000 (UTC) To: "Zhang, Qi Z" , "Wu, Jingjing" References: <20170810104807.59436-1-qi.z.zhang@intel.com> <039ED4275CED7440929022BC67E70611530CE060@SHSMSX103.ccr.corp.intel.com> Cc: "dev@dpdk.org" , "stable@dpdk.org" From: Kevin Traynor Organization: Red Hat Message-ID: <94a7b3a7-a3d8-6d67-52ab-2c3595e4b302@redhat.com> Date: Tue, 15 Aug 2017 18:40:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <039ED4275CED7440929022BC67E70611530CE060@SHSMSX103.ccr.corp.intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 15 Aug 2017 17:40:19 +0000 (UTC) Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix flow control watermark mismatch X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 15 Aug 2017 17:40:20 -0000 On 08/13/2017 01:05 AM, Zhang, Qi Z wrote: > Hi Kevin: > >> -----Original Message----- >> From: Kevin Traynor [mailto:ktraynor@redhat.com] >> Sent: Thursday, August 10, 2017 11:16 PM >> To: Zhang, Qi Z ; Wu, Jingjing >> Cc: dev@dpdk.org; stable@dpdk.org >> Subject: Re: [dpdk-dev] [PATCH] net/i40e: fix flow control watermark >> mismatch >> >> On 08/10/2017 11:48 AM, Qi Zhang wrote: >>> Flow control watermark is not read out correctly, that may cause an >>> application who not intend to change watermark but does change it with >>> a rte_eth_dev_flow_ctrl_set call right after >>> rte_eth_dev_flow_ctrl_get. >>> >>> Fixes: f53577f06925 ("i40e: support flow control") >>> Cc: stable@dpdk.org >>> >>> Signed-off-by: Qi Zhang >>> --- >>> drivers/net/i40e/i40e_ethdev.c | 19 +++++++++++++------ >>> 1 file changed, 13 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/i40e/i40e_ethdev.c >>> b/drivers/net/i40e/i40e_ethdev.c index 71cb7d3..8b008c4 100644 >>> --- a/drivers/net/i40e/i40e_ethdev.c >>> +++ b/drivers/net/i40e/i40e_ethdev.c >>> @@ -86,12 +86,6 @@ >>> /* Flow control default timer */ >>> #define I40E_DEFAULT_PAUSE_TIME 0xFFFFU >>> >>> -/* Flow control default high water */ -#define >>> I40E_DEFAULT_HIGH_WATER (0x1C40/1024) >>> - >>> -/* Flow control default low water */ >>> -#define I40E_DEFAULT_LOW_WATER (0x1A40/1024) >>> - >>> /* Flow control enable fwd bit */ >>> #define I40E_PRTMAC_FWD_CTRL 0x00000001 >>> >>> @@ -101,6 +95,12 @@ >>> /* Kilobytes shift */ >>> #define I40E_KILOSHIFT 10 >>> >>> +/* Flow control default high water */ #define >> I40E_DEFAULT_HIGH_WATER >>> +(0xF2000 >> I40E_KILOSHIFT) >>> + >>> +/* Flow control default low water */ >>> +#define I40E_DEFAULT_LOW_WATER (0xF2000 >> I40E_KILOSHIFT) >>> + >> >> I'm not sure these are needed anymore. Would seem if you want a correct >> value prior to flow_ctl_get() being called, the registers should also be read >> during init. Is there a need for the PMD to have the correct value prior to >> flow_ctrl_get() ? I didn't see one. > > > You are right, the value here does not take effect based on current implementation, I just correct it to align with datasheet. > I think it's not necessary to remove it here, it may be used in future. > > The idea fix is: during init, the watermark is set with default value, so it is not necessary to read out from hw register during flow_ctl_get, > But due to I40E_GLRPB_GHW limitation, it is shared by different ports on the same device, it is possible the value is changed on another port, but local variable not sync, so we have to read out register every flow_ctl_get. > I agree it's not worth to read the registers at init if the real values are not needed. As long as the registers are read before the values are used for anything, which you've confirmed, then I'm ok with whatever default you want to init fc_conf to. This is needed for LTS as OVS reads and writes fc with the intention of not changing the watermarks, but does change them due to the bug that is fixed here. I've tested with DPDK 16.11. Acked-by: Kevin Traynor > Regards > Qi > >> >>> /* Receive Average Packet Size in Byte*/ #define >>> I40E_PACKET_AVERAGE_SIZE 128 >>> >>> @@ -3199,6 +3199,13 @@ i40e_flow_ctrl_get(struct rte_eth_dev *dev, >> struct rte_eth_fc_conf *fc_conf) >>> struct i40e_pf *pf = >> I40E_DEV_PRIVATE_TO_PF(dev->data->dev_private); >>> >>> fc_conf->pause_time = pf->fc_conf.pause_time; >>> + >>> + /* read out from register, in case they are modified by other port */ >>> + pf->fc_conf.high_water[I40E_MAX_TRAFFIC_CLASS] = >>> + I40E_READ_REG(hw, I40E_GLRPB_GHW) >> I40E_KILOSHIFT; >>> + pf->fc_conf.low_water[I40E_MAX_TRAFFIC_CLASS] = >>> + I40E_READ_REG(hw, I40E_GLRPB_GLW) >> I40E_KILOSHIFT; >>> + >>> fc_conf->high_water = >> pf->fc_conf.high_water[I40E_MAX_TRAFFIC_CLASS]; >>> fc_conf->low_water = >> pf->fc_conf.low_water[I40E_MAX_TRAFFIC_CLASS]; >>> >>> >