From: "Zhang, Qi Z" <qi.z.zhang@intel.com>
To: Kevin Traynor <ktraynor@redhat.com>,
"Wu, Jingjing" <jingjing.wu@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, "stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] net/i40e: fix flow control watermark mismatch
Date: Sun, 13 Aug 2017 00:05:53 +0000 [thread overview]
Message-ID: <039ED4275CED7440929022BC67E70611530CE060@SHSMSX103.ccr.corp.intel.com> (raw)
In-Reply-To: <adbf5925-dba2-fef9-6aa0-4aec613a5c85@redhat.com>
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];
> >
> >
next prev parent reply other threads:[~2017-08-13 0:05 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-10 10:48 [dpdk-stable] " Qi Zhang
2017-08-10 15:15 ` [dpdk-stable] [dpdk-dev] " Kevin Traynor
2017-08-13 0:05 ` Zhang, Qi Z [this message]
2017-08-15 17:40 ` Kevin Traynor
2017-08-18 14:35 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=039ED4275CED7440929022BC67E70611530CE060@SHSMSX103.ccr.corp.intel.com \
--to=qi.z.zhang@intel.com \
--cc=dev@dpdk.org \
--cc=jingjing.wu@intel.com \
--cc=ktraynor@redhat.com \
--cc=stable@dpdk.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).