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

[Alexander Duyck <alexander.h.duyck@redhat.com>]

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.

This loop probably needs to be rewritten so that you walk though 
processing 4 entries at a time, and generate the register starting at 
rss_indr_tbl[i + 3] and work your way down to rss_indr_tbl[i].

> @@ -3283,34 +3286,88 @@ static void ixgbe_setup_reta(struct ixgbe_adapter *adapter, const u32 *seed)
>   	}
>   }
>   
> -static void ixgbe_setup_vfreta(struct ixgbe_adapter *adapter, const u32 *seed)
> +/**
> + * Write the RETA table to HW (for x550 devices in SRIOV mode)
> + *
> + * @adapter: device handle
> + *
> + * Write the RSS redirection table stored in adapter.rss_indir_tbl[] to HW.
> + */
> +static void ixgbe_store_vfreta(struct ixgbe_adapter *adapter)
>   {
> +	u32 i, reta_entries = ixgbe_rss_indir_tbl_entries(adapter);
>   	struct ixgbe_hw *hw = &adapter->hw;
>   	u32 vfreta = 0;
> +	unsigned int pf_pool = adapter->num_vfs;
> +
> +	/* Write redirection table to HW */
> +	for (i = 0; i < reta_entries; i++) {
> +		vfreta = (vfreta << 8) | adapter->rss_indir_tbl[i];
> +		if ((i & 3) == 3)
> +			IXGBE_WRITE_REG(hw, IXGBE_PFVFRETA(i >> 2, pf_pool),
> +					vfreta);
> +	}
> +}
> +

Same here, the byte ordering is wrong.

An alternative would be to store the RETA as a union that is both a u8 
array and a __le32 array.  Then you could write everything in the first 
pass as bytes, and when you go to write it to the registers you could 
read it using le32_to_cpu, then write the value into the register.

- Alex