DPDK patches and discussions
 help / color / mirror / Atom feed
From: Michel Machado <michel@digirati.com.br>
To: Gaëtan Rivet <gaetan.rivet@6wind.com>, Qiaobin Fu <qiaobinf@bu.edu>
Cc: bruce.richardson@intel.com, pablo.de.lara.guarch@intel.com,
	dev@dpdk.org,  doucette@bu.edu, keith.wiles@intel.com,
	sameh.gobriel@intel.com, charlie.tai@intel.com,
	stephen@networkplumber.org, nd@arm.com,
	honnappa.nagarahalli@arm.com, yipeng1.wang@intel.com
Subject: Re: [dpdk-dev] [PATCH v3] hash table: add an iterator over conflicting entries
Date: Tue, 4 Sep 2018 14:46:35 -0400
Message-ID: <6f83afbe-a033-8ff8-cb49-5f58090aafdd@digirati.com.br> (raw)
In-Reply-To: <20180831225318.dumiys3sxgadv6of@bidouze.vm.6wind.com>

Hi Gaëtan,

On 08/31/2018 06:53 PM, Gaëtan Rivet wrote:
> Hi Qiaobin,
> 
> This work seems interesting, but is difficult to follow because
> the previous discussion is not referenced.
> 
> You can find a how-to there:
> 
> http://doc.dpdk.org/guides/contributing/patches.html#sending-patches
> 
> --in-reply-to is useful to check which comments were already made and
> understand the work previously done on a patchset.

    Thanks for bringing this to our attention.

>> +/* istate stands for internal state. */
> 
> Is a name requiring a comment to explain a good name?
> Maybe rte_hash_iterator_priv?
> 
>> +struct rte_hash_iterator_istate {
>> +	const struct rte_hash *h;
>> +	uint32_t              next;
>> +	uint32_t              total_entries;
>> +};

    We agree that the suffix _priv is better.

> You should check that your private structure does not grow beyond
> the public one, using RTE_BUILD_BUG_ON(sizeof(priv) < sizeof(pub)) somewhere.

    We have overlooked the macro RTE_BUILD_BUG_ON(). We'll use it.

> "rte_hash_iterator_[i]state" seems unnecessarily verbose.
> The memory you are manipulating through this variable is already holding
> the state of your iterator. It is useless to append "_state".
> 
>      struct rte_hash_iterator_priv *state;
> 
> is also clear and reads better.
> On the other hand "h" is maybe not verbose enough. Why not "hash"?

    We'll keep the parameter name "state" and rename the variable 
"__state" to "it" as you suggest in a comment later in your email.

    About the variable "h", we are following the coding convention in 
the library. You can find plenty of examples of using "h" for a hash table.

> Also, please do not align field names in a structure. It forces
> future changes to either break the pattern or edit the whole structure
> when someone attempts to insert a field with a name that is too long.

    Your suggestion goes against the coding style of DPDK. See section 
"1.5.5. Structure Declarations" on the page:

https://doc.dpdk.org/guides-18.08/contributing/coding_style.html

>> +
>> +int32_t
>> +rte_hash_iterator_init(const struct rte_hash *h,
>> +	struct rte_hash_iterator_state *state)
>> +{
>> +	struct rte_hash_iterator_istate *__state;
> 
> Please do not use the "__" prefix to convey that
> you are using a private version of the structure.
> 
> You could use "istate" or "it", the common shorthand for
> iterator handles.

    We'll do it as explained before.

>>   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)
> 
> Why an empty first line of parameters here?
> 
> rte_hash_iterate(struct rte_hash_iterator_state *state,
>                   const void **key,
>                   void **data)
> 
> reads better.

   Okay.

>> +
>> +/* istate stands for internal state. */
>> +struct rte_hash_iterator_conflict_entries_istate {
> 
> I find "conflict_entries" awkward, how about
> 
> rte_hash_dup_iterator
> 
> instead? It is shorter and conveys that you will iterate duplicate
> entries.

    Yipeng Wang suggested the expression "conflict_entries" in his 
review of the first version of this patch. You find his suggestion here:

http://mails.dpdk.org/archives/dev/2018-August/109103.html

    I find the name "dup" misleading because it suggests that the 
returned entries have the same key or refer to the same object. For 
example, the file descriptor returned by dup(2) refers to the same file.

>> +	const struct rte_hash *h;
>> +	uint32_t              vnext;
>> +	uint32_t              primary_bidx;
>> +	uint32_t              secondary_bidx;
>> +};
>> +
>> +int32_t __rte_experimental
>> +rte_hash_iterator_conflict_entries_init_with_hash(const struct rte_hash *h,
> 
> rte_hash_dup_iterator_init() maybe?
> 
> Why is _with_hash mentioned here? Is it possible to initialize this kind
> of iterator without a reference to compare against? That this reference
> is an rte_hash is already given by the parameter list.
> 
> In any case, 49 characters for a name is too long.

    Honnappa Nagarahalli suggested adding the suffix "_with_hash" during 
his review of the second version of this patch. His argument went as 
follows: "Let me elaborate. For the API 'rte_hash_lookup', there are 
multiple variations such as 'rte_hash_lookup_with_hash', 
'rte_hash_lookup_data', 'rte_hash_lookup_with_hash_data' etc. We do not 
need to create similar variations for 
'rte_hash_iterate_conflict_entries' API right now. But the naming of the 
API should be such that these variations can be created in the future."

    You find a copy of his original message here:

https://www.mail-archive.com/dev@dpdk.org/msg109653.html

>> +int32_t __rte_experimental
>> +rte_hash_iterate_conflict_entries(
>> +	struct rte_hash_iterator_state *state, const void **key, void **data)
> 
> How about "rte_hash_dup_next()"?
> Also, please break the parameter list instead of having an empty first
> line.

    We are preserving the name convention used in DPDK (e.g. 
rte_hash_iterate()).

    We'll break the parameter list to avoid the empty first line.

>> diff --git a/lib/librte_hash/rte_hash_version.map b/lib/librte_hash/rte_hash_version.map
>> index e216ac8e2..301d4638c 100644
>> --- a/lib/librte_hash/rte_hash_version.map
>> +++ b/lib/librte_hash/rte_hash_version.map
>> @@ -24,6 +24,7 @@ DPDK_2.1 {
>>   
>>   	rte_hash_add_key_data;
>>   	rte_hash_add_key_with_hash_data;
>> +	rte_hash_iterator_init;
>>   	rte_hash_iterate;
>>   	rte_hash_lookup_bulk_data;
>>   	rte_hash_lookup_data;
>> @@ -53,3 +54,10 @@ DPDK_18.08 {
>>   	rte_hash_count;
>>   
>>   } DPDK_16.07;
>> +
>> +EXPERIMENTAL {
>> +	global:
>> +
>> +	rte_hash_iterator_conflict_entries_init_with_hash;
>> +	rte_hash_iterate_conflict_entries;
>> +};
> 
> A blank line may be missing here.

    Do you mean a blank line before "};"?

    Thank you for the careful review.

[ ]'s
Michel Machado

  reply	other threads:[~2018-09-04 18:46 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 [this message]
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
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=6f83afbe-a033-8ff8-cb49-5f58090aafdd@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=gaetan.rivet@6wind.com \
    --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 \
    /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