[SMP]: Big Kernel Lock vs ISR_Disable

[Sebastian Huber]

Hello!

On 09/12/2011 08:25 PM, Marta Rybczynska wrote:
[...]
> Normally the code looks like this:
> --- git/cpukit/libfs/src/devfs/devfs_mknod.c    2011-08-22
> 21:10:06.230348773 +0200
> +++ rtems/cpukit/libfs/src/devfs/devfs_mknod.c  2011-08-22
> 21:13:41.265537357 +0200
> @@ -69,11 +69,13 @@ int devFS_mknod(
>         rtems_set_errno_and_return_minus_one( ENOMEM );
>
>     _ISR_Disable(level);
> +  _Lock_Critical_Section();
>     device_name_table[slot].device_name  = (char *)path;
>     device_name_table[slot].device_name_length = strlen(path);

Out of context: the strlen() inside the critical section is bad.

>     device_name_table[slot].major = major;
>     device_name_table[slot].minor = minor;
>     device_name_table[slot].mode  = mode;
> +  _Unlock_Critical_Section();
>     _ISR_Enable(level);

Does the _ISR_Disable/Enable still disable/enable interrupts on the executing 
core?  I suppose the lock used in _Lock/Unlock_Critical_Section uses a spin 
lock and thus disables also the interrupts on the executing core?  Is it a fair 
lock?

Suppose the _ISR_Disable() still disables interrupts on the executing core, 
then the maximum length of disabled interrupts on a single core depends on all 
other cores and in particular the fairness of the lock.  I think this is very 
bad for a real-time system.

>
>     return 0;
>
> In this case, it seems natural to merge _ISR_Disable and
> _Lock_Critical_Section into one function.

Yes, definitely.

>
> However, we do have more complicated cases:
> --- git/cpukit/posix/src/mutexlocksupp.c        2011-09-01
> 14:13:52.455646968 +0200
> +++ rtems/cpukit/posix/src/mutexlocksupp.c      2011-09-10
> 18:50:03.379722079 +0200
> @@ -43,6 +43,8 @@ int _POSIX_Mutex_Lock_support(
>     Objects_Locations             location;
>     ISR_Level                     level;
>
> +  _Lock_Critical_Section();
> +
>     the_mutex = _POSIX_Mutex_Get_interrupt_disable( mutex,&location,
> &level );
>     switch ( location ) {
>
> @@ -62,7 +64,8 @@ int _POSIX_Mutex_Lock_support(
>       case OBJECTS_REMOTE:
>   #endif
>       case OBJECTS_ERROR:
> -      break;
> +       _Unlock_Critical_Section();
> +       break;
>     }
>
>     return EINVAL;
>
> Here we have a side-effect of the _POSIX_Mutex_Get_interrupt_disable
> function.

Can't we move this into the _Object_Get() or _Objects_Get_isr_disable()?  I 
think the auto-initialization in the _POSIX_Mutex_Get_interrupt_disable() can 
use its own critical section.

>
>
> The options we have are:
> 1. Add lock to _ISR_Disable/Enable
> This will break certain points, especially around _Objects_Get (the
> function exits in the critical section of Disable_Dispatch and it is up to
> the further code to release it, sometimes the code can sleep in the
> process).

Without much thinking I would prefer that option.  This is similar to the Giant 
lock in FreeBSD.

>
> 2. Add a level parameter to our
> _Lock_Critical_Section/_Unlock_Critical_Section functions and call SMP lock
> inside
> However, this will lead to unnecessary recursive interrupt disable

I suppose this may lead to potentially unbounded interrupt disable times.

>
> 3. Add an SMP lock variant that does not disable the interrupts

Is this possible?

Kind regards,
	Sebastian

-- 
Sebastian Huber, embedded brains GmbH

Address : Obere Lagerstr. 30, D-82178 Puchheim, Germany
Phone   : +49 89 18 90 80 79-6
Fax     : +49 89 18 90 80 79-9
E-Mail  : sebastian.huber at embedded-brains.de
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.