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
