From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 1E3F9A0524; Tue, 13 Apr 2021 14:28:17 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D34EB160F04; Tue, 13 Apr 2021 14:28:16 +0200 (CEST) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by mails.dpdk.org (Postfix) with ESMTP id 414DB160EFC for ; Tue, 13 Apr 2021 14:28:15 +0200 (CEST) IronPort-SDR: HzLAbh8scH96LeOvb8pK0EXo6+ql8Dj53inRxjFkNPtPbLA7YG3DmCqk2nmEspiYFyqsfyfCH2 faX0djVIwJtA== X-IronPort-AV: E=McAfee;i="6200,9189,9952"; a="194514142" X-IronPort-AV: E=Sophos;i="5.82,219,1613462400"; d="scan'208";a="194514142" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga102.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Apr 2021 05:28:13 -0700 IronPort-SDR: vo7hkBBd52BFiXIRxa3Z8pedRsW/t7Tdhc7JPzXWzNx9PsJIPhvRfiV8uracowVDkTzSk58jLK hP8EgHDAoZ4A== X-IronPort-AV: E=Sophos;i="5.82,219,1613462400"; d="scan'208";a="417840202" Received: from vmedvedk-mobl.ger.corp.intel.com (HELO [10.252.18.25]) ([10.252.18.25]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 13 Apr 2021 05:28:10 -0700 To: "Ananyev, Konstantin" , "dev@dpdk.org" Cc: "Chilikin, Andrey" , "Kinsella, Ray" , "Wang, Yipeng1" , "Gobriel, Sameh" , "Richardson, Bruce" References: <1615919077-77774-1-git-send-email-vladimir.medvedkin@intel.com> <1617738643-258635-3-git-send-email-vladimir.medvedkin@intel.com> <3fb31ad3-cb84-05a4-fb0e-46e33e008f57@intel.com> From: "Medvedkin, Vladimir" Message-ID: <8cb12932-aba3-66f2-4560-f7baffe950ea@intel.com> Date: Tue, 13 Apr 2021 15:28:08 +0300 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v2 2/3] hash: add predictable RSS implementation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Konstantin, On 12/04/2021 12:47, Ananyev, Konstantin wrote: >> >> >> >>>> +#defineRETA_SZ_MIN2U >>>> +#defineRETA_SZ_MAX16U >>> >>> Should these RETA_SZ defines be in public header? >>> So user can know what are allowed values? >>> >> >> I don't think this is necessary, because the user chooses it not >> arbitrary, but depending on the NIC. > > Sure thing, but it would be goo for the user to know what are the SW > Limitations on it without digging through .c. > I see, I'll move it to .h >> >>>> +#define RETA_SZ_IN_RANGE(reta_sz)((reta_sz >= RETA_SZ_MIN) && \ >> >> >> >>>> +uint32_t i; >>> >>> Empty line is missing. >>> >> >> Thanks >> >>>> +if ((name == NULL) || (key_len == 0) || !RETA_SZ_IN_RANGE(reta_sz)) { >>>> +rte_errno = EINVAL; >>>> +return NULL; >>>> +} >> >> >> >>>> +static inline void >>>> +set_bit(uint8_t *ptr, uint32_t bit, uint32_t pos) >>>> +{ >>>> +uint32_t byte_idx = pos >> 3; >>> >>> Just as a nit to be consistent with the line below: >>> pos / CHAR_BIT; >>> >> >> Fixed >> >>>> +uint32_t bit_idx = (CHAR_BIT - 1) - (pos & (CHAR_BIT - 1)); >>>> +uint8_t tmp; >> >> >> >>>> +ent = rte_zmalloc(NULL, sizeof(struct rte_thash_subtuple_helper) + >>>> +sizeof(uint32_t) * (1 << ctx->reta_sz_log), 0); >>> >>> Helper can be used by data-path code (via rte_thash_get_compliment()) right? >>> Then might be better to align it at cache-line. >>> >> >> Agree, I'll fix it >> >>>> +if (ent == NULL) >>>> +return -ENOMEM; >> >> >> >>>> uint32_t >>>> -rte_thash_get_compliment(struct rte_thash_subtuple_helper *h __rte_unused, >>>> -uint32_t hash __rte_unused, uint32_t desired_hash __rte_unused) >>>> +rte_thash_get_compliment(struct rte_thash_subtuple_helper *h, >>>> +uint32_t hash, uint32_t desired_hash) >>>> { >>>> -return 0; >>>> +return h->compl_table[(hash ^ desired_hash) & h->lsb_msk]; >>>> } >>> >>> Would it make sense to add another-one for multi values: >>> rte_thash_get_compliment(uint32_t hash, const uint32_t desired_hashes[], uint32_t adj_hash[], uint32_t num); >>> So user can get adjustment values for multiple queues at once? >>> >> >> At the moment I can't find scenarios why do we need to have a bulk >> version for this function > > My thought was about case when number of configured > HW queues is less than reta_size. > Let say reta_size==4, but user configured only 3 queues and reta={0,1,2,0}. > In that case for queue 0, both 0 and 3 values would suit. > Ah, yes, I remember out discussion about this. I'll think about this and add next releases. I should be kind of int rte_thash_get_compliment_reta(struct rte_thash_subtuple_helper *helper, uint32_t hash, const uint16_t queue_id, struct rte_eth_rss_reta_entry64 *reta, int reta_sz); where reta is completed by rte_eth_dev_rss_reta_query() -- Regards, Vladimir