[LinuxPPS] [PATCH 02/11] Streamline interrupt handling; get rid of a lock and idr_find()

[George Spelvin]

First, thank you very much for the careful review!  I know it's
hard, and I'm sorry that I left some messes in there.

Yes, I really did two separate things here and, because they both affect
the same regions of code, I was too lazy to disentangle them; I lumped
them both under "pps_event streamlining".

(I actually did them both in the same editing session.)

1) Pass "struct pps_device *pps" around instead of "int source".

   This gets rid of a lot of idr_find() calls.  Getting rid of the one
   from the pps_event() interrupt handler seems like a particularly
   big win.

2) Get rid of pps->lock.

   This necessitates using a circular buffer for the timestamps and
   atomic updates of the sequence number, because pps_event can
   now happen in the middle of PPS_FETCH.

   As is, it's not 100% race free; if 5 pps_events arrive during the
   execution of one PPS_FETCH call, it could get a corrupted
   timestamp, but that seems a bit unlikely, even on a preemptible
   kernel.  Applications need to sanity-check the timestamps anyway
   because settimeofday() might have intervened.

   (I could make it totally safe using a seq_lock-like mechanism
   if necessary.)

Rodolfo Giometti <giometti at enneenne.com> wrote:
>>  static void pps_tty_close(struct tty_struct *tty)
>>  {
>> -	long id = (long) tty->disc_data;
>> +	struct pps_device *pps = tty->disc_data;
>> +	int id = pps->id;
>>  
>> -	pps_unregister_source(id);
>>  	n_tty_close(tty);
>> +	pps_unregister_source(pps);
>
> Why?

Looking at the code, I'm not sure if it can happen, but I didn't
want to deallocate pps until the reference to it (tty->disc_data)
was gone.

I'm not sure there's anything that can happen here, but it just
makes me feel safer.

>>  /* pps_event - register a PPS event into the system
>> - * @source: the PPS source ID
>> + * @pps: the pps_device structure
>>   * @ts: the event timestamp
>>   * @event: the event type
>>   * @data: userdef pointer
>> @@ -252,66 +245,72 @@ EXPORT_SYMBOL(pps_unregister_source);
>>   *
>>   * If an echo function is associated with the PPS source it will be called
>>   * as:
>> - *	pps->info.echo(source, event, data);
>> + *	pps->info.echo(pps->id, event, data);
>> + *
>> + * NOTE: This function does no locking.  The caller must ensure that
>> + * a) calls are serialized, and
>> + * b) do not occur after pps_deregister_source is called.
>> + * For current PPS clients, this is taken care of by:
>> + * serial: a) struct uart_port's lock field, b) ldisc refcount
>> + * ktimer: a) struct tvec_base's lock, b) del_timer_sync()
>>   */
>>  
>> -void pps_event(int source, struct pps_ktime *ts, int event, void *data)
>> +void
>> +pps_event(struct pps_device *pps, struct pps_ktime *ts, int event, void *data)
>>  {
>> -	struct pps_device *pps;
>> -	unsigned long flags;
>> +	int mode;
>> +	u32 seq;
>> +
>> +	if (!pps)
>> +		return;
>
> I don't understand your locking scheme here, what about if a
> pps_unregister_source() is executed after this point?

See the comment I added above; it's not possible.  For the serial case,
you can't call pps_tty_close() (and thus pps_unregister_source()) while
the ldisc refcount is non-zero, and it's incremented aorund every call
to pps_event().

uart_handle_dcd_change() calls tty_ldisc_ref() and _deref() around the
calls to the PPS event handler, and tty_ldisc_release() waits for the
reference count to go to zero before calling tty->ldisc.ops->close.

(It also calls getimeofday(), so the previous discussion about
NULL timespec pointers passed to dcd_change() is actually moot.)

The above is complete protection, and applies when changing ldiscs, but
when shutting down a port, things are even more careful.  To be precise,
in drivers/char/tty_io.c:tty_release_dev(), it calls tty->ops->close,
which is drivers/serial/serial_core.c:uart_close(), does a bunch of
cleanup, culminating in uart_shutdown(), which disables the interrupt and
calls synchronize_irq(), ensuring that pps_event() is not in progress.

Only well after that does tty_release_dev() call tty_ldisc_release()
that ultimately calls pps_unregister_source() (as described above).


For the ktimer case pps_ktimer_exit() calls del_timer_sync() (which
waits for pps_event to return) before calling pps_unregister_source.

>>  
>>  	if ((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0) {
>>  		printk(KERN_ERR "pps: unknown event (%x) for source %d\n",
>> -			event, source);
>> +			event, pps->id);
>>  		return;
>>  	}
>> -
>> -	pps = pps_get_source(source);
>
> Why are you removing pps_get_source() here and not pps_put_source()
> into pps_unregister_source()?

Because I don't need an additional reference; the one held by the tty,
acquired in pps_register_source() and released in pps_unregister_source()
is sufficient.

The refcount is now:
- 1 from the ldisc being in use, plus
- 1 from each open of /dev/ppsX.

>> -	if (!pps)
>> -		return;
>> +	event &= PPS_CAPTUREASSERT | PPS_CAPTURECLEAR;
>>  
>>  	pr_debug("PPS event on source %d at %llu.%06u\n",
>>  			pps->id, (unsigned long long) ts->sec, ts->nsec);
>>  
>> -	spin_lock_irqsave(&pps->lock, flags);
>> -
>> -	/* Must call the echo function? */
>> -	if ((pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
>> -		pps->info.echo(source, event, data);
>> -
>>  	/* Check the event */
>> -	pps->current_mode = pps->params.mode;
>> +	mode = pps->params.mode;
>>  	if (event & PPS_CAPTUREASSERT) {
>> +		if (mode & PPS_ECHOASSERT)
>> +			pps->info.echo(pps->id, PPS_CAPTUREASSERT, data);
>
> Keep in mind that RFC says that echo function should be called as soon
> as possible.

Yes, which is why I got rid of:
pps_get_source
	spin_lock_irqsave(&pps_idr_lock)
	idr_find
	atomic_inc(&pps->usage);
	spin_unlock_irqrestore
spin_lock_irqsave(&pps->lock)

...and you're complaining that I added one in-line conditional if()?

I think I'm offended. :-) :-) :-) :-)

>>  		/* We have to add an offset? */
>> -		if (pps->params.mode & PPS_OFFSETASSERT)
>> +		if (mode & PPS_OFFSETASSERT)
>>  			pps_add_offset(ts, &pps->params.assert_off_tu);
>>  
>>  		/* Save the time stamp */
>> -		pps->assert_tu = *ts;
>> -		pps->assert_sequence++;
>> +		seq = pps->assert_sequence + 1;
>> +		pps->assert_tu[seq % 4] = *ts;
>> +		wmb();
>> +		pps->assert_sequence = seq;
>
> Why are you doing like this? If not related with the patch's title,
> please split the patch.

This is the "get rid of a lock" thing.  Yes, I should explain it better.

>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>> @@ -78,8 +79,9 @@ static long pps_cdev_ioctl(struct file *file,
>>  		pr_debug("PPS_GETPARAMS: source %d\n", pps->id);
>>  
>>  		/* Return current parameters */
>> -		err = copy_to_user(uarg, &pps->params,
>> -						sizeof(struct pps_kparams));
>> +		spin_lock(&pps->lock);
>> +		err = copy_to_user(uarg, &pps->params, sizeof pps->params);
>> +		spin_unlock(&pps->lock);
>
> Here you are adding a lock, if not related with patch's title, please
> split patch.

Good point, sorry.  Ths idea, of course, is to get a consistent set of
parameters that doesn't race with a SETPARAMS.

Figuring out all the locking was a long editing session and I added a
few incodental cleanups while doing it.

>> -		err = copy_from_user(&params, uarg, sizeof(struct pps_kparams));
>> +		/* Sanity checks */
>> +		if (!uarg)
>> +			return -EINVAL;
>> +		err = copy_from_user(&params, uarg, sizeof params);
>
> copy_from_user() should already do the necessary checking. Again, this
> is not related to patch's title, please, split the patch.

Mea culpa; that was cargo-cult programming (i.e. I copied something that
I didn't understand); I now know better, and will fix it.

>> @@ -108,23 +113,21 @@ static long pps_cdev_ioctl(struct file *file,
>>  			return -EINVAL;
>>  		}
>>  
>> -		spin_lock_irq(&pps->lock);
>> -
>> -		/* Save the new parameters */
>> -		pps->params = params;
>> -
>>  		/* Restore the read only parameters */
>>  		if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
>>  			/* section 3.3 of RFC 2783 interpreted */
>>  			pr_debug("time format unspecified (%x)\n",
>>  								params.mode);
>> -			pps->params.mode |= PPS_TSFMT_TSPEC;
>> +			params.mode |= PPS_TSFMT_TSPEC;
>>  		}
>>  		if (pps->info.mode & PPS_CANWAIT)
>> -			pps->params.mode |= PPS_CANWAIT;
>> -		pps->params.api_version = PPS_API_VERS;
>> +			params.mode |= PPS_CANWAIT;
>> +		params.api_version = PPS_API_VERS;
>>  
>> -		spin_unlock_irq(&pps->lock);
>> +		/* Save the new parameters */
>> +		spin_lock(&pps->lock);
>> +		pps->params = params;
>> +		spin_unlock(&pps->lock);
>
> Ditto.

Same apology applies.  It was a drive-by cleanup while I was doing deep
hacking.  I'll split it out.

>> @@ -171,17 +176,14 @@ static long pps_cdev_ioctl(struct file *file,
>>  		}
>>  
>>  		/* Return the fetched timestamp */
>> -		spin_lock_irq(&pps->lock);
>> -
>> -		fdata.info.assert_sequence = pps->assert_sequence;
>> -		fdata.info.clear_sequence = pps->clear_sequence;
>> -		fdata.info.assert_tu = pps->assert_tu;
>> -		fdata.info.clear_tu = pps->clear_tu;
>> +		fdata.info.assert_sequence = seq1 = pps->assert_sequence;
>> +		fdata.info.clear_sequence = seq2 = pps->clear_sequence;
>> +		read_barrier_depends();
>> +		fdata.info.assert_tu = pps->assert_tu[seq1 % 4];
>> +		fdata.info.clear_tu = pps->clear_tu[seq2 % 4];
>
> Not related with patch's title, please split the patch.

This is the "get rid of a lock" thing.  Yes, I should probably split that
from the eliminatoin of idr_find(), but it is required for that.

>>  		fdata.info.current_mode = pps->current_mode;
>>  
>> -		spin_unlock_irq(&pps->lock);
>> -
>> -		err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));
>> +		err = copy_to_user(uarg, &fdata, sizeof fdata);
>>  		if (err)
>>  			return -EFAULT;
>>  
>> @@ -201,7 +203,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
>>  						struct pps_device, cdev);
>>  	int found;
>>  
>> -	found = pps_get_source(pps->id) != 0;
>> +	found = pps_get_source(pps->id) != 0;	/* Bump refcount */
>
> Ditto.

Um... okay, style question.  I'm given to adding comments as I'm
puzzling out code, to save the next person the trouble.  In this case,
the important side effect was buried in the middle of an expression,
and I didn't see it the first time.

Admittedly, it's the only patch to pps_cdev_open() and not strictly
related, but is it really that much clutter to add a comment here and
there to a patch?  Additional patches are are form of clutter, too,
and the alternative is to discourage explanatory comments.

> Please, solve (or explain) the locking issue during
> pps_unregister_source() and pps_event(), other things are cosmetics!
> :)

Hopefully, I've addressed that.  Would you like me to expand the
comment in the source code?  I tried to make it pretty clear the
first time, but apparently not clear enough.

I'll get rid of the unnecessary NULL tests and split the two
improvements apart.  Any other requests?