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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
