DPDK patches and discussions
 help / color / mirror / Atom feed
From: Michel Machado <michel@digirati.com.br>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	Qiaobin Fu <qiaobinf@bu.edu>,
	"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
	"pablo.de.lara.guarch@intel.com" <pablo.de.lara.guarch@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"doucette@bu.edu" <doucette@bu.edu>,
	"keith.wiles@intel.com" <keith.wiles@intel.com>,
	"sameh.gobriel@intel.com" <sameh.gobriel@intel.com>,
	"charlie.tai@intel.com" <charlie.tai@intel.com>,
	"stephen@networkplumber.org" <stephen@networkplumber.org>,
	nd <nd@arm.com>,
	"yipeng1.wang@intel.com" <yipeng1.wang@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries
Date: Thu, 6 Sep 2018 10:28:41 -0400
Message-ID: <df706d15-a877-76c1-1627-97d4963fc66d@digirati.com.br> (raw)
In-Reply-To: <AM6PR08MB3672FDBB50B197C59A7444E098020@AM6PR08MB3672.eurprd08.prod.outlook.com>

On 09/05/2018 06:13 PM, Honnappa Nagarahalli wrote:
>> +	uint32_t              next;
>> +	uint32_t              total_entries;
>> +};
>> This structure can be moved to rte_cuckoo_hash.h file.
> 
>      What's the purpose of moving this struct to a header file since it's only used in the C file rte_cuckoo_hash.c?
> 
> This is to maintain consistency. For ex: 'struct queue_node', which is an internal structure, is kept in rte_cuckoo_hash.h

    Okay. We'll move it there.

>> +int32_t
>> +rte_hash_iterator_init(const struct rte_hash *h,
>> +	struct rte_hash_iterator_state *state) {
>> +	struct rte_hash_iterator_istate *__state;
>> '__state' can be replaced by 's'.
>>
>> +
>> +	RETURN_IF_TRUE(((h == NULL) || (state == NULL)), -EINVAL);
>> +
>> +	__state = (struct rte_hash_iterator_istate *)state;
>> +	__state->h = h;
>> +	__state->next = 0;
>> +	__state->total_entries = h->num_buckets * RTE_HASH_BUCKET_ENTRIES;
>> +
>> +	return 0;
>> +}
>> IMO, creating this API can be avoided if the initialization is handled in 'rte_hash_iterate' function. The cost of doing this is very trivial (one extra 'if' statement) in 'rte_hash_iterate' function. It will help keep the number of APIs to minimal.
> 
>      Applications would have to initialize struct rte_hash_iterator_state *state before calling rte_hash_iterate() anyway. Why not initializing the fields of a state only once?
> 
> My concern is about creating another API for every iterator API. You have a valid point on saving cycles as this API applies for data plane. Have you done any performance benchmarking with and without this API? May be we can guide our decision based on that.

    It's not just about creating one init function for each iterator 
because an iterator may have a couple of init functions. For example, 
someone may eventually find useful to add another init function for the 
conflicting-entry iterator that we are advocating in this patch. A 
possibility would be for this new init function to use the key of the 
new entry instead of its signature to initialize the state. Similar to 
what is already done in rte_hash_lookup*() functions. In spite of 
possibly having multiple init functions, there will be a single iterator 
function.

    About the performance benchmarking, the current API only requites 
applications to initialize a single 32-bit integer. But with the 
adoption of a struct for the state, the initialization will grow to 64 
bytes.

>>    int32_t
>> -rte_hash_iterate(const struct rte_hash *h, const void **key, void
>> **data, uint32_t *next)
>> +rte_hash_iterate(
>> +	struct rte_hash_iterator_state *state, const void **key, void
>> +**data)
>>
>> IMO, as suggested above, do not store 'struct rte_hash *h' in 'struct rte_hash_iterator_state'. Instead, change the API definition as follows:
>> rte_hash_iterate(const struct rte_hash *h, const void **key, void
>> **data, struct rte_hash_iterator_state *state)
>>
>> This will help keep the API signature consistent with existing APIs.
>>
>> This is an ABI change. Please take a look at https://doc.dpdk.org/guides/contributing/versioning.html.
> 
>      The ABI will change in a way or another, so why not going for a single state instead of requiring parameters that are already needed for the initialization of the state?
> 
> Are there any cost savings we can achieve by keeping the 'h' in the iterator state?

    There's a tiny cost saving: avoiding to push that parameter in the 
execution stack every time the iterator will get another entry. However, 
the reason I find more important is to make impossible to introduce a 
bug in the code. Consider a function that is dealing with two hash 
tables and two iterators. Without asking for the hash table to make 
progress in an iterator, it's impossible to mix up hash tables and 
iterator states.

    There's even the possibility that an iterator doesn't need the hash 
table after its initialization. This would be an *unlikely* case, but 
consider an iterator that only returns a couple of entries. It could 
cache those entries during initialization.

>>    	/* Calculate bucket and index of current iterator */
>> -	bucket_idx = *next / RTE_HASH_BUCKET_ENTRIES;
>> -	idx = *next % RTE_HASH_BUCKET_ENTRIES;
>> +	bucket_idx = __state->next / RTE_HASH_BUCKET_ENTRIES;
>> +	idx = __state->next % RTE_HASH_BUCKET_ENTRIES;
>>    
>>    	/* If current position is empty, go to the next one */
>> -	while (h->buckets[bucket_idx].key_idx[idx] == EMPTY_SLOT) {
>> -		(*next)++;
>> +	while (__state->h->buckets[bucket_idx].key_idx[idx] == EMPTY_SLOT) {
>> +		__state->next++;
>>    		/* End of table */
>> -		if (*next == total_entries)
>> +		if (__state->next == __state->total_entries)
>>    			return -ENOENT;
>> -		bucket_idx = *next / RTE_HASH_BUCKET_ENTRIES;
>> -		idx = *next % RTE_HASH_BUCKET_ENTRIES;
>> +		bucket_idx = __state->next / RTE_HASH_BUCKET_ENTRIES;
>> +		idx = __state->next % RTE_HASH_BUCKET_ENTRIES;
>>    	}
>> -	__hash_rw_reader_lock(h);
>> +	__hash_rw_reader_lock(__state->h);
>>    	/* Get position of entry in key table */
>> -	position = h->buckets[bucket_idx].key_idx[idx];
>> -	next_key = (struct rte_hash_key *) ((char *)h->key_store +
>> -				position * h->key_entry_size);
>> +	position = __state->h->buckets[bucket_idx].key_idx[idx];
>> +	next_key = (struct rte_hash_key *) ((char *)__state->h->key_store +
>> +				position * __state->h->key_entry_size);
>>    	/* Return key and data */
>>    	*key = next_key->key;
>>    	*data = next_key->pdata;
>>    
>> -	__hash_rw_reader_unlock(h);
>> +	__hash_rw_reader_unlock(__state->h);
>>    
>>    	/* Increment iterator */
>> -	(*next)++;
>> +	__state->next++;
>> This comment is not related to this change, it is better to place this inside the lock.
> 
>      Even though __state->next does not depend on the lock?
> 
> It depends on if this API needs to be multi-thread safe. Interestingly, the documentation does not say it is multi-thread safe. If it has to be multi-thread safe, then the state also needs to be protected. For ex: what happens if the user uses a global variable for the state?

    If an application needs to share an iterator state between threads, 
it has to have a synchronization mechanism for that as it would for any 
other shared variable. The lock above is allowing applications to share 
a hash table between threads, it has no semantic over anything else.

>> diff --git a/lib/librte_hash/rte_hash.h b/lib/librte_hash/rte_hash.h
>> index 9e7d9315f..fdb01023e 100644
>> --- a/lib/librte_hash/rte_hash.h
>> +++ b/lib/librte_hash/rte_hash.h
>> @@ -14,6 +14,8 @@
>>    #include <stdint.h>
>>    #include <stddef.h>
>>    
>> +#include <rte_compat.h>
>> +
>>    #ifdef __cplusplus
>>    extern "C" {
>>    #endif
>> @@ -64,6 +66,16 @@ struct rte_hash_parameters {
>>    /** @internal A hash table structure. */  struct rte_hash;
>>    
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this API may change without prior notice.
>> + *
>> + * @internal A hash table iterator state structure.
>> + */
>> +struct rte_hash_iterator_state {
>> +	uint8_t space[64];
>> I would call this 'state'. 64 can be replaced by 'RTE_CACHE_LINE_SIZE'.
> 
>      Okay.

    I think we should not replace 64 with RTE_CACHE_LINE_SIZE because 
the ABI would change based on the architecture for which it's compiled.

[ ]'s
Michel Machado

  reply	other threads:[~2018-09-06 14:28 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 [this message]
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
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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=df706d15-a877-76c1-1627-97d4963fc66d@digirati.com.br \
    --to=michel@digirati.com.br \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=bruce.richardson@intel.com \
    --cc=charlie.tai@intel.com \
    --cc=dev@dpdk.org \
    --cc=doucette@bu.edu \
    --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 \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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 \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


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