Re: LinuxPPS & spinlocks

Hi Rodolfo,


On Sun, 29 Jul 2007, Rodolfo Giometti wrote:

On Sat, Jul 28, 2007 at 02:17:24AM +0530, Satyam Sharma wrote:

I only glanced through the code, so could be wrong, but I noticed that
the only global / shared data you have in there is a global "pps_source"
array of pps_s structs. That's accessed / modified from the various
syscalls introduced in the API exported to userspace, as well as the
register/unregister/pps_event API exported to in-kernel client subsystems,
yes? So it looks like you need to introduce proper locking for it, simply
type-qualifying it as "volatile" is not enough.

However, I think you've introduced two locks for it. The syscalls (that
run in process context, obviously) seem to use a pps_mutex and
pps_event() seems to be using the pps_lock spinlock (because that
gets executed from interrupt context) -- and from the looks of it, the
register/unregister functions are using /both/ the mutex and spinlock (!)

This is right.

This isn't quite right, (in fact there's nothing to protect pps_event from
racing against a syscall), so you should use *only* the spinlock for
synchronization -- the spin_lock_irqsave/restore() variants, in fact.

We can't use the spin_lock_irqsave/restore() variants since PPS
sources cannot manage IRQ enable/disable. For instance, the serial
source doesn't manage IRQs directly but just uses it to record PPS
events. The serial driver manages the IRQ enable/disable, not the PPS
source which only uses the IRQ handler to records events.

Hmm? I still don't see why you can't introduce spin_lock_irqsave/restore()
in pps_event() around the access to pps_source.


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).

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.

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!


[ Also, have you considered making pps_source a list and not an array?
It'll help you lose a whole lot of MAX_SOURCES, pps_is_allocated, etc
kind of gymnastics in there, and you _can_ return a pointer to the
corresponding pps source struct from the register() function to the in-kernel
users, so that way you get to retain the O(1) access to the corresponding
source when a client calls into pps_event(), similar to how you're using the
array index presently. ]

I also noticed code like (from pps_event):

+ /* Try to grab the lock, if not we prefere loose the event... */
+ if (!spin_trylock(&pps_lock))
+ return;

which looks worrisome and unnecessary. That spinlock looks to be of
fine enough granularity to me, do you think there'd be any contention
on it? I /think/ you can simply make that a spin_lock().

This is due the fact I cannot manage IRQ enable/disable.

What I meant is that you could make it a proper spin_lock() --
or spin_lock_irqsave(), actually -- instead of the _trylock_ variant
that it currently is.

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.


Satyam
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • Re: + uml-sigwinch-handling-cleanup.patch added to -mm tree
    ... There's no particular problem with host system calls being under ... > I don't think have a problem being called under a spinlock. ... What in there is sensitive to being called under a lock? ... Doing syscalls under spinlocks is ...
    (Linux-Kernel)
  • [PATCH RFC 0/4] Paravirtual spinlocks
    ... The obvious first order effect is that while the VCPU is not running, ... the likelihood of being preempted while holding a lock is low. ... This means that the effective contention time of the lock is not ... but just normal spinlock level contention. ...
    (Linux-Kernel)
  • Re: LinuxPPS & spinlocks
    ... My point is that the lock should be used to protect specific data. ... would be more correct to say, "spinlock foo is taken because ... syscalls introduced in the API exported to userspace, ...
    (Linux-Kernel)
  • Re: [PATCH 2.6.28-rc4]lock_stat: Add "con-hungry" to show that how many person-time figh
    ... contention on the spinlock. ... of threads waiting for a spinlock/ a contention". ... The lockstat code should not ever need to know about lock implementation ...
    (Linux-Kernel)
  • [patch] generic rwsems
    ... This patch converts all architectures to a generic rwsem implementation, ... take a spinlock to take an uncontested rwsem) as a basis. ... better even for architectures that do atomics with hashed spinlocks. ... * lock for reading ...
    (Linux-Kernel)
Loading