lock_kernel() / unlock_kernel inconsistency

lock_kernel() / unlock_kernel inconsistency

[Jason Wohlgemuth]

In an effort to stay consistent with the community, I migrated some code 
to a driver to use the daemonize() routine in the function specified by 
the kernel_thread() call.

However, in looking at a few drivers in the system (drivers/usb/hub.c , 
drivers/md/md.c, drivers/media/video/msp3400.c), I noticed some 
inconsistencies.  Specifically with the use of lock_kernel() / 
unlock_kernel().

drivers/md/md.c looks like:
int md_thread(void * arg)
{
   md_lock_kernel();

   daemonize();
   .
   .
   .
   //md_unlock_kernel();
}

this is similiar to drivers/usb/hub.c (which doesn't call unlock_kernel 
following lock_kernel)

however drivers/media/video/msp3400.c looks like:
static int msp3400c_thread(void *data)
{
   .
   .
   .
#ifdef CONFIG_SMP
   lock_kernel();
#endif
   daemonize();
   .
   .
   .
#ifdef CONFIG_SMP
   unlock_kernel();
#endif
}

The latter example seems logically correct to me.  Does this imply that 
after the CPU that is responsible for starting the thread in md.c or 
hub.c claims the global lock it will never be released to any other CPU?

If I am incorrect here please just point out my error, however, I 
figured I would bring this to the mailing list's attention if in fact 
this is truely in error.

Thanks,
Jason

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

Re: lock_kernel() / unlock_kernel inconsistency Don't do this!

[george anzinger]

Jason Wohlgemuth wrote:
> 
> In an effort to stay consistent with the community, I migrated some code
> to a driver to use the daemonize() routine in the function specified by
> the kernel_thread() call.
> 
> However, in looking at a few drivers in the system (drivers/usb/hub.c ,
> drivers/md/md.c, drivers/media/video/msp3400.c), I noticed some
> inconsistencies.  Specifically with the use of lock_kernel() /
> unlock_kernel().
> 
> drivers/md/md.c looks like:
> int md_thread(void * arg)
> {
>    md_lock_kernel();
> 
>    daemonize();
>    .
>    .
>    .
>    //md_unlock_kernel();
> }
> 
> this is similiar to drivers/usb/hub.c (which doesn't call unlock_kernel
> following lock_kernel)
> 
> however drivers/media/video/msp3400.c looks like:
> static int msp3400c_thread(void *data)
> {
>    .
>    .
>    .
> #ifdef CONFIG_SMP
>    lock_kernel();
> #endif
>    daemonize();
>    .
>    .
>    .
> #ifdef CONFIG_SMP
>    unlock_kernel();
> #endif
> }
> 
> The latter example seems logically correct to me.  Does this imply that
> after the CPU that is responsible for starting the thread in md.c or
> hub.c claims the global lock it will never be released to any other CPU?
> 
> If I am incorrect here please just point out my error, however, I
> figured I would bring this to the mailing list's attention if in fact
> this is truely in error.

Both of these methods have problems, especially with the proposed
preemptions changes.  The first case causes the thread to run with the
BKL for the whole time.  This means that any other task that wants the
BKL will be blocked.  Surly the needed protections don't require this. 
These locks should be replaced with fine grain locking and once taken,
they should be released ASAP.

The second practice will not provide the needed protection in a
preemptable UP system.  The BKL on a UP is just a NOP anyway.  On the
other hand we want to use these lock points to disable preemption. 
Letting the defining code for the lock decide the SMP/UP issue allows
the preemption code to do the right thing.  This said, still, the BKL
should go away, see above.

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

Re: lock_kernel() / unlock_kernel inconsistency Don't do this!

[Alan Cox]

> Both of these methods have problems, especially with the proposed
> preemptions changes.  The first case causes the thread to run with the
> BKL for the whole time.  This means that any other task that wants the
> BKL will be blocked.  Surly the needed protections don't require this. 

The BKL is dropped on rescheduling of that task. Its an enforcement of the
old unix guarantees against other code making the same assumptions. Its also
the standard 2.4 locking for several things still


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

Re: lock_kernel() / unlock_kernel inconsistency Don't do this!

[george anzinger]

Alan Cox wrote:
> 
> > Both of these methods have problems, especially with the proposed
> > preemptions changes.  The first case causes the thread to run with the
> > BKL for the whole time.  This means that any other task that wants the
> > BKL will be blocked.  Surly the needed protections don't require this.
> 
> The BKL is dropped on rescheduling of that task. Its an enforcement of the
> old unix guarantees against other code making the same assumptions. Its also
> the standard 2.4 locking for several things still
> 
Yes, I am aware of the drop on schedule, but a preemptive schedule call
should (can not) do this.  Result, no preemption, i.e. the thread does
not let anyone else in.  Some how I don't think a long term hold, such
as this is needed.  Of course, if the code blocks (i.e. calls
schedule()) often... but then we find folks using such code a pattern
and learning tool.  Remember this thread was started by just such a
study.

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