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?
> To enable:
> ethtool --set-priv-flags $DEV rss-symmetric on
>
> To disable:
> ethtool --set-priv-flags $DEV rss-symmetric off
>
> To see the current setting:
> # ethtool --show-priv-flags $DEV
> Private flags for ens2f1:
> ...
> 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.
> 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.
> 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.
> 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.
> 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))
> /* Determine the RSS size of the VSI */
> if (!vsi->rss_size)
> vsi->rss_size = min_t(int, pf->alloc_rss_size,
> --
> 2.7.4
>
>
> ------------------------------------------------------------------------------
> 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
------------------------------------------------------------------------------
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
