LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
Cc: Alex Elder <elder@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	greybus-dev@lists.linaro.org, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org,
	Matthew Wilcox <willy@infradead.org>
Subject: Re: [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray
Date: Mon, 30 Aug 2021 13:52:48 +0200	[thread overview]
Message-ID: <YSzGkNfG6HOayiXi@hovoldconsulting.com> (raw)
In-Reply-To: <2879439.WEJLM9RBEh@localhost.localdomain>

[ Please wrap your mails at 72 columns or so. ]

On Mon, Aug 30, 2021 at 01:10:54PM +0200, Fabio M. De Francesco wrote:
> On Monday, August 30, 2021 11:12:28 AM CEST Johan Hovold wrote:
> > On Sun, Aug 29, 2021 at 11:22:50AM +0200, Fabio M. De Francesco wrote:
> > > Convert greybus/uart.c from IDR to XArray. The abstract data type XArray
> > > is more memory-efficient, parallelisable, and cache friendly. It takes
> > > advantage of RCU to perform lookups without locking. Furthermore, IDR is
> > > deprecated because XArray has a better (cleaner and more consistent) API.
> > 
> > Where does it say that IDR is deprecated? Almost all drivers use IDR/IDA
> > and its interfaces are straight-forward. In most cases we don't care
> > about a possible slight increase in efficiency either, and so also in
> > this case. Correctness is what matters and doing these conversions risks
> > introducing regressions.
> > 
> > And I believe IDR use XArray internally these days anyway.
> 
> Please read the following message by Matthew Wilcox for an
> authoritative response to your doubts about efficiency of XArray and
> IDR deprecation:

> https://lore.kernel.org/lkml/20210503182629.GE1847222@casper.infradead.org/
> 
> In particular he says that "[] The advantage of the XArray over the
> IDR is that it has a better API (and the IDR interface is
> deprecated).".

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.

Your commit message sounds like ad for something, and is mostly
irrelevant for the case at hand.

> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > > 
> > > v3->v4: 
> > > 	Remove mutex_lock/unlock around xa_load(). These locks seem to
> > > 	be unnecessary because there is a 1:1 correspondence between
> > > 	a specific minor and its gb_tty and there is no reference
> > > 	counting. I think that the RCU locks used inside xa_load()
> > > 	are sufficient to protect this API from returning an invalid
> > > 	gb_tty in case of concurrent access. Some more considerations 
> > > 	on this topic are in the following message to linux-kernel list:
> > > 	https://lore.kernel.org/lkml/3554184.2JXonMZcNW@localhost.localdomain/
> > 
> > This just doesn't make sense (and a valid motivation would need to go in
> > the commit message if there was one).
> 
> OK, I'll take your words on it. Alex Elder wrote that he guess the
> mutex_lock/unlock() around xa_load() are probably not necessary. As I
> said I don't yet have knowledge of this kind of topics, so I was just
> attempting to find out a reason why. Now I know that my v1 was correct
> in having those Mutexes as the original code with IDR had.

This is partly why doing such conversions is a bad idea in the first
place. You need to understand the code that you're changing.

You don't know the code and risk introducing bugs, we need to spend time
reviewing it, and in the end we gain close to nothing at best.

Johan

  reply	other threads:[~2021-08-30 11:53 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 [this message]
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           ` [greybus-dev] " Alex Elder
2021-08-31  8:07             ` 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=YSzGkNfG6HOayiXi@hovoldconsulting.com \
    --to=johan@kernel.org \
    --cc=elder@kernel.org \
    --cc=fmdefrancesco@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=greybus-dev@lists.linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-staging@lists.linux.dev \
    --cc=willy@infradead.org \
    --subject='Re: [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).