FIFO DSRs... the 2nd
Stefan Sommerfeld
sommerfeld@mikrom.de
Mon Apr 10 10:08:00 GMT 2006
Hi Sergei,
I don't think your implementation of call_pending_DSRs_inner() is save. You
cannot reset the dsr_list and dsr_list_tail entries before running the dsr
routines, because the irqs are activated while the dsr routines run, so
another irq with a dsr could happend. This will result in a change of the
dsr order and also dsr could be missed because the irq->next pointer is
changed. The dsr_list and dsr_list_tail pointer must be current while the
dsrs are running.
Bye...
BTW: I don't think you modified less files with your patch ;)
----- Original Message -----
From: "Sergei Organov" <osv@javad.com>
To: <ecos-patches@sources.redhat.com>
Cc: "Stefan Sommerfeld" <sommerfeld@mikrom.de>
Sent: Montag, 10. April 2006 08:28
Subject: Re: FIFO DSRs... the 2nd
> "Stefan Sommerfeld" <sommerfeld@mikrom.de> writes:
>> Hi,
>>
>> attached is the patch inclusive the changelog.
>
> Attached is modified patch that implements FIFO scheduling of DSRs and
> makes it the default.
>
> Compared to the original patch, less files are modified, more assertions
> are added, and FIFO variant of the call_pending_DSRs_inner() is
> optimized both for single DSR and for multiple DSRs in the list.
>
> In addition, two related minor fixes to the comments in the kernel tests
> are provided.
>
> Tested on ARM.
>
>
--------------------------------------------------------------------------------
> Index: packages/kernel/current/ChangeLog
> ===================================================================
> RCS file: /var/local/cvsroot/ecos/packages/kernel/current/ChangeLog,v
> retrieving revision 1.5
> diff -u -r1.5 ChangeLog
> --- packages/kernel/current/ChangeLog 10 Apr 2006 05:32:53 -0000 1.5
> +++ packages/kernel/current/ChangeLog 10 Apr 2006 06:02:36 -0000
> @@ -1,3 +1,21 @@
> +2006-04-10 Sergei Organov <osv@javad.com>
> +
> + Implement FIFO variant of scheduling of DSRs and make it the
> + default. This is reworked patch originally suggested by Stefan
> + Sommerfeld <sommerfeld@mikrom.com>.
> +
> + * cdl/interrupts.cdl (CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST): make it
> + cdl_component.
> + * cdl/interrupts.cdl (CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO):
> + new option for CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST.
> + * include/intr.hxx (class Cyg_Interrupt): new static variable
> + dsr_list_tail.
> + * src/intr/intr.cxx (call_pending_DSRs_inner): add
> + CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO variant.
> + (post_dsr): likewise.
> + * tests/intr0.cxx: fix comments to match actual option names.
> + * tests/kintr0.c: likewise.
> +
> 2006-03-27 Marco Cruz <marco@daruma.com.br>
>
> * include/thread.hxx: removed extra qualifier of
> Index: packages/kernel/current/cdl/interrupts.cdl
> ===================================================================
> RCS file:
> /var/local/cvsroot/ecos/packages/kernel/current/cdl/interrupts.cdl,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 interrupts.cdl
> --- packages/kernel/current/cdl/interrupts.cdl 7 Dec 2005 12:06:34 -0000
> 1.1.1.1
> +++ packages/kernel/current/cdl/interrupts.cdl 10 Apr 2006 05:34:11 -0000
> @@ -74,7 +74,7 @@
> # NOTE: the choice of list vs table should not be two separate
> # options. There is a single option which must have one of
> # two legal values.
> - cdl_option CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST {
> + cdl_component CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST {
> display "Use linked lists for DSRs"
> default_value 1
> implements CYGINT_KERNEL_INTERRUPTS_DSRS
> @@ -85,6 +85,23 @@
> requires that the kernel disable interrupts for a very short
> period of time outside interrupt handlers, but there is no
> possibility of a table overflow occurring."
> +
> + cdl_option CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO {
> + display "Schedule DSRs in FIFO order"
> + flavor bool
> + default_value 1
> + description "When this option is 1, DSRs are scheduled
> + in the natural FIFO (first in, first out) order,
> + otherwise they are scheduled in LIFO (last in, first
> + out) order. Applications should not rely on any
> + particular order of scheduling of DSRs. LIFO
> + scheduling is kept for backward compatibility only and
> + is not recommended as it may lead to high (up to 2
> + times higher then FIFO) IRQ-to-DSR latencies at some
> + (typically rare) conditions. If unsure, leave this to
> + be 1."
> + }
> +
> }
>
> cdl_component CYGIMP_KERNEL_INTERRUPTS_DSRS_TABLE {
> Index: packages/kernel/current/include/intr.hxx
> ===================================================================
> RCS file:
> /var/local/cvsroot/ecos/packages/kernel/current/include/intr.hxx,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 intr.hxx
> --- packages/kernel/current/include/intr.hxx 7 Dec 2005 12:06:34 -0000
> 1.1.1.1
> +++ packages/kernel/current/include/intr.hxx 10 Apr 2006 05:34:11 -0000
> @@ -195,11 +195,17 @@
> // next DSR in list
> Cyg_Interrupt* volatile next_dsr CYGBLD_ANNOTATE_VARIABLE_INTR;
>
> - // static list of pending DSRs
> + // head of static list of pending DSRs
> static Cyg_Interrupt* volatile dsr_list[CYGNUM_KERNEL_CPU_MAX]
> CYGBLD_ANNOTATE_VARIABLE_INTR;
> -
> -#endif
> +
> +# ifdef CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> + // tail of static list of pending DSRs
> + static Cyg_Interrupt* volatile dsr_list_tail[CYGNUM_KERNEL_CPU_MAX]
> +
> CYGBLD_ANNOTATE_VARIABLE_INTR;
> +# endif
> +
> +#endif // defined CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
>
> #ifdef CYGIMP_KERNEL_INTERRUPTS_CHAIN
>
> Index: packages/kernel/current/src/intr/intr.cxx
> ===================================================================
> RCS file:
> /var/local/cvsroot/ecos/packages/kernel/current/src/intr/intr.cxx,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 intr.cxx
> --- packages/kernel/current/src/intr/intr.cxx 7 Dec 2005 12:06:34 -0000
> 1.1.1.1
> +++ packages/kernel/current/src/intr/intr.cxx 10 Apr 2006 06:04:32 -0000
> @@ -137,6 +137,10 @@
>
> Cyg_Interrupt* volatile Cyg_Interrupt::dsr_list[CYGNUM_KERNEL_CPU_MAX];
>
> +# ifdef CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +Cyg_Interrupt* volatile
> Cyg_Interrupt::dsr_list_tail[CYGNUM_KERNEL_CPU_MAX];
> +# endif
> +
> #endif
>
> // -------------------------------------------------------------------------
> @@ -170,6 +174,35 @@
>
> #ifdef CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
>
> +# ifdef CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> + cyg_uint32 old_intr;
> + HAL_DISABLE_INTERRUPTS(old_intr);
> + Cyg_Interrupt* intr = dsr_list[cpu];
> + CYG_ASSERT(intr != 0, "No DSRs are pended");
> + dsr_list[cpu] = 0;
> + dsr_list_tail[cpu] = 0;
> + while(true)
> + {
> + cyg_count32 count = intr->dsr_count;
> + Cyg_Interrupt* next = intr->next_dsr;
> + intr->dsr_count = 0;
> + intr->next_dsr = 0;
> + HAL_RESTORE_INTERRUPTS(old_intr);
> +
> + CYG_ASSERT(intr->dsr != 0, "No DSR defined");
> + CYG_ASSERT(count > 0, "DSR posted but post count is zero");
> + intr->dsr(intr->vector, count, (CYG_ADDRWORD)intr->data);
> +
> + if (!next)
> + break;
> +
> + intr = next;
> + HAL_DISABLE_INTERRUPTS(old_intr);
> + }
> +
> +# else // ! defined CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> while( dsr_list[cpu] != NULL )
> {
> Cyg_Interrupt* intr;
> @@ -191,8 +224,10 @@
>
> }
>
> -#endif
> -
> +# endif // ! defined CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> +#endif // defined CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
> +
> };
>
> externC void
> @@ -245,17 +280,39 @@
>
> // Only add the interrupt to the dsr list if this is
> // the first DSR call.
> - // At present DSRs are pushed onto the list and will be
> - // called in reverse order. We do not define the order
> - // in which DSRs are called, so this is acceptable.
> -
> if( dsr_count++ == 0 )
> {
> +# ifdef CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> + // Add to the tail of the list.
> + Cyg_Interrupt* tail = dsr_list_tail[cpu];
> + dsr_list_tail[cpu] = this;
> + if( tail )
> + {
> + CYG_ASSERT( 0 != dsr_list[cpu] ,
> + "DSR list is not empty but its head is 0");
> + tail->next_dsr = this;
> + }
> + else
> + {
> + CYG_ASSERT( 0 == dsr_list[cpu] ,
> + "DSR list tail is 0 but its head is not");
> + dsr_list[cpu] = this;
> + }
> +
> +# else // ! defined CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> + // At present DSRs are pushed onto the list and will be called
> + // in reverse order. We do not define the order in which DSRs
> + // are called, so this is acceptable.
> next_dsr = dsr_list[cpu];
> dsr_list[cpu] = this;
> +
> +# endif // ! defined CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> +
> }
> -
> -#endif
> +
> +#endif // defined CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
>
> HAL_RESTORE_INTERRUPTS(old_intr);
> };
> Index: packages/kernel/current/tests/intr0.cxx
> ===================================================================
> RCS file:
> /var/local/cvsroot/ecos/packages/kernel/current/tests/intr0.cxx,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 intr0.cxx
> --- packages/kernel/current/tests/intr0.cxx 7 Dec 2005 12:06:34 -0000
> 1.1.1.1
> +++ packages/kernel/current/tests/intr0.cxx 10 Apr 2006 05:34:11 -0000
> @@ -46,8 +46,9 @@
> // Description: Very basic test of interrupt objects
> // Options:
> // CYGIMP_KERNEL_INTERRUPTS_DSRS_TABLE
> -// CYGIMP_KERNEL_INTERRUPTS_DSRS_TABLE_SIZE
> +// CYGNUM_KERNEL_INTERRUPTS_DSRS_TABLE_SIZE
> // CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
> +// CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> //####DESCRIPTIONEND####
>
> #include <pkgconf/kernel.h>
> Index: packages/kernel/current/tests/kintr0.c
> ===================================================================
> RCS file:
> /var/local/cvsroot/ecos/packages/kernel/current/tests/kintr0.c,v
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 kintr0.c
> --- packages/kernel/current/tests/kintr0.c 7 Dec 2005 12:06:34 -0000
> 1.1.1.1
> +++ packages/kernel/current/tests/kintr0.c 10 Apr 2006 05:34:11 -0000
> @@ -46,10 +46,10 @@
> // Description: Very basic test of interrupt objects
> // Options:
> // CYGIMP_KERNEL_INTERRUPTS_DSRS_TABLE
> -// CYGIMP_KERNEL_INTERRUPTS_DSRS_TABLE_MAX
> +// CYGNUM_KERNEL_INTERRUPTS_DSRS_TABLE_SIZE
> // CYGIMP_KERNEL_INTERRUPTS_DSRS_LIST
> +// CYGSEM_KERNEL_INTERRUPTS_DSRS_LIST_FIFO
> //####DESCRIPTIONEND####
> -*/
>
> #include <cyg/kernel/kapi.h>
> #include <cyg/hal/hal_intr.h>
>
More information about the Ecos-patches
mailing list