LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Alex Elder <elder@linaro.org>
To: Johan Hovold <johan@kernel.org>, Matthew Wilcox <willy@infradead.org>
Cc: Alex Elder <elder@kernel.org>,
	linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
	greybus-dev@lists.linaro.org,
	"Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Subject: Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Mon, 30 Aug 2021 08:20:25 -0500	[thread overview]
Message-ID: <f7a25eb1-20f4-5031-a156-9e5dc019ad28@linaro.org> (raw)
In-Reply-To: <YSzQAd0Rg5CF/eLe@hovoldconsulting.com>

On 8/30/21 7:33 AM, Johan Hovold wrote:
> On Mon, Aug 30, 2021 at 01:16:07PM +0100, Matthew Wilcox wrote:
>> On Mon, Aug 30, 2021 at 01:52:48PM +0200, Johan Hovold wrote:
>>> Whether the API is better is debatable. As I said, almost no drivers use
>>> the new XArray interface, and perhaps partly because the new interface
>>> isn't as intuitive as has been claimed (e.g. xa_load() instead of
>>> ida_find()). And IDR/IDA isn't marked/documented as deprecated as far as
>>> I know.
>>
>> I can't just slap a 'deprecated' attribute on it.  That'll cause a
>> storm of warnings.  What would you suggest I do to warn people that
>> this interface is deprecated and I would like to remove it?
> 
> I'd at least expect a suggestion in the IDR documentation to consider
> using XArray instead.
> 
>> Why do you think that idr_find() is more intuitive than xa_load()?
>> The 'find' verb means that you search for something.  But it doesn't
>> search for anything; it just returns the pointer at that index.
>> 'find' should return the next non-NULL pointer at-or-above a given
>> index.
> 
> We're looking up a minor number which may or may not exist. "Find" (or
> "lookup" or "search") seems to describe this much better than "load"
> (even if that may better reflect the implementation of XArray).
> 
> And no, I would not expect a find implementation to return the next
> entry if the requested entry does not exist (and neither does idr_find()
> or radix_tree_lookup())

For what it's worth, I think of "find" as meaning "look up this one
thing, and return it if it's there (or tell me if it's not)."  That
is irrespective of underlying implementation.

That verb sometimes might mean something else (like create it if it
doesn't exist, or perhaps "get it or the next available" as suggested)
but I wish we had a slightly different naming convention for those
things.

The XArray interface was designed to better match typical usage of
IDR/IDA, as I understand it.  And its name suggests it is modeled
as an array, so "load" seems like a reasonable verb to mean returning
the value associated with an identified entry.  (Even though the
"doesn't exist" part doesn't match normal array semantics.)

So I guess I agree in part with both Johan and Matthew on the
 meaning
of "load" as used in the XArray interface.  Either way, that *is* the
interface at the moment.


I have been offering review feedback on this patch for three reasons:

- First, because I think the intended change does no real harm to the

  Greybus code, and in a small way actually simplifies it.

- Because I wanted to encourage Fabio's efforts to be part of the

  Linux contributor community.

- Because I suspected that Matthew's long-term intention was to

  replace IDR/IDA use with XArray, so this would represent an early

  conversion.



The Greybus code is generally good, but that doesn't mean it can't

evolve.  It gets very little patch traffic, so I don't consider small

changes like this "useless churn."  The Greybus code is held up as
 an
example of Linux kernel code that can be safely modified, and I
 think
it's actively promoted as a possible source of new developer
 tasks
(e.g. for Outreachy).



So aside from the details of the use of XArray, I'd rather we be
more open to changes to the Greybus code.

					-Alex

> 
> Johan
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 


  parent reply	other threads:[~2021-08-30 13:20 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-29  9:22 Fabio M. De Francesco
2021-08-30  9:12 ` Johan Hovold
2021-08-30 11:10   ` Fabio M. De Francesco
2021-08-30 11:52     ` Johan Hovold
2021-08-30 12:16       ` Matthew Wilcox
2021-08-30 12:33         ` Johan Hovold
2021-08-30 13:16           ` Fabio M. De Francesco
2021-08-30 13:20           ` Alex Elder [this message]
2021-08-31  8:07             ` [greybus-dev] " Johan Hovold
2021-08-31 10:42               ` Alex Elder
2021-08-31 11:51                 ` Johan Hovold
2021-08-31 11:50               ` Fabio M. De Francesco
2021-08-31 12:18                 ` Johan Hovold
2021-09-01 12:09                 ` Alex Elder
2021-09-01 13:56                   ` Fabio M. De Francesco
2021-09-01 14:29                     ` Matthew Wilcox
2021-09-01 15:39                       ` Fabio M. De Francesco
2021-08-30 13:31           ` Matthew Wilcox
2021-08-31  8:16             ` Johan Hovold

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=f7a25eb1-20f4-5031-a156-9e5dc019ad28@linaro.org \
    --to=elder@linaro.org \
    --cc=elder@kernel.org \
    --cc=fmdefrancesco@gmail.com \
    --cc=greybus-dev@lists.linaro.org \
    --cc=johan@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=willy@infradead.org \
    --subject='Re: [greybus-dev] [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray' \
    /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

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).