From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 13E9591FC for ; Fri, 16 Oct 2015 00:19:27 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga103.jf.intel.com with ESMTP; 15 Oct 2015 15:19:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.17,687,1437462000"; d="scan'208";a="581718279" Received: from irsmsx105.ger.corp.intel.com ([163.33.3.28]) by FMSMGA003.fm.intel.com with ESMTP; 15 Oct 2015 15:19:25 -0700 Received: from irsmsx155.ger.corp.intel.com (163.33.192.3) by irsmsx105.ger.corp.intel.com (163.33.3.28) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 15 Oct 2015 23:19:25 +0100 Received: from irsmsx105.ger.corp.intel.com ([169.254.7.75]) by irsmsx155.ger.corp.intel.com ([169.254.14.242]) with mapi id 14.03.0248.002; Thu, 15 Oct 2015 23:19:24 +0100 From: "Ananyev, Konstantin" To: "Lu, Wenzhuo" , "dev@dpdk.org" Thread-Topic: [dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550 Thread-Index: AQHQ+cK/QSt7c3Ph2Ea9fmXSbfYouJ5tOC7A Date: Thu, 15 Oct 2015 22:19:23 +0000 Message-ID: <2601191342CEEE43887BDE71AB97725836AB02D5@irsmsx105.ger.corp.intel.com> References: <1443426751-4906-1-git-send-email-wenzhuo.lu@intel.com> <1443426751-4906-2-git-send-email-wenzhuo.lu@intel.com> In-Reply-To: <1443426751-4906-2-git-send-email-wenzhuo.lu@intel.com> Accept-Language: en-IE, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.182] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Oct 2015 22:19:28 -0000 Hi Wenzhuo, > -----Original Message----- > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu > Sent: Monday, September 28, 2015 8:52 AM > To: dev@dpdk.org > Subject: [dpdk-dev] [PATCH 1/4] ixgbe: 512 entries RSS table on x550 >=20 > Comparing with the older NICs, x550's RSS redirection table is enlarged t= o 512 > entries. As the original code is for the NICs which have a 128 entries RS= S table, > it means only part of the RSS table is set on x550. So, RSS cannot work a= s > expected on x550, it doesn't redirect the packets evenly. > This patch configs the entries beyond 128 on x550 to let RSS work well, a= nd also > update the query and update functions to support 512 entries. >=20 > Signed-off-by: Wenzhuo Lu > --- > drivers/net/ixgbe/ixgbe_ethdev.c | 106 +++++++++++++++++++++++++++++++++= +----- > drivers/net/ixgbe/ixgbe_rxtx.c | 18 +++++-- > 2 files changed, 109 insertions(+), 15 deletions(-) >=20 > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_e= thdev.c > index ec2918c..a1ef26f 100644 > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > @@ -2397,7 +2397,17 @@ ixgbe_dev_info_get(struct rte_eth_dev *dev, struct= rte_eth_dev_info *dev_info) > ETH_TXQ_FLAGS_NOOFFLOADS, > }; > dev_info->hash_key_size =3D IXGBE_HKEY_MAX_INDEX * sizeof(uint32_t); > - dev_info->reta_size =3D ETH_RSS_RETA_SIZE_128; > + > + switch (hw->mac.type) { > + case ixgbe_mac_X550: > + case ixgbe_mac_X550EM_x: > + dev_info->reta_size =3D ETH_RSS_RETA_SIZE_512; > + break; > + default: > + dev_info->reta_size =3D ETH_RSS_RETA_SIZE_128; > + break; > + } > + Instead of adding switch(hw->mac_type) in dozen of places, why not to create a new function: get_reta_size(hw) and use it whenever it is n= eeded? > dev_info->flow_type_rss_offloads =3D IXGBE_RSS_OFFLOAD_ALL; > } >=20 > @@ -3205,14 +3215,29 @@ ixgbe_dev_rss_reta_update(struct rte_eth_dev *dev= , > struct ixgbe_hw *hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private)= ; >=20 > PMD_INIT_FUNC_TRACE(); > - if (reta_size !=3D ETH_RSS_RETA_SIZE_128) { > - PMD_DRV_LOG(ERR, "The size of hash lookup table configured " > - "(%d) doesn't match the number hardware can supported " > - "(%d)\n", reta_size, ETH_RSS_RETA_SIZE_128); > - return -EINVAL; > + switch (hw->mac.type) { > + case ixgbe_mac_X550: > + case ixgbe_mac_X550EM_x: > + if (reta_size !=3D ETH_RSS_RETA_SIZE_512) { > + PMD_DRV_LOG(ERR, "The size of hash lookup table " > + "configured (%d) doesn't match the " > + "number hardware can supported (%d)\n", > + reta_size, ETH_RSS_RETA_SIZE_512); > + return -EINVAL; > + } > + break; > + default: > + if (reta_size !=3D ETH_RSS_RETA_SIZE_128) { > + PMD_DRV_LOG(ERR, "The size of hash lookup table " > + "configured (%d) doesn't match the " > + "number hardware can supported (%d)\n", > + reta_size, ETH_RSS_RETA_SIZE_128); > + return -EINVAL; > + } > + break; > } >=20 > - for (i =3D 0; i < reta_size; i +=3D IXGBE_4_BIT_WIDTH) { > + for (i =3D 0; i < ETH_RSS_RETA_SIZE_128; i +=3D IXGBE_4_BIT_WIDTH) { > idx =3D i / RTE_RETA_GROUP_SIZE; > shift =3D i % RTE_RETA_GROUP_SIZE; > mask =3D (uint8_t)((reta_conf[idx].mask >> shift) & > @@ -3234,6 +3259,30 @@ ixgbe_dev_rss_reta_update(struct rte_eth_dev *dev, > IXGBE_WRITE_REG(hw, IXGBE_RETA(i >> 2), reta); > } >=20 > + for (i =3D ETH_RSS_RETA_SIZE_128; i < reta_size; i +=3D IXGBE_4_BIT_WID= TH) { > + idx =3D i / RTE_RETA_GROUP_SIZE; > + shift =3D i % RTE_RETA_GROUP_SIZE; > + mask =3D (uint8_t)((reta_conf[idx].mask >> shift) & > + IXGBE_4_BIT_MASK); > + if (!mask) > + continue; > + if (mask =3D=3D IXGBE_4_BIT_MASK) > + r =3D 0; > + else > + r =3D IXGBE_READ_REG(hw, > + IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >> 2)); > + for (j =3D 0, reta =3D 0; j < IXGBE_4_BIT_WIDTH; j++) { > + if (mask & (0x1 << j)) > + reta |=3D reta_conf[idx].reta[shift + j] << > + (CHAR_BIT * j); > + else > + reta |=3D r & (IXGBE_8_BIT_MASK << > + (CHAR_BIT * j)); > + } > + IXGBE_WRITE_REG(hw, > + IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >> 2), reta); > + } > + Is there any good reason to create a second loop and duplicate loops body? Why not just: if (reta_size !=3D get_reta_size(hw) {return -EINVAL;} for (i=3D0; I !=3D reta_size; i+=3D...) {...} ? =20 > return 0; > } >=20 > @@ -3248,11 +3297,26 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev, > struct ixgbe_hw *hw =3D IXGBE_DEV_PRIVATE_TO_HW(dev->data->dev_private)= ; >=20 > PMD_INIT_FUNC_TRACE(); > - if (reta_size !=3D ETH_RSS_RETA_SIZE_128) { > - PMD_DRV_LOG(ERR, "The size of hash lookup table configured " > - "(%d) doesn't match the number hardware can supported " > - "(%d)\n", reta_size, ETH_RSS_RETA_SIZE_128); > - return -EINVAL; > + switch (hw->mac.type) { > + case ixgbe_mac_X550: > + case ixgbe_mac_X550EM_x: > + if (reta_size !=3D ETH_RSS_RETA_SIZE_512) { > + PMD_DRV_LOG(ERR, "The size of hash lookup table " > + " configured (%d) doesn't match the " > + "number hardware can supported (%d)\n", > + reta_size, ETH_RSS_RETA_SIZE_512); > + return -EINVAL; > + } > + break; > + default: > + if (reta_size !=3D ETH_RSS_RETA_SIZE_128) { > + PMD_DRV_LOG(ERR, "The size of hash lookup table " > + " configured (%d) doesn't match the " > + "number hardware can supported (%d)\n", > + reta_size, ETH_RSS_RETA_SIZE_128); > + return -EINVAL; > + } > + break; > } >=20 > for (i =3D 0; i < ETH_RSS_RETA_SIZE_128; i +=3D IXGBE_4_BIT_WIDTH) { > @@ -3272,6 +3336,24 @@ ixgbe_dev_rss_reta_query(struct rte_eth_dev *dev, > } > } >=20 > + for (i =3D ETH_RSS_RETA_SIZE_128; i < reta_size; i +=3D IXGBE_4_BIT_WID= TH) { > + idx =3D i / RTE_RETA_GROUP_SIZE; > + shift =3D i % RTE_RETA_GROUP_SIZE; > + mask =3D (uint8_t)((reta_conf[idx].mask >> shift) & > + IXGBE_4_BIT_MASK); > + if (!mask) > + continue; > + > + reta =3D IXGBE_READ_REG(hw, > + IXGBE_ERETA((i - ETH_RSS_RETA_SIZE_128) >> 2)); > + for (j =3D 0; j < IXGBE_4_BIT_WIDTH; j++) { > + if (mask & (0x1 << j)) > + reta_conf[idx].reta[shift + j] =3D > + ((reta >> (CHAR_BIT * j)) & > + IXGBE_8_BIT_MASK); > + } > + } > + Same as above - don't see any need to create an extra loop with identical b= ody. Pls merge them into one. > return 0; > } >=20 > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxt= x.c > index a598a72..a746ae7 100644 > --- a/drivers/net/ixgbe/ixgbe_rxtx.c > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c > @@ -2790,7 +2790,7 @@ ixgbe_rss_configure(struct rte_eth_dev *dev) > { > struct rte_eth_rss_conf rss_conf; > struct ixgbe_hw *hw; > - uint32_t reta; > + uint32_t reta =3D 0; > uint16_t i; > uint16_t j; >=20 > @@ -2802,8 +2802,7 @@ ixgbe_rss_configure(struct rte_eth_dev *dev) > * The byte-swap is needed because NIC registers are in > * little-endian order. > */ > - reta =3D 0; What was wrong with that one? > - for (i =3D 0, j =3D 0; i < 128; i++, j++) { > + for (i =3D 0, j =3D 0; i < ETH_RSS_RETA_SIZE_128; i++, j++) { > if (j =3D=3D dev->data->nb_rx_queues) > j =3D 0; > reta =3D (reta << 8) | j; > @@ -2812,6 +2811,19 @@ ixgbe_rss_configure(struct rte_eth_dev *dev) > rte_bswap32(reta)); > } >=20 > + if ((hw->mac.type =3D=3D ixgbe_mac_X550) || > + (hw->mac.type =3D=3D ixgbe_mac_X550EM_x)) { > + for (i =3D 0, j =3D 0; i < (ETH_RSS_RETA_SIZE_512 - ETH_RSS_RETA_SIZE_= 128); > + i++, j++) { > + if (j =3D=3D dev->data->nb_rx_queues) > + j =3D 0; > + reta =3D (reta << 8) | j; > + if ((i & 3) =3D=3D 3) > + IXGBE_WRITE_REG(hw, IXGBE_ERETA(i >> 2), > + rte_bswap32(reta)); > + } > + } > + Same as above. Seems no need to have 2 identical loops, pls merge into one. Konstantin > /* > * Configure the RSS key and the RSS protocols used to compute > * the RSS hash of input packets. > -- > 1.9.3