[Xenomai] potentially missing timer rating check in select_root_only_timer()

[Xenomai] potentially missing timer rating check in select_root_only_timer()

[Pham, Phong]

Hi,

I am suspecting that in kernel/ipipe/timer.c:select_root_only_timer()

When t->irq == per_cpu(ipipe_percpu.hrtimer_irq, icpu), a condition to test the timer rating against current timer rating before installing the timer is missing.  That is t->rating > percpu_timer->rating (combined with same irq #) before the condition becomes true.

Here's screenshot:
[cid:image001.png@01D3E177.63116AC0]

Phong.

Notice: This e-mail and any files transmitted with it may contain Data Device Corporation's privileged and proprietary information. It is intended solely for the use of the individual or entity to whom it is addressed. If you are not the named recipient of this transmission, any disclosure, copying, distribution or reliance on the contents of this message is prohibited. If you received this e-mail in error, please destroy it and any attached files and notify me immediately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 100938 bytes
Desc: image001.png
URL: <http://xenomai.org/pipermail/xenomai/attachments/20180502/fb07b0f2/attachment.png>

Re: [Xenomai] potentially missing timer rating check in select_root_only_timer()

[Jan Kiszka]

Hi Phong,

On 2018-05-02 03:13, Pham, Phong wrote:
> Hi,
> 
> I am suspecting that in kernel/ipipe/timer.c:select_root_only_timer()
> 
> When t->irq == per_cpu(ipipe_percpu.hrtimer_irq, icpu), a condition to test the timer rating against current timer rating before installing the timer is missing.  That is t->rating > percpu_timer->rating (combined with same irq #) before the condition becomes true.

Conceptually, I-pipe timers should have highest rating. Can you explain
in more details why it should be there?

Also note that your own code looks suspicious: percpu_timer is a per-cpu
variable but is not accesses as such.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [Xenomai] potentially missing timer rating check in select_root_only_timer()

[Pham, Phong]

Hi Jan,

In my scenario, it is a single CPU Powerpc system.  What I was intending to do is replacing the decrementer (Powerpc specific count down timer) clockevent with my timer clockevent.  (My timer is located somewhere else on the chip accessible via memory mapped IO).   According to Linux, user can change the rating and kernel will select the highest rating timer for you.  So I change my timer rating to be higher than the decrementer.  In ipipe_select_timers(), it will pick the first timer found on the timers linked list.  That linked list of timers is also sorted by highest priority first.  However, when select_root_only_timer() is executed, the code goes thru the entire list and select the last one, the one with the lowest rating.

W/ regard to "percpu_timer->rating" coding style, yes, this can be done better but since I am on single cpu, what I did "does the job".  I recalled there are a bunch of macros just to change a variable; this can certainly be changed.  The focus is more on checking the ratings.

I am also not familiar with the code enough so I may have misunderstandings on their intended usage.  Maybe you could clarify them for me.

Thanks,
Phong.

PS: For our need, I decided I will no longer pusue the path of replace the decrementer with a different timer.  But I just thought to bring up my observation with regard to selecting the highest timer rating.

-----Original Message-----
From: Jan Kiszka [mailto:jan.kiszka@siemens.com]
Sent: Tuesday, May 01, 2018 10:34 PM
To: Pham, Phong; xenomai@xenomai.org
Cc: Hillman, Robert
Subject: Re: potentially missing timer rating check in select_root_only_timer()

Hi Phong,

On 2018-05-02 03:13, Pham, Phong wrote:
> Hi,
>
> I am suspecting that in kernel/ipipe/timer.c:select_root_only_timer()
>
> When t->irq == per_cpu(ipipe_percpu.hrtimer_irq, icpu), a condition to test the timer rating against current timer rating before installing the timer is missing.  That is t->rating > percpu_timer->rating (combined with same irq #) before the condition becomes true.

Conceptually, I-pipe timers should have highest rating. Can you explain
in more details why it should be there?

Also note that your own code looks suspicious: percpu_timer is a per-cpu
variable but is not accesses as such.

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux
Notice: This e-mail and any files transmitted with it may contain Data Device Corporation's privileged and proprietary information. It is intended solely for the use of the individual or entity to whom it is addressed. If you are not the named recipient of this transmission, any disclosure, copying, distribution or reliance on the contents of this message is prohibited. If you received this e-mail in error, please destroy it and any attached files and notify me immediately.

Re: [Xenomai] potentially missing timer rating check in select_root_only_timer()

[Jan Kiszka]

On 2018-05-02 19:43, Pham, Phong wrote:
> 
> Hi Jan,
> 
> In my scenario, it is a single CPU Powerpc system.  What I was intending to do is replacing the decrementer (Powerpc specific count down timer) clockevent with my timer clockevent.  (My timer is located somewhere else on the chip accessible via memory mapped IO).   According to Linux, user can change the rating and kernel will select the highest rating timer for you.  So I change my timer rating to be higher than the decrementer.  In ipipe_select_timers(), it will pick the first timer found on the timers linked list.  That linked list of timers is also sorted by highest priority first.  However, when select_root_only_timer() is executed, the code goes thru the entire list and select the last one, the one with the lowest rating.

OK, I'm starting to see the problem:

1. You have some CPUs that are not used by Xenomai (ipipe_select_timers,
   "fixup" mask is non-empty)

2. There are multiple ipipe-timers available, sorted according to their
   rating in "timers" list (higher ratings first).

3. select_root_only_timer is now called for each timer in the list, and
   if the timer matches regarding the IRQ, it will be registered
   (install_pcpu_timer).

Due to this multi-registration (actually overwriting), you end up with
the lowest rated timer installed.

So the proper fix is to prevent further calls to select_root_only_timer
after the first successful one. Would you write such a patch? Should be
about making select_root_only_timer return some true/false, depending on
whether it actually installed something.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux