Re: [greybus-dev] [PATCH v3] staging: greybus: Convert uart.c from IDR to XArray

[Fabio M. De Francesco]

On Saturday, August 28, 2021 5:43:49 PM CEST Alex Elder wrote:
> On 8/16/21 2:50 PM, 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.
> > 
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@xxxxxxxxx>
> 
> I have one more comment, below.  Generally, I don't think it is
> important to make this change, but I think it's fine to switch
> to the newer XArray interface.  The result is a little simpler.

I agree that the result of using XArray is a little simpler and readable. As far as 
performance is regarded (memory-efficiency, cache friendliness, parallelization
improvements) I have to take for true the words of Matthew W.. Some time ago
I did a similar conversion for staging/unisys/visorhba after discussing with him 
on IRC; he confirmed that the driver would have got several benefits. This is why
I decided to do this work on staging/greybus too.

I cannot affirm the same for IDA to XArray conversions, since IDA are relatively
lighter and efficient than IDR. Unfortunately, I cannot profile such conversions
in order to prove/disprove they *really* gain on execution time and/or memory 
footprint.
> >  
> > []
> >
> >  static int gb_uart_receive_data_handler(struct gb_operation *op)
> >  {
> > @@ -341,8 +341,8 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
> >  {
> >  	struct gb_tty *gb_tty;
> >  
> > -	mutex_lock(&table_lock);
> > -	gb_tty = idr_find(&tty_minors, minor);
> > +	xa_lock(&tty_minors);
> 
> I'm basically new to using the XArray interface, but I
> don't think you really need the xa_lock()/xa_unlock()
> calls here.  You are not relying on reference counting
> to control when the allocated minor device numbers are
> freed, so I'm pretty sure you can simply call xa_load()
> to look up the gb_tty for the given minor device.

I haven't yet had time to understand how driver works. However,
I can attempt a response mostly due to logic than to a real knowledge
of how drivers work...

(1) I see that alloc_minor is called at "probe" (that I suppose it means
the the kernel "feels" that a new device has been added and so it should 
initialize it somehow and make it ready to operate properly - I hope
I'm not too far from the truth :)).

(2) I see that xa_alloc() finds an *unused* identifier and, if it succeeds, 
that identifier is used as the "minor". So, we have one minor per device
and that the same minor cannot be re-assigned to other devices. It also
should mean that there's no need for reference counting because that
"minor" is not shared.

(3) If the logic above is sound, we have a 1:1 correspondence between
minors and devices (max 16 gb_tty's) and therefore we don't need to lock 
tty_minors because concurrent code passes different minors to xa_load() 
which always returns different gb_tty's.

If the above argument is wrong I think I should read a book on device 
drivers for the first time. I have Greg's but I haven't yet opened it for
reading :) 

Thanks,

Fabio

> But please don't only take my word for it; investigate
> it for yourself, and if needed ask others about it so
> you're confident it's correct.  There is no harm in
> taking the lock, but if it's not needed, it would be
> nice to avoid it.
> 
> If you conclude the locks are necessary, just say so,
> and explain why, and I'll probably just accept it.
> Otherwise, please explain why you are sure they are
> not needed when you send version 4.  Thank you.
> 
> 					-Alex
> 
> 
> > +	gb_tty = xa_load(&tty_minors, minor);
> >  	if (gb_tty) {
> >  		mutex_lock(&gb_tty->mutex);
> >  		if (gb_tty->disconnected) {
> > @@ -353,19 +353,19 @@ static struct gb_tty *get_gb_by_minor(unsigned int minor)
> >  			mutex_unlock(&gb_tty->mutex);
> >  		}
> >  	}
> > -	mutex_unlock(&table_lock);
> > +	xa_unlock(&tty_minors);
> >  	return gb_tty;
> >  }