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@xxxxxxxxxxxxxxxxxxxx/ 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@xxxxxxxxx> > > --- > > > > 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 _______________________________________________ greybus-dev mailing list greybus-dev@xxxxxxxxxxxxxxxx https://lists.linaro.org/mailman/listinfo/greybus-dev
|