diff mbox

[dpdk-dev] net/i40e: fix flow control watermark mismatch

Message ID 20170810104807.59436-1-qi.z.zhang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Qi Zhang Aug. 10, 2017, 10:48 a.m. UTC 
  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 <qi.z.zhang@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

Comments

Kevin Traynor Aug. 10, 2017, 3:15 p.m. UTC | #1 
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 <qi.z.zhang@intel.com>
> ---
>  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.

>  /* 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];
>  
>
Qi Zhang Aug. 13, 2017, 12:05 a.m. UTC | #2 
Hi Kevin:

> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Thursday, August 10, 2017 11:16 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
> 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 <qi.z.zhang@intel.com>
> > ---
> >  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.

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];
> >
> >
Kevin Traynor Aug. 15, 2017, 5:40 p.m. UTC | #3 
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 <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>> 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 <qi.z.zhang@intel.com>
>>> ---
>>>  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 <ktraynor@redhat.com>

> 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];
>>>
>>>
>
Ferruh Yigit Aug. 18, 2017, 2:35 p.m. UTC | #4 
On 8/15/2017 6:40 PM, Kevin Traynor wrote:
> 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 <qi.z.zhang@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>
>>> 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 <qi.z.zhang@intel.com>

> Acked-by: Kevin Traynor <ktraynor@redhat.com>

Applied to dpdk-next-net/master, thanks.

(Updated commit log with the information provided in the mail.)
diff mbox

Patch

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)
+
 /* 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];