Message-ID: <YSzGkNfG6HOayiXi@hovoldconsulting.com>
Date:   Mon, 30 Aug 2021 13:52:48 +0200
From:   Johan Hovold <johan@...nel.org>
To:     "Fabio M. De Francesco" <fmdefrancesco@...il.com>
Cc:     Alex Elder <elder@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        greybus-dev@...ts.linaro.org, linux-staging@...ts.linux.dev,
        linux-kernel@...r.kernel.org, Matthew Wilcox <willy@...radead.org>
Subject: Re: [PATCH v4] staging: greybus: Convert uart.c from IDR to XArray

[ 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@...il.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

Powered by blists - more mailing lists