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).".
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.
[...]
You can't just drop the locking here since you'd introduce a potential use-after-free in case gb_tty is freed after the lookup but before the port reference is taken.
That said, this driver is already broken since it can currently free the gb_tty while there are references to the port. I'll try to fix it up...
return gb_tty; }
But as you may have gathered, I don't think doing these conversions is a good idea.
Johan
Thanks for your review,
Fabio