DPDK patches and discussions
 help / color / mirror / Atom feed
From: Michel Machado <michel@digirati.com.br>
To: "Wang, Yipeng1" <yipeng1.wang@intel.com>,
	Qiaobin Fu <qiaobinf@bu.edu>,
	"Richardson, Bruce" <bruce.richardson@intel.com>,
	"De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"doucette@bu.edu" <doucette@bu.edu>,
	"Wiles, Keith" <keith.wiles@intel.com>,
	"Gobriel, Sameh" <sameh.gobriel@intel.com>,
	"Tai, Charlie" <charlie.tai@intel.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	"nd@arm.com" <nd@arm.com>,
	"honnappa.nagarahalli@arm.com" <honnappa.nagarahalli@arm.com>
Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries
Date: Tue, 4 Sep 2018 16:26:18 -0400
Message-ID: <dd13fa58-a3ac-3db5-413d-af0a7604ab2d@digirati.com.br> (raw)
In-Reply-To: <D2C4A16CA39F7F4E8E384D204491D7A6614A1A83@ORSMSX105.amr.corp.intel.com>

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

  reply	other threads:[~2018-09-04 20:26 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-31 16:51 Qiaobin Fu
2018-08-31 22:53 ` Gaëtan Rivet
2018-09-04 18:46   ` Michel Machado
2018-09-02 22:05 ` Honnappa Nagarahalli
2018-09-04 19:36   ` Michel Machado
2018-09-05 22:13     ` Honnappa Nagarahalli
2018-09-06 14:28       ` Michel Machado
2018-09-12 20:37         ` Honnappa Nagarahalli
2018-09-20 19:50           ` Michel Machado
2018-09-04 18:55 ` Wang, Yipeng1
2018-09-04 19:07   ` Michel Machado
2018-09-04 19:51     ` Wang, Yipeng1
2018-09-04 20:26       ` Michel Machado [this message]
2018-09-04 20:57         ` Wang, Yipeng1
2018-09-05 17:52           ` Michel Machado
2018-09-05 20:27             ` Wang, Yipeng1
2018-09-06 13:34               ` Michel Machado
2018-10-09 19:29 ` [dpdk-dev] [PATCH v4 1/2] hash table: fix a bug in rte_hash_iterate() Qiaobin Fu
2018-10-09 19:29   ` [dpdk-dev] [PATCH v4 2/2] hash table: add an iterator over conflicting entries Qiaobin Fu
2018-10-10  2:54     ` Wang, Yipeng1
2018-10-10  1:55   ` [dpdk-dev] [PATCH v4 1/2] hash table: fix a bug in rte_hash_iterate() Wang, Yipeng1
  -- strict thread matches above, loose matches on Subject: below --
2018-08-30 15:56 [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries Qiaobin Fu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dd13fa58-a3ac-3db5-413d-af0a7604ab2d@digirati.com.br \
    --to=michel@digirati.com.br \
    --cc=bruce.richardson@intel.com \
    --cc=charlie.tai@intel.com \
    --cc=dev@dpdk.org \
    --cc=doucette@bu.edu \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=keith.wiles@intel.com \
    --cc=nd@arm.com \
    --cc=pablo.de.lara.guarch@intel.com \
    --cc=qiaobinf@bu.edu \
    --cc=sameh.gobriel@intel.com \
    --cc=stephen@networkplumber.org \
    --cc=yipeng1.wang@intel.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git