From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mta112.f1.k8.com.br (mta112.f1.k8.com.br [187.73.32.184]) by dpdk.org (Postfix) with ESMTP id DEF291DBE for ; Tue, 4 Sep 2018 22:26:33 +0200 (CEST) Received: from [192.168.1.4] (pool-173-48-214-200.bstnma.fios.verizon.net [173.48.214.200]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtpz.f1.k8.com.br (Postfix) with ESMTPSA id 5722680146; Tue, 4 Sep 2018 20:26:21 +0000 (UTC) X-DKIM: OpenDKIM Filter v2.6.8 smtpz.f1.k8.com.br 5722680146 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=digirati.com.br; s=default; t=1536092791; bh=HBthC1YIH8xx+z9lzwY8HgJLhbnl2jWD7ZOLy7NjMFE=; h=Subject:To:From:Date:Feedback-ID; b=LQvgraukC+SHS8bWHEYogWnd85Q28GwvisJ4ppkfuraMBFcNTn6jsJ3G6UGf7WalR y0m6Mnq5oqNdxpLnLB5J94M9r9qqar509QZ4AvGBjwrQM3eP4WbpyaG+R4wIsGmpDP lnXG74fSuFTR+TB25mE6+/saZBAoYj0FbCHp1Ejk= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=k8.com.br; s=default; t=1536092791; bh=HBthC1YIH8xx+z9lzwY8HgJLhbnl2jWD7ZOLy7NjMFE=; h=Subject:To:From:Date:Feedback-ID; b=KktdQ35LuWbuBHFzaNw6risEXItScOf2CKrQQzHGYU+p/9O41nkr+gkE5AFnAA9ra fiX7uenouZ83gHo0wllYlA6II+RMH4nVQyKa3jKYIVEzCfl2abK08IVlNcU3bMicjy SFm1aPgsHy0KY9uTUXTUMKi9RMw/ceJcUzRmQqTE= To: "Wang, Yipeng1" , Qiaobin Fu , "Richardson, Bruce" , "De Lara Guarch, Pablo" Cc: "dev@dpdk.org" , "doucette@bu.edu" , "Wiles, Keith" , "Gobriel, Sameh" , "Tai, Charlie" , "stephen@networkplumber.org" , "nd@arm.com" , "honnappa.nagarahalli@arm.com" References: <20180831165101.20026-1-qiaobinf@bu.edu> From: Michel Machado Organization: Digirati Internet LTDA. Message-ID: Date: Tue, 4 Sep 2018 16:26:18 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-HN-S: bWljaGVsQGRpZ2lyYXRpLmNvbS5icg== X-HN-R: aG9ubmFwcGEubmFnYXJhaGFsbGlAYXJtLmNvbSxuZEBhcm0uY29tLHN0ZXBoZW5AbmV0d29ya3BsdW1iZXIub3JnLGNoYXJsaWUudGFpQGludGVsLmNvbSxzYW1laC5nb2JyaWVsQGludGVsLmNvbSxrZWl0aC53aWxlc0BpbnRlbC5jb20sZG91Y2V0dGVAYnUuZWR1LGRldkBkcGRrLm9yZyxwYWJsby5kZS5sYXJhLmd1YXJjaEBpbnRlbC5jb20sYnJ1Y2UucmljaGFyZHNvbkBpbnRlbC5jb20scWlhb2JpbmZAYnUuZWR1LHlpcGVuZzEud2FuZ0BpbnRlbC5jb20= Feedback-ID: MjAxODA5MDQ=:bWljaGVsQGRpZ2lyYXRpLmNvbS5icg==:ZGlnaXJhdGkuY29tLmJy:k8networks X-Mailman-Approved-At: Thu, 06 Sep 2018 15:08:09 +0200 Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 Sep 2018 20:26:34 -0000 Hi Yipeng, On 09/04/2018 03:51 PM, Wang, Yipeng1 wrote: > Hmm, I guess my comment is for code readability. If we don’t need the extra state that would be great. Notice that applications only see the public, opaque state. And the private versions are scoped to the C file where they are needed. > I think "rte_hash" is defined as an internal data structure but expose the type to the public header. Would this work? Exposing the private fields would bind the interface with the current implementation of the hash table. In the way we are proposing, one should be able to replace the underlying algorithm and not touching the header files that applications use. But, yes, your solution would enable applications to allocate iterator states as local variables as well. > I propose to malloc inside function mostly because I think it is cleaner to the user. But your argument is > valid. Depending on use case I think it is OK. I don't know how other applications will use this iterator, but we use it when our application is already overloaded. So avoiding an expensive operation like malloc() is a win. > Another comment is you put the total_entry in the state, is it for performance of the rte_hash_iterate? We are saving one integer multiplication per call of rte_hash_iterate(). It's not a big deal, but since there's room in the state variable, we thought this would be a good idea because it grows with the size of the table. We didn't actually measure the effect of this decision. > If you use it to iterate conflict entries, especially If you reuse same "state" struct and init it again and again for different keys, > would this slow down the performance for your specific use case? Notice that the field total_entry only exists for rte_hash_iterate(). But even if total_entry were in the state of rte_hash_iterate_conflict_entries(), it would still save on the multiplication as long as rte_hash_iterate_conflict_entries() is called at least twice. Calling rte_hash_iterate_conflict_entries() once evens out, and calling rte_hash_iterate_conflict_entries() more times adds further savings. As a side note. in our application, whenever an iterator of conflicting entries is initialized, we call rte_hash_iterate_conflict_entries() at least once. > Also iterate_conflic_entry may need reader lock protection. We are going to add the reader lock protection. Thanks. [ ]'s Michel Machado