Re: [PATCH 10/18] rhashtable: remove rhashtable_walk_peek()

From: NeilBrown
Date: Sun Jun 03 2018 - 22:09:28 EST


On Sun, Jun 03 2018, Tom Herbert wrote:

> On Sun, Jun 3, 2018 at 5:30 PM, NeilBrown <neilb@xxxxxxxx> 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@xxxxxxxx>
>>>
>>> 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.

Thanks,
NeilBrown

Attachment: signature.asc
Description: PGP signature