[LinuxPPS] [PATCH 02/11] Streamline interrupt handling; get rid of a lock and idr_find()
Rodolfo Giometti
giometti at enneenne.com
Tue Feb 10 10:03:16 CET 2009
On Mon, Feb 09, 2009 at 09:46:31PM -0500, George Spelvin wrote:
> 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.
I added them to build up my locking scheme, so I'm curious to see how
you can substitute them without breaking the core. :)
> 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.
Ok.
> 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.
Nack.
I worked hard to make _all_ code 100% bugs free (even if some bugs may
still alive, eheheh ;) and now I don't want doing steps backward. So,
please, provide only 100% race free code.
> (I could make it totally safe using a seq_lock-like mechanism
> if necessary.)
Better. :)
>
> 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().
The question is: is it not possible in the serial case or in _every_
case? The PPS core must be 100% races free since it cannot (and it
shouldn't) know how the upper layers will use its methods.
> 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.
This still considers a particular implementation. You cannot provide a
core implementation of anything which delegates locking of its
internal data to the upper layers, if so your code is broken by
design!
Currently I propose for kernel inclusion the LinuxPPS core code only,
so please, we should focus our attention on it proving 100% bugs free
code on everycase of exported methods usage.
>
> >>
> >> 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.
Again, forgot the ldisc and any particular implementation. The
question is: does your locking scheme provide 100% race free code? You
cannot relay on which PPS client a user may implement. Users can be
evil! :)
> >> - 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. :-) :-) :-) :-)
The above sequence is needed for locking, so, in my case, "as soon as"
possible means "after lock acquiring". But, in your case, you can move
up the echo function call _expecially_ for PPS_ECHOCLEAR case. :)
>
> >> /* 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.
Please, split the patch. For git-log readability is better having
several patches whose solve a well defined problem.
> >> - err = copy_from_user(¶ms, uarg, sizeof(struct pps_kparams));
> >> + /* Sanity checks */
> >> + if (!uarg)
> >> + return -EINVAL;
> >> + err = copy_from_user(¶ms, 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.
If you wish adding "explanatory comments", please provide a patch
titled "explanatory comments". If your patch fixes some bugs or adds
new features you may add whatever comments you wish but related with
the change you are doing.
Apart those stuff, really do you consider "Bump refcount" an
"explanatory comment"? ;)
>
> > 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
I disagree. You explanation relays on ldisc implementation. Please,
check the code considering only the core and any possible case of
methods call.
> 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?
Let's fix up the races before doing anything else (as already said
other things are cosmetics) then provide a signle patch for each
logical change you wish to do.
Ciao,
Rodolfo
--
GNU/Linux Solutions e-mail: giometti at enneenne.com
Linux Device Driver giometti at linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
More information about the LinuxPPS
mailing list