From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751537AbeFDWNF (ORCPT ); Mon, 4 Jun 2018 18:13:05 -0400 Received: from mail-wr0-f194.google.com ([209.85.128.194]:42910 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751230AbeFDWND (ORCPT ); Mon, 4 Jun 2018 18:13:03 -0400 X-Google-Smtp-Source: ADUXVKJZTZ0xgeCYsD7NuZBCmoeW8BhyuUiBl4PdvfkN4LVk4XiR1kZCRdaNEn63tWm4XNkewoEp8hZgnn11FGeShi4= MIME-Version: 1.0 In-Reply-To: References: <152782754287.30340.4395718227884933670.stgit@noble> <152782824964.30340.6329146982899668633.stgit@noble> <20180602154851.pfy4wryezuhxp76v@gondor.apana.org.au> <87y3fvpf40.fsf@notabene.neil.brown.name> <87sh63pakb.fsf@notabene.neil.brown.name> From: Tom Herbert Date: Mon, 4 Jun 2018 15:13:01 -0700 Message-ID: Subject: Re: [PATCH 10/18] rhashtable: remove rhashtable_walk_peek() To: Tom Herbert Cc: NeilBrown , Herbert Xu , Thomas Graf , Linux Kernel Network Developers , LKML Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jun 4, 2018 at 2:31 PM, Tom Herbert wrote: > On Sun, Jun 3, 2018 at 7:09 PM, NeilBrown wrote: >> On Sun, Jun 03 2018, Tom Herbert wrote: >> >>> On Sun, Jun 3, 2018 at 5:30 PM, NeilBrown 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 >>>>> >>>>> 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