[ 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:

quoted

On Sun, Aug 29, 2021 at 11:22:50AM +0200, Fabio M. De Francesco wrote:

quoted

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/ (local)

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.

quoted

quoted

Signed-off-by: Fabio M. De Francesco <redacted>
---

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/ (local)

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