1120471 - Support timers in libfdio

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

We will need timers in bluetoothd for fixing bug 1119176. By implementing them in libfdio, we can safely integrate them into the main I/O loop and won't need ad-hoc solutions.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

This patch adds timers to libfdio. The code requires at least Kitkat, but that should not be a problem for now. We will need this feature for bluetoothd on Lollipop. Bug 1119176 has more information.

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: Github pull request

Updated Github pull request:

  - periodic timer bugfix from Bruce

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8548857 [details]
Github pull request

Updated Github pull request:

  - added spaces around | operators
  - added separate API functions for absolute and relative timers

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

2.2?, because this bug is a dependency for bug 1119176, which already has 2.2+.

Comment 5

[Michael Wu [:mwu]]

Hmm this API is still confusing to me. In add_timer_to_epoll_loop, what the heck does frequency mean if it's not periodic?

Also, I wasn't asking for a fallback on kernels that don't support this syscall. I was asking to support this where the syscall is available, regardless of whether our libc directly supports it.

Comment 6

[Bruce Sun [:brsun]]

How about:
 - a. using the same parameter list of |add_absolute_timer_to_epoll_loop()| for |add_timer_to_epoll_loop()|; or
 - b. merging |add_absolute_timer_to_epoll_loop()| and |add_timer_to_epoll_loop()| into one function but adding one additional input parameter to distinguish between an absolute timer and an relative timer?

Comment 7

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Michael Wu [:mwu] from comment #5)
> Hmm this API is still confusing to me. In add_timer_to_epoll_loop, what the
> heck does frequency mean if it's not periodic?

'frequency' is the duration from the current time until the timer fires. Maybe 'interval' is a better term. Non-periodic timers will fire once, periodic timers in regular intervals.

> Also, I wasn't asking for a fallback on kernels that don't support this
> syscall. I was asking to support this where the syscall is available,
> regardless of whether our libc directly supports it.

As I mentioned, yes it can be build with syscalls. The timerfd code was added in Linux 2.6.22, so all our kernels should support it.

The rest of my reply just meant that it might be better to emulate the API with something portable, than to use platform-specific syscalls.

Comment 8

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8548857 [details]
Github pull request

Updated Github pull request

  - use 'interval' instead of 'frequency' throughout timer.{c,h}
  - moved setup of timespec structure to separate function
  - replaced plain numeric values by constants

Comment 9

[Michael Wu [:mwu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #7)
> > Also, I wasn't asking for a fallback on kernels that don't support this
> > syscall. I was asking to support this where the syscall is available,
> > regardless of whether our libc directly supports it.
> 
> As I mentioned, yes it can be build with syscalls. The timerfd code was
> added in Linux 2.6.22, so all our kernels should support it.
> 

I think this should be done instead of not supporting ANDROID_VERSION < 19. Basically, implement the calls ourselves via syscall when ANDROID_VERSION < 19. Not supporting timer functionality based on the bionic version seems too surprising.

> The rest of my reply just meant that it might be better to emulate the API
> with something portable, than to use platform-specific syscalls.

Oh, but isn't this API better? I don't think we should pretend to support anything but Linux, but if we do, we should support it using the best available API.

Back to the API being added to libfdio..

s/frequency/interval/ makes the API make more sense, but it still doesn't seem ideal. Some objections:
- periodic is being used as a bool. AFAICT bools aren't great in function args, since it's hard to easily determine what it means without referencing the docs. Wrapping the function in two wrapper functions that specifies the difference in the name is one way to avoid this, which is what I initially meant by splitting the function.
- The API is too different between add_timer_to_epoll_loop and add_absolute_timer_to_epoll_loop. At a glance, you would expect the only difference to be whether the time specified for the first timer firing is specified in relative or absolute time. Not true! They're different apis, and one of them doesn't allow the interval to be specified separately from first fire time. brsun's suggestion addresses this.

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8548857 [details]
Github pull request

Updated Github pull request

  - added timerfd support for Android APIs 14 to 18
  - cleaned up API according to previous review

Timerfd support for older releases was a lot less work than I expected. These functions are just wrappers around the system calls and simply forward the arguments into the kernel. The |syscall| function does the same in a portable way. On x86, |syscall| loads 6 arguments into registers when doing the system call. So when I call |syscall| I use '0' for unused arguments.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 8548857 [details]
Github pull request

Ok, that makes sense.

BTW, I think you can remove the ATTRIBS(UNUSED) on the add_timer and remove_timer functions since we just assume there's support now.

The makefile logic seems slightly verbose.. how about:

ifneq ($(filter 14 15 16 17,$(PLATFORM_SDK_VERSION)),)
    local_source_files := ports/timerfd.c
endif

Up to you though, I don't care about that too much.

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8548857 [details]
Github pull request

Updated Github pull request

  - removed unused ATTRIB statements in timer.c

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks Michael.

Comment 14

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/platform_system_libfdio/commit/8fcd25d64f0f67d1a6f7037a4c83ce6d95466770

Comment 15

[Bruce Sun [:brsun]]

Hi Thomas, will you request for the approval to uplift this patch?

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8548857 [details]
Github pull request

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Bug 1119176. This patch set is part of the Lillopop port.

User impact if declined: 

Bluetooth on L might not work reliably

Testing completed: 

Manual testing on local machines by running Bluetooth.

Risk to taking this patch (and alternatives if risky): 

Low. We can roll-back this change and bug 1119176 easily.

String or UUID changes made by this patch:

None.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/platform_system_libfdio/commit/20c8dbc33dafc442f9c5412a4a47585284e60866