Re: [Intel-wired-lan] [PATCH net-next v1 1/2] ixgbe: Refactor the RSS configuration code.

[Vlad Zolotarov <vladz@cloudius-systems.com>]

On 03/30/15 20:04, Alexander Duyck wrote:
> On 03/30/2015 09:59 AM, Vlad Zolotarov wrote:
>>
>> On 03/30/15 19:19, Alexander Duyck wrote:
>>> On 03/26/2015 10:56 AM, Vlad Zolotarov wrote:
>>>> This patch is a preparation for enablement of ethtool RSS
>>>> indirection table
>>>> and hash key querying. We don't want to read registers every time
>>>> the RSS info
>>>> is queried. Therefore we will store its current content in the
>>>> arrays in the adapter struct and will read it from there (instead of
>>>> from
>>>> registers) when requested.
>>>>
>>>> Will change the code that writes the indirection table and hash key
>>>> into the HW registers
>>>> to take its content from these arrays. This will also simplify the
>>>> indirection
>>>> table updating ethtool callback implementation in the future.
>>>>
>>>> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
>>>> ---
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe.h      |   2 +
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 143
>>>> ++++++++++++++++++--------
>>>>    drivers/net/ethernet/intel/ixgbe/ixgbe_type.h |   7 ++
>>>>    3 files changed, 109 insertions(+), 43 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> index 5e44e48..402d182 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
>>>> @@ -766,6 +766,8 @@ struct ixgbe_adapter {
>>>>          u8 default_up;
>>>>        unsigned long fwd_bitmask; /* Bitmask indicating in use pools */
>>>> +    u8 rss_indir_tbl[IXGBE_MAX_RETA_ENTRIES];
>>>> +    u32 rss_key[IXGBE_RSS_KEY_SIZE / sizeof(u32)];
>>>>    };
>>>>      static inline u8 ixgbe_max_rss_indices(struct ixgbe_adapter
>>>> *adapter)
>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> index 8853d52..e3de0c9 100644
>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>>>> @@ -3228,51 +3228,54 @@ static void ixgbe_configure_srrctl(struct
>>>> ixgbe_adapter *adapter,
>>>>        IXGBE_WRITE_REG(hw, IXGBE_SRRCTL(reg_idx), srrctl);
>>>>    }
>>>>    -static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const
>>>> u32 *seed)
>>>> +/**
>>>> + * Return a number of entries in the RSS indirection table
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + *  - 82598/82599/X540:     128
>>>> + *  - X550(non-SRIOV mode): 512
>>>> + *  - X550(SRIOV mode):     64
>>>> + */
>>>> +static u32 ixgbe_rss_indir_tbl_entries(struct ixgbe_adapter *adapter)
>>>> +{
>>>> +    if (adapter->hw.mac.type < ixgbe_mac_X550)
>>>> +        return 128;
>>>> +    else if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>>>> +        return 64;
>>>> +    else
>>>> +        return 512;
>>>> +}
>>>> +
>>>> +/**
>>>> + * Write the RETA table to HW
>>>> + *
>>>> + * @adapter: device handle
>>>> + *
>>>> + * Write the RSS redirection table stored in
>>>> adapter.rss_indir_tbl[] to HW.
>>>> + */
>>>> +static void ixgbe_store_reta(struct ixgbe_adapter *adapter)
>>>>    {
>>>> +    u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>>>>        struct ixgbe_hw *hw = &adapter->hw;
>>>>        u32 reta = 0;
>>>> -    int i, j;
>>>> -    int reta_entries = 128;
>>>> -    u16 rss_i = adapter->ring_feature[RING_F_RSS].indices;
>>>>        int indices_multi;
>>>> -
>>>> -    /*
>>>> -     * Program table for at least 2 queues w/ SR-IOV so that VFs can
>>>> -     * make full use of any rings they may have.  We will use the
>>>> -     * PSRTYPE register to control how many rings we use within the
>>>> PF.
>>>> -     */
>>>> -    if ((adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) && (rss_i < 2))
>>>> -        rss_i = 2;
>>>> -
>>>> -    /* Fill out hash function seeds */
>>>> -    for (i = 0; i < 10; i++)
>>>> -        IXGBE_WRITE_REG(hw, IXGBE_RSSRK(i), seed[i]);
>>>> +    u8 *indir_tbl = adapter->rss_indir_tbl;
>>>>          /* Fill out the redirection table as follows:
>>>> -     * 82598: 128 (8 bit wide) entries containing pair of 4 bit RSS
>>>> indices
>>>> -     * 82599/X540: 128 (8 bit wide) entries containing 4 bit RSS index
>>>> -     * X550: 512 (8 bit wide) entries containing 6 bit RSS index
>>>> +     *  - 82598:      8 bit wide entries containing pair of 4 bit RSS
>>>> +     *    indices.
>>>> +     *  - 82599/X540: 8 bit wide entries containing 4 bit RSS index
>>>> +     *  - X550:       8 bit wide entries containing 6 bit RSS index
>>>>         */
>>>>        if (adapter->hw.mac.type == ixgbe_mac_82598EB)
>>>>            indices_multi = 0x11;
>>>>        else
>>>>            indices_multi = 0x1;
>>>>    -    switch (adapter->hw.mac.type) {
>>>> -    case ixgbe_mac_X550:
>>>> -    case ixgbe_mac_X550EM_x:
>>>> -        if (!(adapter->flags & IXGBE_FLAG_SRIOV_ENABLED))
>>>> -            reta_entries = 512;
>>>> -    default:
>>>> -        break;
>>>> -    }
>>>> -
>>>> -    /* Fill out redirection table */
>>>> -    for (i = 0, j = 0; i < reta_entries; i++, j++) {
>>>> -        if (j == rss_i)
>>>> -            j = 0;
>>>> -        reta = (reta << 8) | (j * indices_multi);
>>>> +    /* Write redirection table to HW */
>>>> +    for (i = 0; i < reta_entries; i++) {
>>>> +        reta = (reta << 8) | (indices_multi * indir_tbl[i]);
>>>>            if ((i & 3) == 3) {
>>>>                if (i < 128)
>>>>                    IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta);
>>> I am pretty sure this is being written in the wrong byte order. Entry
>>> 0 should be the lowest bits in the register, not the highest.
>> The order of entries doesn't seem to be important here.
>>
>> thanks,
>> vlad
> Huh?  It is populating the register as (0 << 24) | (1 << 16) | (2 << 8)
> | (3 << 0) which means the 3rd entry is populating the entry belonging
> to what should be entry 0.  This is fine as long as we aren't reporting
> it or allowing changes.  However, now that it is being reported via your
> changes it matters.  Otherwise if someone were to take the key and the
> redirection table and do the calculations themselves the results
> provided by the hardware would appear to be wrong.

Hmmm. Right. No prob. I'll biteswap32 them before writing.

>
> In order to get the ordering right we need to either byteswap the reta
> value before we write it, or generate the value in the correct order
> which should be (3 << 24) | (2 << 16) | (1 << 8) | 0.
>
> - Alex