From: Tom Herbert <tom@quantonium.net>
To: Tom Herbert <tom@herbertland.com>
Cc: NeilBrown <neilb@suse.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
Thomas Graf <tgraf@suug.ch>,
Linux Kernel Network Developers <netdev@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 10/18] rhashtable: remove rhashtable_walk_peek()
Date: Mon, 4 Jun 2018 15:13:01 -0700 [thread overview]
Message-ID: <CAPDqMeo0hV+-ijYPKhpzpHWVovYrS7s22tBfUZTfvXvNv_qpXw@mail.gmail.com> (raw)
In-Reply-To: <CALx6S35sf4oj=x73SfhUxi1e9-u4Stxgr9BAj5E7KKmbufntwg@mail.gmail.com>
On Mon, Jun 4, 2018 at 2:31 PM, Tom Herbert <tom@herbertland.com> wrote:
> On Sun, Jun 3, 2018 at 7:09 PM, NeilBrown <neilb@suse.com> wrote:
>> On Sun, Jun 03 2018, Tom Herbert wrote:
>>
>>> On Sun, Jun 3, 2018 at 5:30 PM, NeilBrown <neilb@suse.com> wrote:
>>>> On Sat, Jun 02 2018, Herbert Xu wrote:
>>>>
>>>>> On Fri, Jun 01, 2018 at 02:44:09PM +1000, NeilBrown wrote:
>>>>>> This function has a somewhat confused behavior that is not properly
>>>>>> described by the documentation.
>>>>>> Sometimes is returns the previous object, sometimes it returns the
>>>>>> next one.
>>>>>> Sometimes it changes the iterator, sometimes it doesn't.
>>>>>>
>>>>>> This function is not currently used and is not worth keeping, so
>>>>>> remove it.
>>>>>>
>>>>>> A future patch will introduce a new function with a
>>>>>> simpler interface which can meet the same need that
>>>>>> this was added for.
>>>>>>
>>>>>> Signed-off-by: NeilBrown <neilb@suse.com>
>>>>>
>>>>> Please keep Tom Herbert in the loop. IIRC he had an issue with
>>>>> this patch.
>>>>
>>>> Yes you are right - sorry for forgetting to add Tom.
>>>>
>>>> My understanding of where this issue stands is that Tom raised issue and
>>>> asked for clarification, I replied, nothing further happened.
>>>>
>>>> It summary, my position is that:
>>>> - most users of my new rhashtable_walk_prev() will use it like
>>>> rhasthable_talk_prev() ?: rhashtable_walk_next()
>>>> which is close to what rhashtable_walk_peek() does
>>>> - I know of no use-case that could not be solved if we only had
>>>> the combined operation
>>>> - BUT it is hard to document the combined operation, as it really
>>>> does two things. If it is hard to document, then it might be
>>>> hard to understand.
>>>>
>>>> So provide the most understandable/maintainable solution, I think
>>>> we should provide rhashtable_walk_prev() as a separate interface.
>>>>
>>> I'm still missing why requiring two API operations instead of one is
>>> simpler or easier to document. Also, I disagree that
>>> rhashtable_walk_peek does two things-- it just does one which is to
>>> return the current element in the walk without advancing to the next
>>> one. The fact that the iterator may or may not move is immaterial in
>>> the API, that is an implementation detail. In fact, it's conceivable
>>> that we might completely reimplement this someday such that the
>>> iterator works completely differently implementation semantics but the
>>> API doesn't change. Also the naming in your proposal is confusing,
>>> we'd have operations to get the previous, and the next next object--
>>> so the user may ask where's the API to get the current object in the
>>> walk? The idea that we get it by first trying to get the previous
>>> object, and then if that fails getting the next object seems
>>> counterintuitive.
>>
>> To respond to your points out of order:
>>
>> - I accept that "rhashtable_walk_prev" is not a perfect name. It
>> suggests a stronger symmetry with rhasthable_walk_next than actually
>> exist. I cannot think of a better name, but I think the
>> description "Return the previously returned object if it is
>> still in the table" is clear and simple and explains the name.
>> I'm certainly open to suggestions for a better name.
>>
>> - I don't think it is meaningful to talk about a "current" element in a
>> table where asynchronous insert/remove is to be expected.
>> The best we can hope for is a "current location" is the sequence of
>> objects in the table - a location which is after some objects and
>> before all others. rhashtable_walk_next() returns the next object
>> after the current location, and advances the location pointer past
>> that object.
>> rhashtable_walk_prev() *doesn't* return the previous object in the
>> table. It returns the previously returned object. ("previous" in
>> time, but not in space, if you like).
>>
>> - rhashtable_walk_peek() currently does one of two different things.
>> It either returns the previously returned object (iter->p) if that
>> is still in the table, or it find the next object, steps over it, and
>> returns it.
>>
>> - I would like to suggest that when an API acts on a iterator object,
>> the question of whether or not the iterator is advanced *must* be a
>> fundamental question, not one that might change from time to time.
>>
>> Maybe a useful way forward would be for you to write documentation for
>> the rhashtable_walk_peek() interface which correctly describes what it
>> does and how it is used. Given that, I can implement that interface
>> with the stability improvements that I'm working on.
>>
>
> Here's how it's documented currently:
>
> "rhashtable_walk_peek - Return the next object but don't advance the iterator"
>
> I don't see what is incorrect about that. Peek returns the next object
> in the walk, however does not move the iterator past that object, so
> sucessive calls to peek return the same object. In other words it's a
> way to inspect the next object but not "consume" it. This is what is
> needed when netlink returns in the middle of a walk. The last object
> retrieved from the table may not have been processed completely, so it
> needs to be the first one processed on the next invocation to netlink.
>
> This is also easily distinguishable from
>
> "rhashtable_walk_next - Return the next object and advance the iterator"
>
> Where the only difference is that peek and walk is that, walk advances
> the iterator and peek does not. Hence why "peek" is a descriptive name
> for what is happening.
>
btw, we are using rhashtable_walk_peek with ILA code that hasn't been
upstreamed yet. I'll (re)post the patches shortly, this demonstates
why we need the peek functionality. If you think that
rhashtable_walk_peek is nothing more than an inline that does "return
rhashtable_walk_prev(iter) ? : rhashtable_walk_next(iter);" then maybe
we could redefine rhashtable_walk_peek to be that. But, then I'll ask
what the use case is for rhashtable_walk_prev as a standalone
function? We created rhashtable_walk_peek for the netlink walk problem
and I don't think any of the related use cases would ever call
rhashtable_walk_prev without the rhashtable_walk_next fallback.
Tom
> Tom
>
>> Thanks,
>> NeilBrown
next prev parent reply other threads:[~2018-06-04 22:13 UTC|newest]
Thread overview: 67+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-01 4:44 [RFC PATCH 00/18] Assorted rhashtable improvements NeilBrown
2018-06-01 4:44 ` [PATCH 11/18] rhashtable: further improve stability of rhashtable_walk NeilBrown
2018-06-01 4:44 ` [PATCH 17/18] rhashtable: rename rht_for_each*continue as *from NeilBrown
2018-06-01 4:44 ` [PATCH 10/18] rhashtable: remove rhashtable_walk_peek() NeilBrown
2018-06-02 15:48 ` Herbert Xu
2018-06-04 0:30 ` NeilBrown
2018-06-04 1:18 ` Tom Herbert
2018-06-04 2:09 ` NeilBrown
2018-06-04 21:31 ` Tom Herbert
2018-06-04 22:13 ` Tom Herbert [this message]
2018-06-05 1:24 ` NeilBrown
2018-06-05 1:00 ` NeilBrown
[not found] ` <CALx6S36Ce-rXQMzmFYZVPGD10Bo6udvRAHiZ5gWwnzVwoTVv0w@mail.gmail.com>
2018-06-06 5:07 ` NeilBrown
2018-06-07 2:45 ` [PATCH - RFC] rhashtable: add rhashtable_walk_last_seen() NeilBrown
2018-06-07 2:46 ` [PATCH - RFC] rhashtable: implement rhashtable_walk_peek() using rhashtable_walk_last_seen() NeilBrown
[not found] ` <CALx6S35GgUOd0dPgv7P96wNNTv5pN7fij0pcAoccqcSWZhvY7Q@mail.gmail.com>
2018-06-12 2:48 ` [PATCH RFC v2] " NeilBrown
2018-06-14 17:41 ` Tom Herbert
2018-06-15 4:23 ` Herbert Xu
2018-06-15 5:31 ` NeilBrown
2018-06-01 4:44 ` [PATCH 18/18] rhashtable: add rhashtable_walk_delay_rehash() NeilBrown
2018-06-01 4:44 ` [PATCH 14/18] rhashtable: allow rht_bucket_var to return NULL NeilBrown
2018-06-01 4:44 ` [PATCH 15/18] rhashtable: use bit_spin_locks to protect hash bucket NeilBrown
2018-06-02 5:03 ` Herbert Xu
2018-06-02 9:53 ` Eric Dumazet
2018-06-04 0:25 ` NeilBrown
2018-06-04 2:52 ` [PATCH 15a/18] rhashtables: add lockdep tracking to bucket bit-spin-locks NeilBrown
2018-06-04 18:16 ` Simon Horman
2018-06-04 21:37 ` NeilBrown
2018-06-01 4:44 ` [PATCH 01/18] rhashtable: silence RCU warning in rhashtable_test NeilBrown
2018-06-01 4:44 ` [PATCH 07/18] rhashtable: use cmpxchg() to protect ->future_tbl NeilBrown
2018-06-01 16:44 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 13/18] rhashtable: don't hold lock on first table throughout insertion NeilBrown
2018-06-01 4:44 ` [PATCH 16/18] rhashtable: allow percpu element counter NeilBrown
2018-06-01 4:44 ` [PATCH 03/18] rhashtable: remove nulls_base and related code NeilBrown
2018-06-07 2:49 ` NeilBrown
2018-06-13 6:25 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 08/18] rhashtable: clean up dereference of ->future_tbl NeilBrown
2018-06-01 16:54 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 06/18] rhashtable: simplify nested_table_alloc() and rht_bucket_nested_insert() NeilBrown
2018-06-01 16:28 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 09/18] rhashtable: use cmpxchg() in nested_table_alloc() NeilBrown
2018-06-01 4:44 ` [PATCH 05/18] rhashtable: simplify INIT_RHT_NULLS_HEAD() NeilBrown
2018-06-01 16:24 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 12/18] rhashtable: add rhashtable_walk_prev() NeilBrown
2018-06-01 4:44 ` [PATCH 02/18] rhashtable: split rhashtable.h NeilBrown
2018-06-01 10:48 ` Herbert Xu
2018-06-01 4:44 ` [PATCH 04/18] rhashtable: detect when object movement might have invalidated a lookup NeilBrown
2018-06-01 16:06 ` Herbert Xu
2018-06-04 3:38 ` NeilBrown
2018-07-06 7:08 ` [PATCH resend] " NeilBrown
2018-07-12 5:46 ` David Miller
2018-07-12 5:48 ` David Miller
2018-07-15 23:55 ` NeilBrown
2018-07-15 23:57 ` [PATCH - revised] " NeilBrown
2018-07-16 0:51 ` Herbert Xu
2018-07-16 1:23 ` NeilBrown
2018-07-16 2:16 ` Herbert Xu
2018-07-16 3:26 ` NeilBrown
2018-07-17 6:30 ` Herbert Xu
2018-07-20 6:24 ` NeilBrown
2018-07-18 20:14 ` David Miller
2018-07-20 6:30 ` NeilBrown
2018-07-20 6:43 ` David Miller
2018-07-20 7:09 ` NeilBrown
2018-07-23 1:56 ` [PATCH net-next] rhashtable: detect when object movement between tables " NeilBrown
2018-07-26 20:55 ` David Miller
2018-07-26 22:04 ` NeilBrown
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=CAPDqMeo0hV+-ijYPKhpzpHWVovYrS7s22tBfUZTfvXvNv_qpXw@mail.gmail.com \
--to=tom@quantonium.net \
--cc=herbert@gondor.apana.org.au \
--cc=linux-kernel@vger.kernel.org \
--cc=neilb@suse.com \
--cc=netdev@vger.kernel.org \
--cc=tgraf@suug.ch \
--cc=tom@herbertland.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).