Hi Jan,
In my module's case the situation is different . The read function in
the module puts to sleep the current process, waiting for some data on the
char buffer(a global variable in the module). Now this char buffer is
filled by another function in the same module, that is process independent,
it is a timer handler, which fills this buffer.
So, right I shld not say save_irq as it would disable interrupts and my
timer handler would never be called to fill that char buffer, resulting in
a deadlock, so i shld do this :
read(){
dont take any locks here, as the process might go to sleep using
wait_event_interruptible;
and this sleeping process wld then be awakened by timer1_expiry_func(),
which calls wake_up_interruptible.
/* process goes to sleep */
}
write(){
spin_lock not called right at the beginning, called after copy_fom_user has
returned some usable values.
spin_lock(&my_lock);
write to some module global variables; /*so hold this lock only while the
variable values are being assigned, the user process cannot go to sleep
here */
spin_unlock(&my_lock);
}
timer1_expiry_func() {
1. fill char buffer
2. wakeup interruptible
}
timer2_expiry_func() {
1. does not fill (touch) thatt char buffer, so need not wake anything, but
in general i should not lock timer1 & timer 2 expiry funcs using spinlocks
}
netfilter_hook_funcs(){
1. module global variables as set in write are used here
2. so do a spin_lock(&my_lock) when this starts
3. and do a spin_unlock(&my_lock) when this ends
}
I hope this looks better. Thanks for the comments Jan, actually I have
tried reading rubini, though my understanding is still a bit wobbly now.
any further comments ?
Amit
Jan Hudec <bulb@ucw.cz>@vagabond.light.src> on 06/10/2004 03:51:18 AM
Sent by: Jan Hudec <bulb@vagabond.light.src>
To: Amit Kumar Singh/HSS@HSS
cc: Kernel Newbies <kernelnewbies@nl.linux.org>, Sven Schuster
<schuster.sven@gmx.de>
Subject: Re: race conditions
You must never, ever, under penalty of complete lockup, sleep with
spin-lock held. The fact, that the spinlock is an irqsave/irqrestore
one only makes that WORSE and will crash even on UP! (If you disabled
interrupts, who is going to schedule you back?)
It is also imperative, that the flags for irqsave/irqrestore live on the
stack!
It seems you don't get what _irqsave/_irqrestore means. spin_lock_irqsave
saves CPU flags to provided variable and clear interrupt flag. The
spin_unlock_irqrestore restores CPU flags from that variable.
I suggest you read some book about linux kernel programming -- the
locking is tricky.
On Wed, Jun 09, 2004 at 20:29:17 +0530, aksingh@hss.hns.com wrote:
> I am giving here the mechanism I intend to use in my module to avoid
> race conditions. I use kernel 2.4.20-8, on a uniprocessor system. Please
> suggest if you find any problems with it.
>
> [snap]
>
> /* two global variables */
> spinlock_t my_lock=SPIN_LOCK_UNLOCKED;
> unsigned long my_flags;
>
> pre_routing() {
>
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER *******/
>
> spin_unlock_irqrestore(&my_lock, flags);
> }
>
> local_out() {
>
> /*** I USE THE SAME lock and flags variable in al functions ****/
Bad, bad, very bad!
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER ******/
>
> spin_unlock_irqrestore(&my_lock, flags);
> }
>
> Timer1_expiry(){
>
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER ******/
>
> spin_unlock_irqrestore(&my_lock, flags);
>
>
> }
>
> timer2_expiry() {
>
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER ******/
>
> spin_unlock_irqrestore(&my_lock, flags);
>
>
> }
>
> read() {
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER, uses copy_to_user ******/
> /*** the current process might go to sleep here when theres nothing to
> read, should not matter since I am using irqsave ***/
It does a HELL LOT matter. It's a bug. A fatal one. With spin_lock often
fatal, with spin_lock_irqsave always fatal.
> spin_unlock_irqrestore(&my_lock, flags);
> }
>
> block() {
> /*... the block call for the char device(called by read), when there is
> nothing to read */
> return wait_event_interruptible(...);
> }
>
> write(){
> spin_lock_irqsave(&my_lock, flags);
>
> /****** PROCESS WHATEVER, uses copy_from_user .. ******/
> /*** process might go to sleep if the user variable from which data is
> being written is swapped out, shld not matter since I am using irqsave
> ****/
>
> spin_unlock_irqrestore(&my_lock, flags);
> }
>
> poll() {
> havent thought about this ;
> }
>
>
> So How does it look, any obvious flaws ?, something I can do better.
>
> Any suggestions are welcome.
-------------------------------------------------------------------------------
Jan 'Bulb' Hudec <bulb@ucw.cz>
Attachment:
signature.asc
Description: Binary data
