Hi
I'm trying to set the I40E_PRTQF_CTL_0_HSYM_ENA_MASK flag and the doesn't
seem to matter.
I'm setting and testing the flag for each port in i40e_pf_config_rss().
reg_val = i40e_read_rx_ctl(hw, I40E_PRTQF_CTL_0);
printk("tala: I40E_PRTQF_CTL_0_HSYM_ENA_MASK set ? %d\n",
(reg_val & I40E_PRTQF_CTL_0_HSYM_ENA_MASK));
reg_val |= I40E_PRTQF_CTL_0_HSYM_ENA_MASK;
i40e_write_rx_ctl(hw, I40E_PRTQF_CTL_0, reg_val);
reg_val = i40e_read_rx_ctl(hw, I40E_PRTQF_CTL_0);
printk("tala: I40E_PRTQF_CTL_0_HSYM_ENA_MASK set ? %d\n",
(reg_val & I40E_PRTQF_CTL_0_HSYM_ENA_MASK));
According to the dmesg output, the flag is set.
I'm running bi-directional traffic that is supposed to be hashed
symmetrically. I have 8 RX queues configured.
eth2-01 2.2.2.10:1025 - > 1.1.1.15:1024
eth2-02 1.1.1.15:1024 -> 2.2.2.10:1025
According to the ethtool -S:
# ethtool -S eth2-01 | grep rx.*pa
rx_packets: 238
rx-0.rx_packets: 0
rx-1.rx_packets: 0
rx-2.rx_packets: 0
rx-3.rx_packets: 238
rx-4.rx_packets: 0
rx-5.rx_packets: 0
rx-6.rx_packets: 0
rx-7.rx_packets: 0
ethtool -S eth2-02 | grep rx.*pa
rx_packets: 827
rx-0.rx_packets: 13
rx-1.rx_packets: 0
rx-2.rx_packets: 0
rx-3.rx_packets: 0
rx-4.rx_packets: 0
rx-5.rx_packets: 814
rx-6.rx_packets: 0
rx-7.rx_packets: 0
Driver version is 1.4.25
I'm running an old kernel and ethtool versions so I can't
change rx-flow-hash on the fly..
Am i missing anything ?
Thanks !
Tal.
On Mon, Jul 4, 2016 at 11:17 AM, Victor Julien <li...@inliniac.net> wrote:
> On 03-07-16 19:31, Alexander Duyck wrote:
> > On Sun, Jul 3, 2016 at 1:21 AM, <li...@inliniac.net> wrote:
> >> From: Victor Julien <vic...@inliniac.net>
> >>
> >> X(L)710 supports symmetric RSS hashing. See "7.1.9.3 Symmetric hash" in
> >> the "Intel® Ethernet Controller XL710 Datasheet"
> >>
> >> Add support for this option as a priv flag in ethtool.
> >
> > Is this something that justifies a private flag or should we look at
> > updating the ethtool Rx flow hash commands so that we can set/get this
> > configuration there?
> >
>
> [...]
>
> >> rss-symmetric : on
> >
> > Yeah, looking this over I don't see this being something that will
> > only be implemented for i40e so we may want to make this device
> > agnostic instead of using a private flag.
>
> This was my first thought as well, but then I realized updating ethtool
> means getting code accepted in other places as well:)
>
> Anyway, will send an attempt at doing this shortly.
>
>
> >> Signed-off-by: Victor Julien <vic...@inliniac.net>
> >> ---
> >> src/i40e.h | 4 +++-
> >> src/i40e_common.c | 1 -
> >> src/i40e_ethtool.c | 13 ++++++++++++-
> >> src/i40e_main.c | 15 +++++++++++++++
> >> 4 files changed, 30 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/i40e.h b/src/i40e.h
> >> index 7742633..167febe 100644
> >> --- a/src/i40e.h
> >> +++ b/src/i40e.h
> >> @@ -113,7 +113,8 @@
> >> #define I40E_PRIV_FLAGS_FD_ATR BIT(2)
> >> #define I40E_PRIV_FLAGS_VEB_STATS BIT(3)
> >> #define I40E_PRIV_FLAGS_HW_ATR_EVICT BIT(4)
> >> -#define I40E_PRIV_FLAGS_TRUE_PROMISC_SUPPORT BIT(5)
> >> +#define I40E_PRIV_FLAGS_RSS_HSYM BIT(5)
> >> +#define I40E_PRIV_FLAGS_TRUE_PROMISC_SUPPORT BIT(6)
> >> #endif
> >>
> >> #define I40E_NVM_VERSION_LO_SHIFT 0
> >> @@ -484,6 +485,7 @@ struct i40e_pf {
> >> #define I40E_FLAG_HAVE_10GBASET_PHY BIT_ULL(48)
> >> #define I40E_FLAG_MPLS_HDR_OFFLOAD_CAPABLE BIT_ULL(49)
> >> #define I40E_FLAG_TRUE_PROMISC_SUPPORT BIT_ULL(50)
> >> +#define I40E_FLAG_RSS_HSYM BIT_ULL(51)
> >>
> >> /* tracks features that get auto disabled by errors */
> >> u64 auto_disable_flags;
> >
> > So you shifted the TRUE_PROMISC feature in the private flags and then
> > placed your feature flag after the TRUE_PROMISC in the next block. It
> > might make more sense to have these ordered the same way in both
> > updates.
>
> Good point.
>
>
> >> diff --git a/src/i40e_common.c b/src/i40e_common.c
> >> index c0b3dcc..9bf9336 100644
> >> --- a/src/i40e_common.c
> >> +++ b/src/i40e_common.c
> >> @@ -5248,7 +5248,6 @@ i40e_status i40e_set_filter_control(struct
> i40e_hw *hw,
> >>
> >> /* Read the PF Queue Filter control register */
> >> val = i40e_read_rx_ctl(hw, I40E_PFQF_CTL_0);
> >> -
> >> /* Program required PE hash buckets for the PF */
> >> val &= ~I40E_PFQF_CTL_0_PEHSIZE_MASK;
> >> val |= ((u32)settings->pe_filt_num <<
> I40E_PFQF_CTL_0_PEHSIZE_SHIFT) &
> >
> > Don't mess with white space unless you are actually changing code in
> > that region. This is just noise and doesn't help the patch at all.
>
> This was an oversight, apologies.
>
>
> >> diff --git a/src/i40e_ethtool.c b/src/i40e_ethtool.c
> >> index 1456ac5..b9f98e4 100644
> >> --- a/src/i40e_ethtool.c
> >> +++ b/src/i40e_ethtool.c
> >> @@ -268,6 +268,7 @@ static const char
> i40e_priv_flags_strings_gl[][ETH_GSTRING_LEN] = {
> >> "flow-director-atr",
> >> "veb-stats",
> >> "hw-atr-eviction",
> >> + "rss-symmetric",
> >> "vf-true-promisc-support",
> >> };
> >>
> >> @@ -279,6 +280,7 @@ static const char
> i40e_priv_flags_strings[][ETH_GSTRING_LEN] = {
> >> "flow-director-atr",
> >> "veb-stats",
> >> "hw-atr-eviction",
> >> + "rss-symmetric",
> >> };
> >>
> >> #define I40E_PRIV_FLAGS_STR_LEN ARRAY_SIZE(i40e_priv_flags_strings)
> >> @@ -4373,7 +4375,8 @@ static u32 i40e_get_priv_flags(struct net_device
> *dev)
> >> ret_flags |= pf->flags & I40E_FLAG_TRUE_PROMISC_SUPPORT
> ?
> >> I40E_PRIV_FLAGS_TRUE_PROMISC_SUPPORT : 0;
> >> }
> >> -
> >> + ret_flags |= pf->flags & I40E_FLAG_RSS_HSYM ?
> >> + I40E_PRIV_FLAGS_RSS_HSYM : 0;
> >> return ret_flags;
> >> }
> >>
> >
> > It might make sense to move all the private flags into one block and
> > just do a single setting of ret_flags like we do with the gso_flags
> > and netdev->features.
> >
> >> @@ -4453,6 +4456,14 @@ static int i40e_set_priv_flags(struct net_device
> *dev, u32 flags)
> >> else
> >> pf->auto_disable_flags |=
> I40E_FLAG_HW_ATR_EVICT_CAPABLE;
> >>
> >> + if (flags & I40E_PRIV_FLAGS_RSS_HSYM) {
> >> + pf->flags |= I40E_FLAG_RSS_HSYM;
> >> + reset_required = true;
> >> + } else {
> >> + pf->flags &= ~I40E_FLAG_RSS_HSYM;
> >> + reset_required = true;
> >> + }
> >> +
> >> /* if needed, issue reset to cause things to take effect */
> >> if (reset_required)
> >> i40e_do_reset(pf, BIT(__I40E_PF_RESET_REQUESTED));
> >
> > You shouldn't always require a reset. It should only require a reset
> > if the feature flag has changed.
>
> Yeah, good point.
>
>
> >> diff --git a/src/i40e_main.c b/src/i40e_main.c
> >> index b9b7d57..7f1cfe6 100644
> >> --- a/src/i40e_main.c
> >> +++ b/src/i40e_main.c
> >> @@ -8479,6 +8479,21 @@ static int i40e_pf_config_rss(struct i40e_pf *pf)
> >> (reg_val & ~I40E_PFQF_CTL_0_HASHLUTSIZE_512);
> >> i40e_write_rx_ctl(hw, I40E_PFQF_CTL_0, reg_val);
> >>
> >> + /* Set symmetric RSS hashing based on priv flag */
> >> + if (pf->flags & I40E_FLAG_RSS_HSYM) {
> >> + reg_val = i40e_read_rx_ctl(hw, I40E_PRTQF_CTL_0);
> >> + if ((reg_val & I40E_PRTQF_CTL_0_HSYM_ENA_MASK) == 0) {
> >> + reg_val |= I40E_PRTQF_CTL_0_HSYM_ENA_MASK;
> >> + i40e_write_rx_ctl(hw, I40E_PRTQF_CTL_0,
> reg_val);
> >> + }
> >> + } else {
> >> + reg_val = i40e_read_rx_ctl(hw, I40E_PRTQF_CTL_0);
> >> + if (reg_val & I40E_PRTQF_CTL_0_HSYM_ENA_MASK) {
> >> + reg_val &= ~I40E_PRTQF_CTL_0_HSYM_ENA_MASK;
> >> + i40e_write_rx_ctl(hw, I40E_PRTQF_CTL_0,
> reg_val);
> >> + }
> >> + }
> >> +
> >
> > You could probably pull the reading of reg_val out to the line before
> > you start this if statement since you do it in both cases. You could
> > probably also just define a mask that indicates what state you want so
> > that you then only xor and write the value if (!!(pf->flags &
> > 40E_FLAG_RSS_HSYM) != !!(reg_val & I40E_PRTQF_CTL_0_HSYM_ENA_MASK))
>
> Never used the !! notation, will look into that once there is agreement
> on the general direction.
>
> Thanks for your feedback!
>
> --
> ---------------------------------------------
> Victor Julien
> http://www.inliniac.net/
> PGP: http://www.inliniac.net/victorjulien.asc
> ---------------------------------------------
>
>
>
> ------------------------------------------------------------------------------
> Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
> Francisco, CA to explore cutting-edge tech and listen to tech luminaries
> present their vision of the future. This family event has something for
> everyone, including kids. Get more information and register today.
> http://sdm.link/attshape
> _______________________________________________
> E1000-devel mailing list
> E1000-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/e1000-devel
> To learn more about Intel® Ethernet, visit
> http://communities.intel.com/community/wired
>
--
Best regards,
Tal Abudi
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
E1000-devel mailing list
E1000-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/e1000-devel
To learn more about Intel® Ethernet, visit
http://communities.intel.com/community/wired
