Re: LinuxPPS & spinlocks

[Rodolfo Giometti <giometti@enneenne.com>]

On Mon, Jul 30, 2007 at 09:49:20AM +0530, Satyam Sharma wrote:
> 
> Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore()
> in pps_event() around the access to pps_source.

In pps_event() is not useful using spin_lock_irqsave/restore() since
the only difference between spin_lock_irqsave() and spin_lock() is
that the former will turn off interrupts if they are on, otherwise
does nothing (if we are already in an interrupt handler).

Maybe you meant I should using spin_lock_irqsave/restore() in user
context, but doing like this I will disable interrupts and I don't
wish doing it since, in this manner, the interrupt handler will be
delayed and the (probably) PPS event recording will be wrong. I
prefere loosing the event that registering it at delayed time.

> > About using both mutex and spinlock I did it since (I think) I should
> > protect syscalls from each others and from pps_register/unregister(),
> > and pps_event() against pps_register/unregister().
> 
> Nopes, it's not about protecting code from each other, you're needlessly
> complicating things. Locking is pretty simple, really -- any shared data,
> that can be concurrently accessed by multiple threads (or from interrupts)
> must be protected with a lock. Note that *data* is protected by a lock,
> and not "code" that handles it (well, this is the kind of behaviour most
> cases need, at least, including yours).

Of course, I meant "protecting data". In fact to protect pps_source[]
I need spin_lock() to protect user context from interrupt context and
mutex to protect user context from itself.

> So here we're introducing the lock to protect *pps_source*, and not keep
> *threads* of execution from stepping over each other. So, simply, just
> ensure you grab the lock whenever you want to start accessing the shared
> data, and release it when you're done.

I see. But consider pps_register_source(). This function should
provide protection of pps_source against both interrupt context
(pps_event()) and user context (maybe pps_unregister_source() or one
syscalls). Using only mutex is not possible, since we cannot use mutex
in interrupt context, and using only spin_locks is not possible since
in UP() they became void.

Can you please show me how I could write pps_register_source() in
order to be correct from your point of view?

> The _irqsave/restore() variants are required because (say) one of the
> syscalls executing in process context grabs the spinlock. Then, before it
> has released it, it gets interrupted and pps_event() begins executing.
> Now pps_event() also wants to grab the lock, but the syscall already
> has it, so will continue spinning and deadlock!

That's the point. I don't wish using _irqsave/restore() since they may
delay interrupt handler execution. As above, I prefere loosing the
event then registering it at wrong time.

> I think you're unnecessarily worrying about contention here -- you can
> have multiple locks (one for the list, and separate ones for your sources)
> if you're really worrying about contention -- or probably rwlocks. But
> really, rwlocks would end up being *slower* than spinlocks, unless the
> contention is really heavy and it helps to keep multiple readers in the
> critical section. But frankly, with at max a few (I'd expect generally
> one) PPS sources ever to be connected / registered with teh system, and
> just one-pulse-per-second, I don't see why any contention is ever gonna
> happen.

Why you wish using one lock per sources? Just one lock for the
list/array is not enought? :-o

Ciao,

Rodolfo

-- 

GNU/Linux Solutions                  e-mail:    giometti@enneenne.com
Linux Device Driver                             giometti@gnudd.com
Embedded Systems                     		giometti@linux.it
UNIX programming                     phone:     +39 349 2432127