[Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

In the latest generation of Atmel SoCs, the interrupt controller changed. Adapt
the existing code to take this into account.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
Hi,

This patch is not meant to be merged as is.

While enough to get the board to boot, it still has stability issues.

Whenever the system has booted, it can stay idle without any (spotted) issue.
However, as soon as we do something with the board, soon, the linux timer
doesn't get any interrupt anymore.

This has been tested with the program found at this URL:
http://git.free-electrons.com/training-materials/tree/lab-data/realtime/rttest/data/rttest.c

That basically just poll on the clock and sleeps.

Maxime

 arch/arm/mach-at91/at91sam926x_time.c |  2 --
 arch/arm/mach-at91/irq.c              | 47 +++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index 1f7ad837c05d..2a78267da445 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -274,8 +274,6 @@ void __init at91sam926x_pit_init(void)
 	pit_clkevt.mult = div_sc(pit_rate, NSEC_PER_SEC, pit_clkevt.shift);
 	pit_clkevt.cpumask = cpumask_of(0);
 	clockevents_register_device(&pit_clkevt);
-
-	at91_pic_muter_register();
 }
 
 void __init at91sam926x_ioremap_pit(u32 addr)
diff --git a/arch/arm/mach-at91/irq.c b/arch/arm/mach-at91/irq.c
index a18f229d5d27..f0be89b0ecba 100644
--- a/arch/arm/mach-at91/irq.c
+++ b/arch/arm/mach-at91/irq.c
@@ -202,7 +202,7 @@ static void at91_aic_mask_irq(struct irq_data *d)
 	hard_cond_local_irq_restore(flags);
 }
 
-static void __maybe_unused at91_aic5_mask_irq(struct irq_data *d)
+static inline void at91_aic5_hard_mask_irq(struct irq_data *d)
 {
 	/* Disable interrupt on AIC5 */
 	at91_aic_write(AT91_AIC5_SSR, d->hwirq & AT91_AIC5_INTSEL_MSK);
@@ -211,6 +211,16 @@ static void __maybe_unused at91_aic5_mask_irq(struct irq_data *d)
 	clear_backup(d->hwirq);
 }
 
+static void at91_aic5_mask_irq(struct irq_data *d)
+{
+	unsigned long flags;
+
+	flags = hard_cond_local_irq_save();
+	at91_aic5_hard_mask_irq(d);
+	ipipe_lock_irq(d->irq);
+	hard_cond_local_irq_restore(flags);
+}
+
 static inline void at91_aic_hard_unmask_irq(struct irq_data *d)
 {
 	/* Enable interrupt on AIC */
@@ -229,7 +239,7 @@ static void at91_aic_unmask_irq(struct irq_data *d)
 	hard_cond_local_irq_restore(flags);
 }
 
-static void __maybe_unused at91_aic5_unmask_irq(struct irq_data *d)
+static inline void at91_aic5_hard_unmask_irq(struct irq_data *d)
 {
 	/* Enable interrupt on AIC5 */
 	at91_aic_write(AT91_AIC5_SSR, d->hwirq & AT91_AIC5_INTSEL_MSK);
@@ -238,6 +248,16 @@ static void __maybe_unused at91_aic5_unmask_irq(struct irq_data *d)
 	set_backup(d->hwirq);
 }
 
+static void at91_aic5_unmask_irq(struct irq_data *d)
+{
+	unsigned long flags;
+
+	flags = hard_cond_local_irq_save();
+	at91_aic5_hard_unmask_irq(d);
+	ipipe_unlock_irq(d->irq);
+	hard_cond_local_irq_restore(flags);
+}
+
 static void at91_aic_eoi(struct irq_data *d)
 {
 	/*
@@ -247,6 +267,11 @@ static void at91_aic_eoi(struct irq_data *d)
 	at91_aic_write(AT91_AIC_EOICR, 0);
 }
 
+static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
+{
+	at91_aic_write(AT91_AIC5_EOICR, 0);
+}
+
 #ifdef CONFIG_IPIPE
 static void at91_aic_hold_irq(struct irq_data *d)
 {
@@ -258,13 +283,20 @@ static void at91_aic_release_irq(struct irq_data *d)
 {
 	at91_aic_hard_unmask_irq(d);
 }
-#endif /* CONFIG_IPIPE */
 
-static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
+static void at91_aic5_hold_irq(struct irq_data *d)
 {
-	at91_aic_write(AT91_AIC5_EOICR, 0);
+	at91_aic5_hard_mask_irq(d);
+	at91_aic5_eoi(d);
 }
 
+static void at91_aic5_release_irq(struct irq_data *d)
+{
+	at91_aic5_hard_unmask_irq(d);
+}
+
+#endif /* CONFIG_IPIPE */
+
 static unsigned long *at91_extern_irq;
 
 u32 at91_get_extern_irq(void)
@@ -529,6 +561,11 @@ int __init at91_aic5_of_init(struct device_node *node,
 	at91_aic_chip.irq_eoi		= at91_aic5_eoi;
 	at91_aic_irq_ops.map		= at91_aic5_irq_map;
 
+#ifdef CONFIG_IPIPE
+	at91_aic_chip.irq_hold		= at91_aic5_hold_irq;
+	at91_aic_chip.irq_release	= at91_aic5_release_irq;
+#endif
+
 	err = at91_aic_of_common_init(node, parent);
 	if (err)
 		return err;
-- 
2.0.0

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

On 07/01/2014 12:27 PM, Maxime Ripard wrote:
> -
> -	at91_pic_muter_register();
>  }

Obviously, some if (soc_is_foo()) missing here.

> +static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
> +{
> +	at91_aic_write(AT91_AIC5_EOICR, 0);
> +}

You want to make that inline, so that the hold callback ends-up doing
just two register writes without any function calls, improving interrupt
latency.

> +
>  #ifdef CONFIG_IPIPE
>  static void at91_aic_hold_irq(struct irq_data *d)
>  {
> @@ -258,13 +283,20 @@ static void at91_aic_release_irq(struct irq_data *d)
>  {
>  	at91_aic_hard_unmask_irq(d);
>  }
> -#endif /* CONFIG_IPIPE */
>  
> -static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
> +static void at91_aic5_hold_irq(struct irq_data *d)
>  {
> -	at91_aic_write(AT91_AIC5_EOICR, 0);
> +	at91_aic5_hard_mask_irq(d);
> +	at91_aic5_eoi(d);
>  }
>  
> +static void at91_aic5_release_irq(struct irq_data *d)
> +{
> +	at91_aic5_hard_unmask_irq(d);
> +}

The ->release callback is called with irqs on, so you may want to call
hard_local_irq_save / hard_local_irq_restore to make it atomic (note
that the old at91s are also broken by the addition of the call to
set_backup). Alternatively, you may move the clear_backup/set_bakcup
calls to the linux mask/unmask routines, so that the register writes
remain atomic, and you can avoid the save/restore and function calls and
improve the interrupt latency.


-- 
                                                                Gilles.

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

Hi Gilles,

On Tue, Jul 01, 2014 at 12:59:51PM +0200, Gilles Chanteperdrix wrote:
> On 07/01/2014 12:27 PM, Maxime Ripard wrote:
> > -
> > -	at91_pic_muter_register();
> >  }
> 
> Obviously, some if (soc_is_foo()) missing here.

Right.

> > +static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
> > +{
> > +	at91_aic_write(AT91_AIC5_EOICR, 0);
> > +}
> 
> You want to make that inline, so that the hold callback ends-up doing
> just two register writes without any function calls, improving interrupt
> latency.
> 
> > +
> >  #ifdef CONFIG_IPIPE
> >  static void at91_aic_hold_irq(struct irq_data *d)
> >  {
> > @@ -258,13 +283,20 @@ static void at91_aic_release_irq(struct irq_data *d)
> >  {
> >  	at91_aic_hard_unmask_irq(d);
> >  }
> > -#endif /* CONFIG_IPIPE */
> >  
> > -static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
> > +static void at91_aic5_hold_irq(struct irq_data *d)
> >  {
> > -	at91_aic_write(AT91_AIC5_EOICR, 0);
> > +	at91_aic5_hard_mask_irq(d);
> > +	at91_aic5_eoi(d);
> >  }
> >  
> > +static void at91_aic5_release_irq(struct irq_data *d)
> > +{
> > +	at91_aic5_hard_unmask_irq(d);
> > +}
> 
> The ->release callback is called with irqs on, so you may want to call
> hard_local_irq_save / hard_local_irq_restore to make it atomic (note
> that the old at91s are also broken by the addition of the call to
> set_backup). Alternatively, you may move the clear_backup/set_bakcup
> calls to the linux mask/unmask routines, so that the register writes
> remain atomic, and you can avoid the save/restore and function calls and
> improve the interrupt latency.

Ok, so, with the changes you mentionned, I can't make the system crash
anymore (or at least, not as easily as it used to be).

But:
  - whenever the program mentionned above calls exit(), it
    stalls. However, ctrl+c makes the program exit properly, and
    everything seems fine otherwise
  - whenever we don't link it against xenomai, it just hangs. I've not
    figured out why yet

With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works fine.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140701/abc294fd/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/01/2014 04:15 PM, Maxime Ripard wrote:
> Hi Gilles,
> 
> On Tue, Jul 01, 2014 at 12:59:51PM +0200, Gilles Chanteperdrix
> wrote:
>> On 07/01/2014 12:27 PM, Maxime Ripard wrote:
>>> - -	at91_pic_muter_register(); }
>> 
>> Obviously, some if (soc_is_foo()) missing here.
> 
> Right.
> 
>>> +static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
>>> +{ +	at91_aic_write(AT91_AIC5_EOICR, 0); +}
>> 
>> You want to make that inline, so that the hold callback ends-up
>> doing just two register writes without any function calls,
>> improving interrupt latency.
>> 
>>> + #ifdef CONFIG_IPIPE static void at91_aic_hold_irq(struct
>>> irq_data *d) { @@ -258,13 +283,20 @@ static void
>>> at91_aic_release_irq(struct irq_data *d) { 
>>> at91_aic_hard_unmask_irq(d); } -#endif /* CONFIG_IPIPE */
>>> 
>>> -static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
>>> +static void at91_aic5_hold_irq(struct irq_data *d) { -
>>> at91_aic_write(AT91_AIC5_EOICR, 0); +
>>> at91_aic5_hard_mask_irq(d); +	at91_aic5_eoi(d); }
>>> 
>>> +static void at91_aic5_release_irq(struct irq_data *d) +{ +
>>> at91_aic5_hard_unmask_irq(d); +}
>> 
>> The ->release callback is called with irqs on, so you may want to
>> call hard_local_irq_save / hard_local_irq_restore to make it
>> atomic (note that the old at91s are also broken by the addition
>> of the call to set_backup). Alternatively, you may move the
>> clear_backup/set_bakcup calls to the linux mask/unmask routines,
>> so that the register writes remain atomic, and you can avoid the
>> save/restore and function calls and improve the interrupt
>> latency.
> 
> Ok, so, with the changes you mentionned, I can't make the system
> crash anymore (or at least, not as easily as it used to be).
> 
> But: - whenever the program mentionned above calls exit(), it 
> stalls. However, ctrl+c makes the program exit properly, and 
> everything seems fine otherwise - whenever we don't link it against
> xenomai, it just hangs. I've not figured out why yet
> 
> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works fine.

My answer was wrong, you probably need to keep the
set_backup/clear_backup calls in he ->hold and ->release callbacks, as
the linux interrupt may expect the backup areas to be in sync. Did you
do this, or did you go for the alternative?


- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTsw2WGpcgE6m/fboRAktjAJ0Q0VTNOiDoA8A9leqjBSe988weEgCeL+xY
CK/e7XIvKMVFtI7qi1B1vZM=
=6llR
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Tue, Jul 01, 2014 at 09:35:50PM +0200, Gilles Chanteperdrix wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 07/01/2014 04:15 PM, Maxime Ripard wrote:
> > Hi Gilles,
> > 
> > On Tue, Jul 01, 2014 at 12:59:51PM +0200, Gilles Chanteperdrix
> > wrote:
> >> On 07/01/2014 12:27 PM, Maxime Ripard wrote:
> >>> - -	at91_pic_muter_register(); }
> >> 
> >> Obviously, some if (soc_is_foo()) missing here.
> > 
> > Right.
> > 
> >>> +static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
> >>> +{ +	at91_aic_write(AT91_AIC5_EOICR, 0); +}
> >> 
> >> You want to make that inline, so that the hold callback ends-up
> >> doing just two register writes without any function calls,
> >> improving interrupt latency.
> >> 
> >>> + #ifdef CONFIG_IPIPE static void at91_aic_hold_irq(struct
> >>> irq_data *d) { @@ -258,13 +283,20 @@ static void
> >>> at91_aic_release_irq(struct irq_data *d) { 
> >>> at91_aic_hard_unmask_irq(d); } -#endif /* CONFIG_IPIPE */
> >>> 
> >>> -static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
> >>> +static void at91_aic5_hold_irq(struct irq_data *d) { -
> >>> at91_aic_write(AT91_AIC5_EOICR, 0); +
> >>> at91_aic5_hard_mask_irq(d); +	at91_aic5_eoi(d); }
> >>> 
> >>> +static void at91_aic5_release_irq(struct irq_data *d) +{ +
> >>> at91_aic5_hard_unmask_irq(d); +}
> >> 
> >> The ->release callback is called with irqs on, so you may want to
> >> call hard_local_irq_save / hard_local_irq_restore to make it
> >> atomic (note that the old at91s are also broken by the addition
> >> of the call to set_backup). Alternatively, you may move the
> >> clear_backup/set_bakcup calls to the linux mask/unmask routines,
> >> so that the register writes remain atomic, and you can avoid the
> >> save/restore and function calls and improve the interrupt
> >> latency.
> > 
> > Ok, so, with the changes you mentionned, I can't make the system
> > crash anymore (or at least, not as easily as it used to be).
> > 
> > But: - whenever the program mentionned above calls exit(), it 
> > stalls. However, ctrl+c makes the program exit properly, and 
> > everything seems fine otherwise - whenever we don't link it against
> > xenomai, it just hangs. I've not figured out why yet
> > 
> > With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works fine.
> 
> My answer was wrong, you probably need to keep the
> set_backup/clear_backup calls in he ->hold and ->release callbacks, as
> the linux interrupt may expect the backup areas to be in sync. Did you
> do this, or did you go for the alternative?

I went for the alternative. I'm sending you a v2 with what I have so
far, that shows the behaviour I was describing.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140704/4512e9dc/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/04/2014 11:27 AM, Maxime Ripard wrote:
> On Tue, Jul 01, 2014 at 09:35:50PM +0200, Gilles Chanteperdrix wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 07/01/2014 04:15 PM, Maxime Ripard wrote:
>>> Hi Gilles,
>>>
>>> On Tue, Jul 01, 2014 at 12:59:51PM +0200, Gilles Chanteperdrix
>>> wrote:
>>>> On 07/01/2014 12:27 PM, Maxime Ripard wrote:
>>>>> - -	at91_pic_muter_register(); }
>>>>
>>>> Obviously, some if (soc_is_foo()) missing here.
>>>
>>> Right.
>>>
>>>>> +static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
>>>>> +{ +	at91_aic_write(AT91_AIC5_EOICR, 0); +}
>>>>
>>>> You want to make that inline, so that the hold callback ends-up
>>>> doing just two register writes without any function calls,
>>>> improving interrupt latency.
>>>>
>>>>> + #ifdef CONFIG_IPIPE static void at91_aic_hold_irq(struct
>>>>> irq_data *d) { @@ -258,13 +283,20 @@ static void
>>>>> at91_aic_release_irq(struct irq_data *d) { 
>>>>> at91_aic_hard_unmask_irq(d); } -#endif /* CONFIG_IPIPE */
>>>>>
>>>>> -static void __maybe_unused at91_aic5_eoi(struct irq_data *d) 
>>>>> +static void at91_aic5_hold_irq(struct irq_data *d) { -
>>>>> at91_aic_write(AT91_AIC5_EOICR, 0); +
>>>>> at91_aic5_hard_mask_irq(d); +	at91_aic5_eoi(d); }
>>>>>
>>>>> +static void at91_aic5_release_irq(struct irq_data *d) +{ +
>>>>> at91_aic5_hard_unmask_irq(d); +}
>>>>
>>>> The ->release callback is called with irqs on, so you may want to
>>>> call hard_local_irq_save / hard_local_irq_restore to make it
>>>> atomic (note that the old at91s are also broken by the addition
>>>> of the call to set_backup). Alternatively, you may move the
>>>> clear_backup/set_bakcup calls to the linux mask/unmask routines,
>>>> so that the register writes remain atomic, and you can avoid the
>>>> save/restore and function calls and improve the interrupt
>>>> latency.
>>>
>>> Ok, so, with the changes you mentionned, I can't make the system
>>> crash anymore (or at least, not as easily as it used to be).
>>>
>>> But: - whenever the program mentionned above calls exit(), it 
>>> stalls. However, ctrl+c makes the program exit properly, and 
>>> everything seems fine otherwise - whenever we don't link it against
>>> xenomai, it just hangs. I've not figured out why yet
>>>
>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works fine.
>>
>> My answer was wrong, you probably need to keep the
>> set_backup/clear_backup calls in he ->hold and ->release callbacks, as
>> the linux interrupt may expect the backup areas to be in sync. Did you
>> do this, or did you go for the alternative?
> 
> I went for the alternative. I'm sending you a v2 with what I have so
> far, that shows the behaviour I was describing.

Could you try the following patch?

diff --git a/arch/arm/mach-at91/gpio.c b/arch/arm/mach-at91/gpio.c
index 8ef9c3e..ec539bc 100644
- --- a/arch/arm/mach-at91/gpio.c
+++ b/arch/arm/mach-at91/gpio.c
@@ -994,6 +994,10 @@ void __init at91_gpio_init(struct at91_gpio_bank *data, int nr_banks)
 }
 
 #if defined(CONFIG_IPIPE)
+extern unsigned long at91_aic_caps;
+#define AT91_AIC_CAP_AIC5	(1 << 0)
+#define has_aic5()		(at91_aic_caps & AT91_AIC_CAP_AIC5)
+
 static void at91_enable_irqdesc(struct ipipe_domain *ipd, unsigned irq)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
@@ -1089,6 +1093,9 @@ void at91_pic_muter_register(void)
 		.unmute = at91_unmute_pic,
 	};
 
+	if (has_aic5())
+		return;
+
 	ipipe_pic_muter_register(&at91_pic_muter);
 }
 #endif /* CONFIG_IPIPE */
diff --git a/arch/arm/mach-at91/irq.c b/arch/arm/mach-at91/irq.c
index a18f229..be13b65 100644
- --- a/arch/arm/mach-at91/irq.c
+++ b/arch/arm/mach-at91/irq.c
@@ -48,7 +48,7 @@ void __iomem *at91_aic_base;
 static struct irq_domain *at91_aic_domain;
 static struct device_node *at91_aic_np;
 static unsigned int n_irqs = NR_AIC_IRQS;
- -static unsigned long at91_aic_caps = 0;
+unsigned long at91_aic_caps = 0;
 
 /* AIC5 introduces a Source Select Register */
 #define AT91_AIC_CAP_AIC5	(1 << 0)
@@ -202,7 +202,7 @@ static void at91_aic_mask_irq(struct irq_data *d)
 	hard_cond_local_irq_restore(flags);
 }
 
- -static void __maybe_unused at91_aic5_mask_irq(struct irq_data *d)
+static inline void at91_aic5_hard_mask_irq(struct irq_data *d)
 {
 	/* Disable interrupt on AIC5 */
 	at91_aic_write(AT91_AIC5_SSR, d->hwirq & AT91_AIC5_INTSEL_MSK);
@@ -211,6 +211,16 @@ static void __maybe_unused at91_aic5_mask_irq(struct irq_data *d)
 	clear_backup(d->hwirq);
 }
 
+static void __maybe_unused at91_aic5_mask_irq(struct irq_data *d)
+{
+	unsigned long flags;
+
+	flags = hard_cond_local_irq_save();
+	at91_aic5_hard_mask_irq(d);
+	ipipe_lock_irq(d->irq);
+	hard_cond_local_irq_restore(flags);
+}
+
 static inline void at91_aic_hard_unmask_irq(struct irq_data *d)
 {
 	/* Enable interrupt on AIC */
@@ -229,7 +239,7 @@ static void at91_aic_unmask_irq(struct irq_data *d)
 	hard_cond_local_irq_restore(flags);
 }
 
- -static void __maybe_unused at91_aic5_unmask_irq(struct irq_data *d)
+static inline void at91_aic5_hard_unmask_irq(struct irq_data *d)
 {
 	/* Enable interrupt on AIC5 */
 	at91_aic_write(AT91_AIC5_SSR, d->hwirq & AT91_AIC5_INTSEL_MSK);
@@ -238,6 +248,16 @@ static void __maybe_unused at91_aic5_unmask_irq(struct irq_data *d)
 	set_backup(d->hwirq);
 }
 
+static void __maybe_unused at91_aic5_unmask_irq(struct irq_data *d)
+{
+	unsigned long flags;
+
+	flags = hard_cond_local_irq_save();
+	at91_aic5_hard_unmask_irq(d);
+	ipipe_unlock_irq(d->irq);
+	hard_cond_local_irq_restore(flags);
+}
+
 static void at91_aic_eoi(struct irq_data *d)
 {
 	/*
@@ -247,6 +267,11 @@ static void at91_aic_eoi(struct irq_data *d)
 	at91_aic_write(AT91_AIC_EOICR, 0);
 }
 
+static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
+{
+	at91_aic_write(AT91_AIC5_EOICR, 0);
+}
+
 #ifdef CONFIG_IPIPE
 static void at91_aic_hold_irq(struct irq_data *d)
 {
@@ -256,14 +281,24 @@ static void at91_aic_hold_irq(struct irq_data *d)
 
 static void at91_aic_release_irq(struct irq_data *d)
 {
+	unsigned long flags = hard_local_irq_save();
 	at91_aic_hard_unmask_irq(d);
+	hard_local_irq_restore(flags);
 }
- -#endif /* CONFIG_IPIPE */
 
- -static void __maybe_unused at91_aic5_eoi(struct irq_data *d)
+static void __maybe_unused at91_aic5_hold_irq(struct irq_data *d)
 {
- -	at91_aic_write(AT91_AIC5_EOICR, 0);
+	at91_aic5_hard_mask_irq(d);
+	at91_aic5_eoi(d);
+}
+
+static void __maybe_unused at91_aic5_release_irq(struct irq_data *d)
+{
+	unsigned long flags = hard_local_irq_save();
+	at91_aic5_hard_unmask_irq(d);
+	hard_local_irq_restore(flags);
 }
+#endif /* CONFIG_IPIPE */
 
 static unsigned long *at91_extern_irq;
 
@@ -527,6 +562,10 @@ int __init at91_aic5_of_init(struct device_node *node,
 	at91_aic_chip.irq_mask		= at91_aic5_mask_irq;
 	at91_aic_chip.irq_unmask	= at91_aic5_unmask_irq;
 	at91_aic_chip.irq_eoi		= at91_aic5_eoi;
+#ifdef CONFIG_IPIPE
+	at91_aic_chip.irq_hold		= at91_aic5_hold_irq;
+	at91_aic_chip.irq_release	= at91_aic5_release_irq;
+#endif
 	at91_aic_irq_ops.map		= at91_aic5_irq_map;
 
 	err = at91_aic_of_common_init(node, parent);


> 
> Maxime
> 


- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTt7O/GpcgE6m/fboRAvANAJ0YX0XXpvWaDGHx5ieWGJ56pPr94wCfaQV4
0BNkzC1CovZ0CwAIQzw1uXk=
=dHKp
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix wrote:
> >>> Ok, so, with the changes you mentionned, I can't make the system
> >>> crash anymore (or at least, not as easily as it used to be).
> >>>
> >>> But: - whenever the program mentionned above calls exit(), it 
> >>> stalls. However, ctrl+c makes the program exit properly, and 
> >>> everything seems fine otherwise - whenever we don't link it against
> >>> xenomai, it just hangs. I've not figured out why yet
> >>>
> >>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works fine.
> >>
> >> My answer was wrong, you probably need to keep the
> >> set_backup/clear_backup calls in he ->hold and ->release callbacks, as
> >> the linux interrupt may expect the backup areas to be in sync. Did you
> >> do this, or did you go for the alternative?
> > 
> > I went for the alternative. I'm sending you a v2 with what I have so
> > far, that shows the behaviour I was describing.
> 
> Could you try the following patch?

It actually works much better, thanks!

The above mentionned issues are gone, there's only one last glitch I
guess. The max latency under load is around a few 100's of ms (while
idle, it's actually around 200-300us, which seems more reasonable.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140707/f7dc479d/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/07/2014 06:02 PM, Maxime Ripard wrote:
> On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix
> wrote:
>>>>> Ok, so, with the changes you mentionned, I can't make the
>>>>> system crash anymore (or at least, not as easily as it used
>>>>> to be).
>>>>> 
>>>>> But: - whenever the program mentionned above calls exit(),
>>>>> it stalls. However, ctrl+c makes the program exit properly,
>>>>> and everything seems fine otherwise - whenever we don't
>>>>> link it against xenomai, it just hangs. I've not figured
>>>>> out why yet
>>>>> 
>>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works
>>>>> fine.
>>>> 
>>>> My answer was wrong, you probably need to keep the 
>>>> set_backup/clear_backup calls in he ->hold and ->release
>>>> callbacks, as the linux interrupt may expect the backup areas
>>>> to be in sync. Did you do this, or did you go for the
>>>> alternative?
>>> 
>>> I went for the alternative. I'm sending you a v2 with what I
>>> have so far, that shows the behaviour I was describing.
>> 
>> Could you try the following patch?
> 
> It actually works much better, thanks!
> 
> The above mentionned issues are gone, there's only one last glitch
> I guess. The max latency under load is around a few 100's of ms
> (while idle, it's actually around 200-300us, which seems more
> reasonable.

200us or 300us seems high for a last generation cortex. milliseconds
issues indicate an issue, does the msw column in latency increment?

> 
> Maxime
> 


- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTusXEGpcgE6m/fboRApbPAJwPqwDkMixnpMskG72oNprm8SCLlQCcCghd
mGkWJQwDvTtfUsFlQt3L/Q4=
=vyT1
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 07/07/2014 06:02 PM, Maxime Ripard wrote:
> > On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix
> > wrote:
> >>>>> Ok, so, with the changes you mentionned, I can't make the
> >>>>> system crash anymore (or at least, not as easily as it used
> >>>>> to be).
> >>>>> 
> >>>>> But: - whenever the program mentionned above calls exit(),
> >>>>> it stalls. However, ctrl+c makes the program exit properly,
> >>>>> and everything seems fine otherwise - whenever we don't
> >>>>> link it against xenomai, it just hangs. I've not figured
> >>>>> out why yet
> >>>>> 
> >>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works
> >>>>> fine.
> >>>> 
> >>>> My answer was wrong, you probably need to keep the 
> >>>> set_backup/clear_backup calls in he ->hold and ->release
> >>>> callbacks, as the linux interrupt may expect the backup areas
> >>>> to be in sync. Did you do this, or did you go for the
> >>>> alternative?
> >>> 
> >>> I went for the alternative. I'm sending you a v2 with what I
> >>> have so far, that shows the behaviour I was describing.
> >> 
> >> Could you try the following patch?
> > 
> > It actually works much better, thanks!
> > 
> > The above mentionned issues are gone, there's only one last glitch
> > I guess. The max latency under load is around a few 100's of ms
> > (while idle, it's actually around 200-300us, which seems more
> > reasonable.
> 
> 200us or 300us seems high for a last generation cortex. milliseconds
> issues indicate an issue, does the msw column in latency increment?

Ok, so after spending more time running xeno-test and latency

  * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b /usr/bin/hackbench 60"
    - the average latency measured is 37us, with a max at 53

  * If I just use the same load generator program I was using
    previously (which basically just run hackbench, dohell, do some
    dd, output some data through netcat, etc.), and latency, for 45
    minutes, I get an average latency at 35us, and a max latency at 60.

So the issue seems to lie in something related to our test program.

It's actually using CLOCK_MONOTONIC. When running just Xenomai's
clocktest program, the drift is OK, but the ToD offset is quite huge
and varies slightly from one run to another. This also happens using
CLOCK_REALTIME.

>From what I understand of the POSIX clocks, gettimeofday is always
using EPOCH as a starting point, while POSIX clocks can pick whatever
fixed starting point, so an offset might be expected. What's weird is
that it whenever you use the CLOCK_HOST_REALTIME, which is used by
xeno-test, and should be in sync with the Linux clock, this offset is
no longer there.

That's the only meaningful difference I could spot between the two
programs (ours vs clocktest).

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140708/265ea386/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Tue, Jul 08, 2014 at 02:55:05PM +0200, Maxime Ripard wrote:
> On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 07/07/2014 06:02 PM, Maxime Ripard wrote:
> > > On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix
> > > wrote:
> > >>>>> Ok, so, with the changes you mentionned, I can't make the
> > >>>>> system crash anymore (or at least, not as easily as it used
> > >>>>> to be).
> > >>>>> 
> > >>>>> But: - whenever the program mentionned above calls exit(),
> > >>>>> it stalls. However, ctrl+c makes the program exit properly,
> > >>>>> and everything seems fine otherwise - whenever we don't
> > >>>>> link it against xenomai, it just hangs. I've not figured
> > >>>>> out why yet
> > >>>>> 
> > >>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it works
> > >>>>> fine.
> > >>>> 
> > >>>> My answer was wrong, you probably need to keep the 
> > >>>> set_backup/clear_backup calls in he ->hold and ->release
> > >>>> callbacks, as the linux interrupt may expect the backup areas
> > >>>> to be in sync. Did you do this, or did you go for the
> > >>>> alternative?
> > >>> 
> > >>> I went for the alternative. I'm sending you a v2 with what I
> > >>> have so far, that shows the behaviour I was describing.
> > >> 
> > >> Could you try the following patch?
> > > 
> > > It actually works much better, thanks!
> > > 
> > > The above mentionned issues are gone, there's only one last glitch
> > > I guess. The max latency under load is around a few 100's of ms
> > > (while idle, it's actually around 200-300us, which seems more
> > > reasonable.
> > 
> > 200us or 300us seems high for a last generation cortex. milliseconds
> > issues indicate an issue, does the msw column in latency increment?
> 
> Ok, so after spending more time running xeno-test and latency
> 
>   * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b /usr/bin/hackbench 60"
>     - the average latency measured is 37us, with a max at 53
> 
>   * If I just use the same load generator program I was using
>     previously (which basically just run hackbench, dohell, do some
>     dd, output some data through netcat, etc.), and latency, for 45
>     minutes, I get an average latency at 35us, and a max latency at 60.
> 
> So the issue seems to lie in something related to our test program.

For the record, the code is there:
http://git.free-electrons.com/training-materials/tree/lab-data/realtime/rttest/data/rttest.c

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140708/c8aeee77/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/08/2014 02:55 PM, Maxime Ripard wrote:
> On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix
> wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 07/07/2014 06:02 PM, Maxime Ripard wrote:
>>> On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix 
>>> wrote:
>>>>>>> Ok, so, with the changes you mentionned, I can't make
>>>>>>> the system crash anymore (or at least, not as easily as
>>>>>>> it used to be).
>>>>>>> 
>>>>>>> But: - whenever the program mentionned above calls
>>>>>>> exit(), it stalls. However, ctrl+c makes the program
>>>>>>> exit properly, and everything seems fine otherwise -
>>>>>>> whenever we don't link it against xenomai, it just
>>>>>>> hangs. I've not figured out why yet
>>>>>>> 
>>>>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it
>>>>>>> works fine.
>>>>>> 
>>>>>> My answer was wrong, you probably need to keep the 
>>>>>> set_backup/clear_backup calls in he ->hold and ->release 
>>>>>> callbacks, as the linux interrupt may expect the backup
>>>>>> areas to be in sync. Did you do this, or did you go for
>>>>>> the alternative?
>>>>> 
>>>>> I went for the alternative. I'm sending you a v2 with what
>>>>> I have so far, that shows the behaviour I was describing.
>>>> 
>>>> Could you try the following patch?
>>> 
>>> It actually works much better, thanks!
>>> 
>>> The above mentionned issues are gone, there's only one last
>>> glitch I guess. The max latency under load is around a few
>>> 100's of ms (while idle, it's actually around 200-300us, which
>>> seems more reasonable.
>> 
>> 200us or 300us seems high for a last generation cortex.
>> milliseconds issues indicate an issue, does the msw column in
>> latency increment?
> 
> Ok, so after spending more time running xeno-test and latency
> 
> * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b
> /usr/bin/hackbench 60" - the average latency measured is 37us, with
> a max at 53
> 
> * If I just use the same load generator program I was using 
> previously (which basically just run hackbench, dohell, do some dd,
> output some data through netcat, etc.), and latency, for 45 
> minutes, I get an average latency at 35us, and a max latency at
> 60.
> 
> So the issue seems to lie in something related to our test
> program.
> 
> It's actually using CLOCK_MONOTONIC. When running just Xenomai's 
> clocktest program, the drift is OK, but the ToD offset is quite
> huge and varies slightly from one run to another. This also happens
> using CLOCK_REALTIME.
> 
> From what I understand of the POSIX clocks, gettimeofday is always 
> using EPOCH as a starting point, while POSIX clocks can pick
> whatever fixed starting point, so an offset might be expected.
> What's weird is that it whenever you use the CLOCK_HOST_REALTIME,
> which is used by xeno-test, and should be in sync with the Linux
> clock, this offset is no longer there.
> 
> That's the only meaningful difference I could spot between the two 
> programs (ours vs clocktest).

The test you are interested in is latency, not clocktest. The main
differences between latency and the test you run are:
- - the "period" (which BTW, in your case would only be a real period if
you removed the call to clock_gettime at the beginning of the loop,
only keeping the one before the loop), which is 1ms in latency case,
100us in your case
- - the fact that your function timespec_diff returns unsigned value, so
in case of early shot, you will get a very large value instead of a
negative ones.

As a general rule, we prefer Xenomai users to use the latency test,
because this is the one we collectively spent time debugging, so it
has more chances to be correct, and if you find a bug, everyone
benefits from the fix.

As for the question of the clocks:
- - for POSIX, CLOCK_REALTIME and gettimeofday use the same time base
- - CLOCK_MONOTONIC may use whatever they want, but usually are the time
from boot or from the clocksource start;
- - CLOCK_HOST_REALTIME should be synchronized with Linux
CLOCK_REALTIME, that is the aim of CLOCK_HOST_REALTIME;
- - CLOCK_REALTIME in the case of Xenomai is not near Linux
CLOCK_REALTIME because the xenomai base time is initialized before
your board sets its base, by whatever mean (hwclock, ntp, ntpdate,
rdate, etc...), you can avoid that by using clock_settime to set
Xenomai clock base, but the clock will drift away from Linux
CLOCK_REALTIME anyway, so you will need to call clock_settime from
time to time to keep them more or less in sync. This early
synchronization is arguably broken, in fact it made sense when xenomai
was compiled as a module, and loaded later on, but probably no longer
makes sense for built-in configurations, which are now the default.

- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTvCq1GpcgE6m/fboRAnIoAJoDY1TmGfrFwl8ywOcche/ozOTcZgCcDllh
9ggKL6Ee3xXn1DVPuqGM+08=
=AhHh
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Tue, Jul 08, 2014 at 07:30:29PM +0200, Gilles Chanteperdrix wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 07/08/2014 02:55 PM, Maxime Ripard wrote:
> > On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix
> > wrote:
> >> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
> >> 
> >> On 07/07/2014 06:02 PM, Maxime Ripard wrote:
> >>> On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix 
> >>> wrote:
> >>>>>>> Ok, so, with the changes you mentionned, I can't make
> >>>>>>> the system crash anymore (or at least, not as easily as
> >>>>>>> it used to be).
> >>>>>>> 
> >>>>>>> But: - whenever the program mentionned above calls
> >>>>>>> exit(), it stalls. However, ctrl+c makes the program
> >>>>>>> exit properly, and everything seems fine otherwise -
> >>>>>>> whenever we don't link it against xenomai, it just
> >>>>>>> hangs. I've not figured out why yet
> >>>>>>> 
> >>>>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it
> >>>>>>> works fine.
> >>>>>> 
> >>>>>> My answer was wrong, you probably need to keep the 
> >>>>>> set_backup/clear_backup calls in he ->hold and ->release 
> >>>>>> callbacks, as the linux interrupt may expect the backup
> >>>>>> areas to be in sync. Did you do this, or did you go for
> >>>>>> the alternative?
> >>>>> 
> >>>>> I went for the alternative. I'm sending you a v2 with what
> >>>>> I have so far, that shows the behaviour I was describing.
> >>>> 
> >>>> Could you try the following patch?
> >>> 
> >>> It actually works much better, thanks!
> >>> 
> >>> The above mentionned issues are gone, there's only one last
> >>> glitch I guess. The max latency under load is around a few
> >>> 100's of ms (while idle, it's actually around 200-300us, which
> >>> seems more reasonable.
> >> 
> >> 200us or 300us seems high for a last generation cortex.
> >> milliseconds issues indicate an issue, does the msw column in
> >> latency increment?
> > 
> > Ok, so after spending more time running xeno-test and latency
> > 
> > * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b
> > /usr/bin/hackbench 60" - the average latency measured is 37us, with
> > a max at 53
> > 
> > * If I just use the same load generator program I was using 
> > previously (which basically just run hackbench, dohell, do some dd,
> > output some data through netcat, etc.), and latency, for 45 
> > minutes, I get an average latency at 35us, and a max latency at
> > 60.
> > 
> > So the issue seems to lie in something related to our test
> > program.
> > 
> > It's actually using CLOCK_MONOTONIC. When running just Xenomai's 
> > clocktest program, the drift is OK, but the ToD offset is quite
> > huge and varies slightly from one run to another. This also happens
> > using CLOCK_REALTIME.
> > 
> > From what I understand of the POSIX clocks, gettimeofday is always 
> > using EPOCH as a starting point, while POSIX clocks can pick
> > whatever fixed starting point, so an offset might be expected.
> > What's weird is that it whenever you use the CLOCK_HOST_REALTIME,
> > which is used by xeno-test, and should be in sync with the Linux
> > clock, this offset is no longer there.
> > 
> > That's the only meaningful difference I could spot between the two 
> > programs (ours vs clocktest).
> 
> The test you are interested in is latency, not clocktest. The main
> differences between latency and the test you run are:
> - - the "period" (which BTW, in your case would only be a real period if
> you removed the call to clock_gettime at the beginning of the loop,
> only keeping the one before the loop), which is 1ms in latency case,
> 100us in your case

I don't really get what you mean here. If we don't call gettime at the
beginning of the loop, but only outside, how are we supposed to get
the next sleep expiration time?

> - - the fact that your function timespec_diff returns unsigned value, so
> in case of early shot, you will get a very large value instead of a
> negative ones.
> 
> As a general rule, we prefer Xenomai users to use the latency test,
> because this is the one we collectively spent time debugging, so it
> has more chances to be correct, and if you find a bug, everyone
> benefits from the fix.

Yes, I don't doubt that latency is much more tested and reliable.

The thing is, as you probably know, we're also a training company, and
we're using this script in our training to give an idea of the latency
on a regular linux kernel, and then on xenomai. It also have the
benefit of being simple enough for the trainees to be able to
understand it rather quickly.

I don't think latency fits both these criterias, that are quite
essential for us. But if you have any better solution that might,
we're definitely open to suggestions :)

> As for the question of the clocks:
> - - for POSIX, CLOCK_REALTIME and gettimeofday use the same time base
> - - CLOCK_MONOTONIC may use whatever they want, but usually are the time
> from boot or from the clocksource start;
> - - CLOCK_HOST_REALTIME should be synchronized with Linux
> CLOCK_REALTIME, that is the aim of CLOCK_HOST_REALTIME;
> - - CLOCK_REALTIME in the case of Xenomai is not near Linux
> CLOCK_REALTIME because the xenomai base time is initialized before
> your board sets its base, by whatever mean (hwclock, ntp, ntpdate,
> rdate, etc...), you can avoid that by using clock_settime to set
> Xenomai clock base, but the clock will drift away from Linux
> CLOCK_REALTIME anyway, so you will need to call clock_settime from
> time to time to keep them more or less in sync. This early
> synchronization is arguably broken, in fact it made sense when xenomai
> was compiled as a module, and loaded later on, but probably no longer
> makes sense for built-in configurations, which are now the default.

Thanks for this explanation :)

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140710/d89e634e/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

On 10/07/2014 17:05, Maxime Ripard wrote:
> On Tue, Jul 08, 2014 at 07:30:29PM +0200, Gilles Chanteperdrix wrote:
>> -----BEGIN PGP SIGNED MESSAGE-----
>> Hash: SHA1
>>
>> On 07/08/2014 02:55 PM, Maxime Ripard wrote:
>>> On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix
>>> wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>>
>>>> On 07/07/2014 06:02 PM, Maxime Ripard wrote:
>>>>> On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles Chanteperdrix 
>>>>> wrote:
>>>>>>>>> Ok, so, with the changes you mentionned, I can't make
>>>>>>>>> the system crash anymore (or at least, not as easily as
>>>>>>>>> it used to be).
>>>>>>>>>
>>>>>>>>> But: - whenever the program mentionned above calls
>>>>>>>>> exit(), it stalls. However, ctrl+c makes the program
>>>>>>>>> exit properly, and everything seems fine otherwise -
>>>>>>>>> whenever we don't link it against xenomai, it just
>>>>>>>>> hangs. I've not figured out why yet
>>>>>>>>>
>>>>>>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it
>>>>>>>>> works fine.
>>>>>>>>
>>>>>>>> My answer was wrong, you probably need to keep the 
>>>>>>>> set_backup/clear_backup calls in he ->hold and ->release 
>>>>>>>> callbacks, as the linux interrupt may expect the backup
>>>>>>>> areas to be in sync. Did you do this, or did you go for
>>>>>>>> the alternative?
>>>>>>>
>>>>>>> I went for the alternative. I'm sending you a v2 with what
>>>>>>> I have so far, that shows the behaviour I was describing.
>>>>>>
>>>>>> Could you try the following patch?
>>>>>
>>>>> It actually works much better, thanks!
>>>>>
>>>>> The above mentionned issues are gone, there's only one last
>>>>> glitch I guess. The max latency under load is around a few
>>>>> 100's of ms (while idle, it's actually around 200-300us, which
>>>>> seems more reasonable.
>>>>
>>>> 200us or 300us seems high for a last generation cortex.
>>>> milliseconds issues indicate an issue, does the msw column in
>>>> latency increment?
>>>
>>> Ok, so after spending more time running xeno-test and latency
>>>
>>> * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b
>>> /usr/bin/hackbench 60" - the average latency measured is 37us, with
>>> a max at 53
>>>
>>> * If I just use the same load generator program I was using 
>>> previously (which basically just run hackbench, dohell, do some dd,
>>> output some data through netcat, etc.), and latency, for 45 
>>> minutes, I get an average latency at 35us, and a max latency at
>>> 60.
>>>
>>> So the issue seems to lie in something related to our test
>>> program.
>>>
>>> It's actually using CLOCK_MONOTONIC. When running just Xenomai's 
>>> clocktest program, the drift is OK, but the ToD offset is quite
>>> huge and varies slightly from one run to another. This also happens
>>> using CLOCK_REALTIME.
>>>
>>> From what I understand of the POSIX clocks, gettimeofday is always 
>>> using EPOCH as a starting point, while POSIX clocks can pick
>>> whatever fixed starting point, so an offset might be expected.
>>> What's weird is that it whenever you use the CLOCK_HOST_REALTIME,
>>> which is used by xeno-test, and should be in sync with the Linux
>>> clock, this offset is no longer there.
>>>
>>> That's the only meaningful difference I could spot between the two 
>>> programs (ours vs clocktest).
>>
>> The test you are interested in is latency, not clocktest. The main
>> differences between latency and the test you run are:
>> - - the "period" (which BTW, in your case would only be a real period if
>> you removed the call to clock_gettime at the beginning of the loop,
>> only keeping the one before the loop), which is 1ms in latency case,
>> 100us in your case
> 
> I don't really get what you mean here. If we don't call gettime at the
> beginning of the loop, but only outside, how are we supposed to get
> the next sleep expiration time?

Look at the code, time1 is not a loop local variable, the next sleep
expiration date is computed and passed to clock_nanosleep. So, you
simply have to add the period to this date, and use the new date. This
is the classical way of using clock_nanosleep for periodic task.
Re-reading the current time before computing the wake up time introduces
an error which breaks the periodicity of the task, and makes the use of
absolute clock_nanosleep useless, a relative sleep would do the same
thing much more simply.

Another problem with this code is that it does not check for
clock_nanosleep return value, so does not account correctly for overruns.

> 
>> - - the fact that your function timespec_diff returns unsigned value, so
>> in case of early shot, you will get a very large value instead of a
>> negative ones.
>>
>> As a general rule, we prefer Xenomai users to use the latency test,
>> because this is the one we collectively spent time debugging, so it
>> has more chances to be correct, and if you find a bug, everyone
>> benefits from the fix.
> 
> Yes, I don't doubt that latency is much more tested and reliable.
> 
> The thing is, as you probably know, we're also a training company, and
> we're using this script in our training to give an idea of the latency
> on a regular linux kernel, and then on xenomai. It also have the
> benefit of being simple enough for the trainees to be able to
> understand it rather quickly.

It is simple but broken.

> 
> I don't think latency fits both these criterias, that are quite
> essential for us. But if you have any better solution that might,
> we're definitely open to suggestions :)

Xenomai forge's latency is based on timerfd, so will be usable on Linux,
preempt-rt and xenomai. But that is for the future.

I suggest you fix the issue with negative latencies, and see if it
avoids the large latencies you observe.

-- 
                                                                Gilles.

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Thu, Jul 10, 2014 at 07:04:20PM +0200, Gilles Chanteperdrix wrote:
> >> The test you are interested in is latency, not clocktest. The main
> >> differences between latency and the test you run are:
> >> - - the "period" (which BTW, in your case would only be a real period if
> >> you removed the call to clock_gettime at the beginning of the loop,
> >> only keeping the one before the loop), which is 1ms in latency case,
> >> 100us in your case
> > 
> > I don't really get what you mean here. If we don't call gettime at the
> > beginning of the loop, but only outside, how are we supposed to get
> > the next sleep expiration time?
> 
> Look at the code, time1 is not a loop local variable, the next sleep
> expiration date is computed and passed to clock_nanosleep. So, you
> simply have to add the period to this date, and use the new date. This
> is the classical way of using clock_nanosleep for periodic task.
> Re-reading the current time before computing the wake up time introduces
> an error which breaks the periodicity of the task, and makes the use of
> absolute clock_nanosleep useless, a relative sleep would do the same
> thing much more simply.

Except that we don't really care for the periodicity of a task. The
purpose of this test is actually to compute a single latency, over a
large enough number of samples to be meaningful.

So the drift away from the start of the loop is not taken into account,
on purpose.

> Another problem with this code is that it does not check for
> clock_nanosleep return value, so does not account correctly for overruns.

That's true.

> 
> > 
> >> - - the fact that your function timespec_diff returns unsigned value, so
> >> in case of early shot, you will get a very large value instead of a
> >> negative ones.
> >>
> >> As a general rule, we prefer Xenomai users to use the latency test,
> >> because this is the one we collectively spent time debugging, so it
> >> has more chances to be correct, and if you find a bug, everyone
> >> benefits from the fix.
> > 
> > Yes, I don't doubt that latency is much more tested and reliable.
> > 
> > The thing is, as you probably know, we're also a training company, and
> > we're using this script in our training to give an idea of the latency
> > on a regular linux kernel, and then on xenomai. It also have the
> > benefit of being simple enough for the trainees to be able to
> > understand it rather quickly.
> 
> It is simple but broken.

See above.

> > I don't think latency fits both these criterias, that are quite
> > essential for us. But if you have any better solution that might,
> > we're definitely open to suggestions :)
> 
> Xenomai forge's latency is based on timerfd, so will be usable on Linux,
> preempt-rt and xenomai. But that is for the future.

Ah, good to know.

> I suggest you fix the issue with negative latencies, and see if it
> avoids the large latencies you observe.

So, I tested it today with the latency calibration disabled.

It doesn't change anything. I slightly modified it to dump the time1
and time2 variables to see if we're observing negative latencies.

The code is here: http://code.bulix.org/y7f8tc-86530?raw
And here is the output of a single run: http://code.bulix.org/ciamlo-86531?raw

So, if you look at it, we can see that we have around half a dozen of
these huge latency spikes, while we gather 180k samples, but these
spikes are not actually caused by some negative latencies. There
actually is such a different of a few 100's of ms between our two
timespec structures.

And it's not due to an error reported by any of the clock functions
either.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140716/a502de69/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/16/2014 06:18 PM, Maxime Ripard wrote:
> On Thu, Jul 10, 2014 at 07:04:20PM +0200, Gilles Chanteperdrix
> wrote:
>>>> The test you are interested in is latency, not clocktest. The
>>>> main differences between latency and the test you run are: -
>>>> - the "period" (which BTW, in your case would only be a real
>>>> period if you removed the call to clock_gettime at the
>>>> beginning of the loop, only keeping the one before the loop),
>>>> which is 1ms in latency case, 100us in your case
>>> 
>>> I don't really get what you mean here. If we don't call gettime
>>> at the beginning of the loop, but only outside, how are we
>>> supposed to get the next sleep expiration time?
>> 
>> Look at the code, time1 is not a loop local variable, the next
>> sleep expiration date is computed and passed to clock_nanosleep.
>> So, you simply have to add the period to this date, and use the
>> new date. This is the classical way of using clock_nanosleep for
>> periodic task. Re-reading the current time before computing the
>> wake up time introduces an error which breaks the periodicity of
>> the task, and makes the use of absolute clock_nanosleep useless,
>> a relative sleep would do the same thing much more simply.
> 
> Except that we don't really care for the periodicity of a task.
> The purpose of this test is actually to compute a single latency,
> over a large enough number of samples to be meaningful.
> 
> So the drift away from the start of the loop is not taken into
> account, on purpose.

People usually do that with periodic tasks, see cyclictest in the
preempt_rt world, and latency for xenomai (or cyclictest also if you
prefer).

> 
>> Another problem with this code is that it does not check for 
>> clock_nanosleep return value, so does not account correctly for
>> overruns.
> 
> That's true.
> 
>> 
>>> 
>>>> - - the fact that your function timespec_diff returns
>>>> unsigned value, so in case of early shot, you will get a very
>>>> large value instead of a negative ones.
>>>> 
>>>> As a general rule, we prefer Xenomai users to use the latency
>>>> test, because this is the one we collectively spent time
>>>> debugging, so it has more chances to be correct, and if you
>>>> find a bug, everyone benefits from the fix.
>>> 
>>> Yes, I don't doubt that latency is much more tested and
>>> reliable.
>>> 
>>> The thing is, as you probably know, we're also a training
>>> company, and we're using this script in our training to give an
>>> idea of the latency on a regular linux kernel, and then on
>>> xenomai. It also have the benefit of being simple enough for
>>> the trainees to be able to understand it rather quickly.
>> 
>> It is simple but broken.
> 
> See above.

The reason why I said it was broken is for the negative latencies.

> 
>>> I don't think latency fits both these criterias, that are
>>> quite essential for us. But if you have any better solution
>>> that might, we're definitely open to suggestions :)
>> 
>> Xenomai forge's latency is based on timerfd, so will be usable on
>> Linux, preempt-rt and xenomai. But that is for the future.
> 
> Ah, good to know.
> 
>> I suggest you fix the issue with negative latencies, and see if
>> it avoids the large latencies you observe.
> 
> So, I tested it today with the latency calibration disabled.
> 
> It doesn't change anything. I slightly modified it to dump the
> time1 and time2 variables to see if we're observing negative
> latencies.
> 
> The code is here: http://code.bulix.org/y7f8tc-86530?raw And here
> is the output of a single run:
> http://code.bulix.org/ciamlo-86531?raw
> 
> So, if you look at it, we can see that we have around half a dozen
> of these huge latency spikes, while we gather 180k samples, but
> these spikes are not actually caused by some negative latencies.
> There actually is such a different of a few 100's of ms between our
> two timespec structures.
> 
> And it's not due to an error reported by any of the clock
> functions either.

Ok, you do not call pthread_setschedparam to make the task run with
the SCHED_FIFO policy, do you set the policy by running the program
with chrt? Because starting with Xenomai 2.6.0, threads with the
SCHED_OTHER policy automatically return to secondary mode after a
primary mode syscall, so your task is essentially a plain linux task.
If you do run the program with chrt, I suggest checking in
/proc/xenomai/stat or /proc/xenomai/sched that the task runs with the
intended priority.

- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTxtbaGpcgE6m/fboRAo9TAJoCLxy1rSCjpW0Eugm9tVjzAHhaYQCggVw0
SxKjiZ2GCFD/0zKp1+5qISg=
=jRyP
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Wed, Jul 16, 2014 at 09:47:38PM +0200, Gilles Chanteperdrix wrote:
> >>> I don't think latency fits both these criterias, that are
> >>> quite essential for us. But if you have any better solution
> >>> that might, we're definitely open to suggestions :)
> >> 
> >> Xenomai forge's latency is based on timerfd, so will be usable on
> >> Linux, preempt-rt and xenomai. But that is for the future.
> > 
> > Ah, good to know.
> > 
> >> I suggest you fix the issue with negative latencies, and see if
> >> it avoids the large latencies you observe.
> > 
> > So, I tested it today with the latency calibration disabled.
> > 
> > It doesn't change anything. I slightly modified it to dump the
> > time1 and time2 variables to see if we're observing negative
> > latencies.
> > 
> > The code is here: http://code.bulix.org/y7f8tc-86530?raw And here
> > is the output of a single run:
> > http://code.bulix.org/ciamlo-86531?raw
> > 
> > So, if you look at it, we can see that we have around half a dozen
> > of these huge latency spikes, while we gather 180k samples, but
> > these spikes are not actually caused by some negative latencies.
> > There actually is such a different of a few 100's of ms between our
> > two timespec structures.
> > 
> > And it's not due to an error reported by any of the clock
> > functions either.
> 
> Ok, you do not call pthread_setschedparam to make the task run with
> the SCHED_FIFO policy, do you set the policy by running the program
> with chrt? Because starting with Xenomai 2.6.0, threads with the
> SCHED_OTHER policy automatically return to secondary mode after a
> primary mode syscall, so your task is essentially a plain linux task.
> If you do run the program with chrt, I suggest checking in
> /proc/xenomai/stat or /proc/xenomai/sched that the task runs with the
> intended priority.

/me hides...

Yes, it was just that stupid mistake... Erm. I guess I owe you a drink
if we ever meet :)

And now, I indeed get some negative latency values.

Which brings an extra question about this. Is the timer calibration
supposed to get a minimum latency to 0 (meaning that with a properly
calibrated timer, we would only get positive latencies), or that the
average latency should be around 0 (which would mean that negative
latencies would actually be frequent, but would break the
clock_nanosleep documented behaviour).

I'm actually seeing the former, but trying the latter only brings us
quite close to having the calibration disabled entirely.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140717/c915c09c/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/17/2014 12:18 PM, Maxime Ripard wrote:
> On Wed, Jul 16, 2014 at 09:47:38PM +0200, Gilles Chanteperdrix
> wrote:
>>>>> I don't think latency fits both these criterias, that are 
>>>>> quite essential for us. But if you have any better
>>>>> solution that might, we're definitely open to suggestions
>>>>> :)
>>>> 
>>>> Xenomai forge's latency is based on timerfd, so will be
>>>> usable on Linux, preempt-rt and xenomai. But that is for the
>>>> future.
>>> 
>>> Ah, good to know.
>>> 
>>>> I suggest you fix the issue with negative latencies, and see
>>>> if it avoids the large latencies you observe.
>>> 
>>> So, I tested it today with the latency calibration disabled.
>>> 
>>> It doesn't change anything. I slightly modified it to dump the 
>>> time1 and time2 variables to see if we're observing negative 
>>> latencies.
>>> 
>>> The code is here: http://code.bulix.org/y7f8tc-86530?raw And
>>> here is the output of a single run: 
>>> http://code.bulix.org/ciamlo-86531?raw
>>> 
>>> So, if you look at it, we can see that we have around half a
>>> dozen of these huge latency spikes, while we gather 180k
>>> samples, but these spikes are not actually caused by some
>>> negative latencies. There actually is such a different of a few
>>> 100's of ms between our two timespec structures.
>>> 
>>> And it's not due to an error reported by any of the clock 
>>> functions either.
>> 
>> Ok, you do not call pthread_setschedparam to make the task run
>> with the SCHED_FIFO policy, do you set the policy by running the
>> program with chrt? Because starting with Xenomai 2.6.0, threads
>> with the SCHED_OTHER policy automatically return to secondary
>> mode after a primary mode syscall, so your task is essentially a
>> plain linux task. If you do run the program with chrt, I suggest
>> checking in /proc/xenomai/stat or /proc/xenomai/sched that the
>> task runs with the intended priority.
> 
> /me hides...
> 
> Yes, it was just that stupid mistake... Erm. I guess I owe you a
> drink if we ever meet :)
> 
> And now, I indeed get some negative latency values.
> 
> Which brings an extra question about this. Is the timer
> calibration supposed to get a minimum latency to 0 (meaning that
> with a properly calibrated timer, we would only get positive
> latencies), or that the average latency should be around 0 (which
> would mean that negative latencies would actually be frequent, but
> would break the clock_nanosleep documented behaviour).
> 
> I'm actually seeing the former, but trying the latter only brings
> us quite close to having the calibration disabled entirely.

The calibration is left entirely to the user, so, you choose what is
best for your application. But indeed, if your application relies on
clock_nanosleep documented behaviour (should never wake-up early
except in case of signal), you want to set /proc/xenomai/latency value
to the minimum value measured by the latency test, minus a safety margin.

- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTx6tSGpcgE6m/fboRAr2DAJ47ysURGA5T27Sq4FrNpFfs59sySQCeMfTR
8XCWDd+NWn5d6VKP+Nh4t/A=
=sYTn
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Maxime Ripard]

On Thu, Jul 17, 2014 at 12:54:10PM +0200, Gilles Chanteperdrix wrote:
> > Which brings an extra question about this. Is the timer
> > calibration supposed to get a minimum latency to 0 (meaning that
> > with a properly calibrated timer, we would only get positive
> > latencies), or that the average latency should be around 0 (which
> > would mean that negative latencies would actually be frequent, but
> > would break the clock_nanosleep documented behaviour).
> > 
> > I'm actually seeing the former, but trying the latter only brings
> > us quite close to having the calibration disabled entirely.
> 
> The calibration is left entirely to the user, so, you choose what is
> best for your application. But indeed, if your application relies on
> clock_nanosleep documented behaviour (should never wake-up early
> except in case of signal), you want to set /proc/xenomai/latency value
> to the minimum value measured by the latency test, minus a safety margin.

Ok, thanks!

I guess you can add my Tested-by tag to your patch, if that makes any
sense for Xenomai.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://www.xenomai.org/pipermail/xenomai/attachments/20140717/7bb489d7/attachment.sig>

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/17/2014 01:59 PM, Maxime Ripard wrote:
> On Thu, Jul 17, 2014 at 12:54:10PM +0200, Gilles Chanteperdrix
> wrote:
>>> Which brings an extra question about this. Is the timer 
>>> calibration supposed to get a minimum latency to 0 (meaning
>>> that with a properly calibrated timer, we would only get
>>> positive latencies), or that the average latency should be
>>> around 0 (which would mean that negative latencies would
>>> actually be frequent, but would break the clock_nanosleep
>>> documented behaviour).
>>> 
>>> I'm actually seeing the former, but trying the latter only
>>> brings us quite close to having the calibration disabled
>>> entirely.
>> 
>> The calibration is left entirely to the user, so, you choose what
>> is best for your application. But indeed, if your application
>> relies on clock_nanosleep documented behaviour (should never
>> wake-up early except in case of signal), you want to set
>> /proc/xenomai/latency value to the minimum value measured by the
>> latency test, minus a safety margin.
> 
> Ok, thanks!
> 
> I guess you can add my Tested-by tag to your patch, if that makes
> any sense for Xenomai.

We do not consistently use the signed-off mechanism, but I have added
your Tested-by anyway, thanks.

- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTyEx6GpcgE6m/fboRArMCAJ4396+Bx1OWjJarJtUYFANN2qs7VACfbkNK
0t/vZA4zO0W8kfv6cVGVA48=
=OODg
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 07/10/2014 05:05 PM, Maxime Ripard wrote:
> On Tue, Jul 08, 2014 at 07:30:29PM +0200, Gilles Chanteperdrix
> wrote:
>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>> 
>> On 07/08/2014 02:55 PM, Maxime Ripard wrote:
>>> On Mon, Jul 07, 2014 at 06:07:32PM +0200, Gilles Chanteperdrix 
>>> wrote:
>>>> -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
>>>> 
>>>> On 07/07/2014 06:02 PM, Maxime Ripard wrote:
>>>>> On Sat, Jul 05, 2014 at 10:13:51AM +0200, Gilles
>>>>> Chanteperdrix wrote:
>>>>>>>>> Ok, so, with the changes you mentionned, I can't
>>>>>>>>> make the system crash anymore (or at least, not as
>>>>>>>>> easily as it used to be).
>>>>>>>>> 
>>>>>>>>> But: - whenever the program mentionned above calls 
>>>>>>>>> exit(), it stalls. However, ctrl+c makes the
>>>>>>>>> program exit properly, and everything seems fine
>>>>>>>>> otherwise - whenever we don't link it against
>>>>>>>>> xenomai, it just hangs. I've not figured out why
>>>>>>>>> yet
>>>>>>>>> 
>>>>>>>>> With CONFIG_XENOMAI and CONFIG_IPIPE disabled, it 
>>>>>>>>> works fine.
>>>>>>>> 
>>>>>>>> My answer was wrong, you probably need to keep the 
>>>>>>>> set_backup/clear_backup calls in he ->hold and
>>>>>>>> ->release callbacks, as the linux interrupt may
>>>>>>>> expect the backup areas to be in sync. Did you do
>>>>>>>> this, or did you go for the alternative?
>>>>>>> 
>>>>>>> I went for the alternative. I'm sending you a v2 with
>>>>>>> what I have so far, that shows the behaviour I was
>>>>>>> describing.
>>>>>> 
>>>>>> Could you try the following patch?
>>>>> 
>>>>> It actually works much better, thanks!
>>>>> 
>>>>> The above mentionned issues are gone, there's only one
>>>>> last glitch I guess. The max latency under load is around a
>>>>> few 100's of ms (while idle, it's actually around
>>>>> 200-300us, which seems more reasonable.
>>>> 
>>>> 200us or 300us seems high for a last generation cortex. 
>>>> milliseconds issues indicate an issue, does the msw column
>>>> in latency increment?
>>> 
>>> Ok, so after spending more time running xeno-test and latency
>>> 
>>> * xeno-test -l "dohell -s 192.168.0.42 -p 5566 -b 
>>> /usr/bin/hackbench 60" - the average latency measured is 37us,
>>> with a max at 53
>>> 
>>> * If I just use the same load generator program I was using 
>>> previously (which basically just run hackbench, dohell, do some
>>> dd, output some data through netcat, etc.), and latency, for 45
>>>  minutes, I get an average latency at 35us, and a max latency
>>> at 60.
>>> 
>>> So the issue seems to lie in something related to our test 
>>> program.
>>> 
>>> It's actually using CLOCK_MONOTONIC. When running just
>>> Xenomai's clocktest program, the drift is OK, but the ToD
>>> offset is quite huge and varies slightly from one run to
>>> another. This also happens using CLOCK_REALTIME.
>>> 
>>> From what I understand of the POSIX clocks, gettimeofday is
>>> always using EPOCH as a starting point, while POSIX clocks can
>>> pick whatever fixed starting point, so an offset might be
>>> expected. What's weird is that it whenever you use the
>>> CLOCK_HOST_REALTIME, which is used by xeno-test, and should be
>>> in sync with the Linux clock, this offset is no longer there.
>>> 
>>> That's the only meaningful difference I could spot between the
>>> two programs (ours vs clocktest).
>> 
>> The test you are interested in is latency, not clocktest. The
>> main differences between latency and the test you run are: - -
>> the "period" (which BTW, in your case would only be a real period
>> if you removed the call to clock_gettime at the beginning of the
>> loop, only keeping the one before the loop), which is 1ms in
>> latency case, 100us in your case
> 
> I don't really get what you mean here. If we don't call gettime at
> the beginning of the loop, but only outside, how are we supposed to
> get the next sleep expiration time?
> 
>> - - the fact that your function timespec_diff returns unsigned
>> value, so in case of early shot, you will get a very large value
>> instead of a negative ones.
>> 
>> As a general rule, we prefer Xenomai users to use the latency
>> test, because this is the one we collectively spent time
>> debugging, so it has more chances to be correct, and if you find
>> a bug, everyone benefits from the fix.
> 
> Yes, I don't doubt that latency is much more tested and reliable.
> 
> The thing is, as you probably know, we're also a training company,
> and we're using this script in our training to give an idea of the
> latency on a regular linux kernel, and then on xenomai. It also
> have the benefit of being simple enough for the trainees to be able
> to understand it rather quickly.

Xenomai 2.6 also compiles the cyclictest test, which is part of
preempt_rt testsuite, so you  can use cyclictest as a common tool to
measure latency for all rt solutions.


- -- 
                                                                Gilles.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Icedove - http://www.enigmail.net/

iD8DBQFTvtsUGpcgE6m/fboRAvynAJ9tuJTt58GWoKQX/uhPMj2OZZRlzgCfWDfr
JYwKcAJO44QZyGcAVJfoIw4=
=Fvho
-----END PGP SIGNATURE-----

Re: [Xenomai] [PATCH] AT91: SAMA5D3: Adapt Ipipe for AIC5

[Gilles Chanteperdrix]

On 07/10/2014 05:27 PM, Thomas Petazzoni wrote:
> Gilles, Maxime,
> 
> On Thu, 10 Jul 2014 17:05:40 +0200, Maxime Ripard wrote:
> 
>>> As a general rule, we prefer Xenomai users to use the latency test,
>>> because this is the one we collectively spent time debugging, so it
>>> has more chances to be correct, and if you find a bug, everyone
>>> benefits from the fix.
>>
>> Yes, I don't doubt that latency is much more tested and reliable.
>>
>> The thing is, as you probably know, we're also a training company, and
>> we're using this script in our training to give an idea of the latency
>> on a regular linux kernel, and then on xenomai. It also have the
>> benefit of being simple enough for the trainees to be able to
>> understand it rather quickly.
>>
>> I don't think latency fits both these criterias, that are quite
>> essential for us. But if you have any better solution that might,
>> we're definitely open to suggestions :)
> 
> I should also add that we have been using rttest.c successfully since
> quite some time, including against the mainline kernel, PREEMPT_RT
> patched kernel, and Xenomai 2.6.2.1 running on a 3.5 kernel on an OMAP3
> based platform. Each time with good results.

You were simply lucky that the tested platforms were correctly
calibrated and you did not experience negative latencies. Have you
calibrated correctly the sama5 board timer?

-- 
                                                                Gilles.