ELL API changes before stabilizing

ELL API changes before stabilizing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1297 bytes --]


We will soon reach a point where breaking changes to the ELL APIs will get 
a lot more painful - changes already create a headache for the handful of 
projects we know about. Given that, I took a look over the public headers 
to see if there were any obvious adjustments to be made, and only found a 
few issues that jumped out at me.


1. timeout: extra nanosecond APIs with extremely_long_function_names. We 
could scrap the *_with_nanosecond calls and include nanoseconds in 
l_timeout_create and l_timeout_modify.


2. timeout: Why not use time_t for seconds? Yeah, 4 billion seconds should 
be enough for anyone, but the same argument could be made for the 
underlying struct timespec that's used for the system calls.


3. main: We recently changed l_main_exit() to be a cleanup function. It 
used to trigger an exit action. It would be more consistent to name the 
function for what it does rather than when it should be run 
(l_main_cleanup()?).


4. uintset: Why does uintset_new() create a set with min == 1? C 
programmers are more likely to assume it's 0-indexed. Maybe we don't need 
both l_uintset_new and l_uintset_new_from_range. One function that takes 
min and max seems sufficient.


Anything else you've noticed?

--
Mat Martineau
Intel OTC

Re: ELL API changes before stabilizing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2513 bytes --]

Hi Mat,

On 10/05/2016 04:45 PM, Mat Martineau wrote:
>
> We will soon reach a point where breaking changes to the ELL APIs will
> get a lot more painful - changes already create a headache for the
> handful of projects we know about. Given that, I took a look over the
> public headers to see if there were any obvious adjustments to be made,
> and only found a few issues that jumped out at me.
>
>
> 1. timeout: extra nanosecond APIs with extremely_long_function_names. We
> could scrap the *_with_nanosecond calls and include nanoseconds in
> l_timeout_create and l_timeout_modify.

So the background on this is that oFono/ConnMan/BlueZ tend to use 
g_timeout_add_seconds since it was supposedly cheaper than using the 
g_timeout_add version.  Hence in ell we used the seconds as the 
'preferred' version and introduced l_idle class as a g_timeout_add(0, 
...) replacement.

It is rare (non-existent in iwd & oFono) that we need timers that 
contain both seconds and microseconds, it is usually one-or-the-other 
situation.

Do we have any users of the nanosecond version? iwd doesn't use it 
currently.

>
>
> 2. timeout: Why not use time_t for seconds? Yeah, 4 billion seconds
> should be enough for anyone, but the same argument could be made for the
> underlying struct timespec that's used for the system calls.

What's the advantage besides requiring an extra #include? time_t is a 
long in the end.  So on 32-bit you're not buying anything, and on 64-bit 
you're in overkill territory.

>
>
> 3. main: We recently changed l_main_exit() to be a cleanup function. It
> used to trigger an exit action. It would be more consistent to name the
> function for what it does rather than when it should be run
> (l_main_cleanup()?).

We tend to use _init to init modules and _exit to tear them down.  So 
the naming sort of makes sense.  I also like the symmetry between 
l_main_init and l_main_exit.

>
>
> 4. uintset: Why does uintset_new() create a set with min == 1? C
> programmers are more likely to assume it's 0-indexed. Maybe we don't
> need both l_uintset_new and l_uintset_new_from_range. One function that
> takes min and max seems sufficient.
>

This class was a straight up port from ofono's idmap utility.  It is 
generally used where 0 is not a valid value.  But I agree that having 
two constructors is redundant.

>
> Anything else you've noticed?
>

Thanks for doing this btw, its still on my TODO list.

Regards,
-Denis

Re: ELL API changes before stabilizing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 4185 bytes --]


On Wed, 5 Oct 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 10/05/2016 04:45 PM, Mat Martineau wrote:
>> 
>> We will soon reach a point where breaking changes to the ELL APIs will
>> get a lot more painful - changes already create a headache for the
>> handful of projects we know about. Given that, I took a look over the
>> public headers to see if there were any obvious adjustments to be made,
>> and only found a few issues that jumped out at me.
>> 
>> 
>> 1. timeout: extra nanosecond APIs with extremely_long_function_names. We
>> could scrap the *_with_nanosecond calls and include nanoseconds in
>> l_timeout_create and l_timeout_modify.
>
> So the background on this is that oFono/ConnMan/BlueZ tend to use 
> g_timeout_add_seconds since it was supposedly cheaper than using the 
> g_timeout_add version.  Hence in ell we used the seconds as the 'preferred' 
> version and introduced l_idle class as a g_timeout_add(0, ...) replacement.

Until just now, I thought l_idle callbacks only ran when the event loop 
ran out of things to do (you know, when "idle"). It's good that it works 
the way it does since we don't want idle callbacks to get starved when 
there's lots of I/O, but add "l_idle" naming to my list as a (very) minor 
annoyance.

I think there's still a use case for a zero timeout when you want some 
recurring work to be triggered by either an event or the timer. It would 
be simpler to modify an existing timer with l_timeout_modify(0, ...) than 
it would be to remove the timer, use l_idle, and create an l_timeout again 
later. Right now this can be done by using a 1-nanosecond timeout, which 
isn't pretty. How about allowing 0 second/nanosecond values in 
l_timeout_modify but not _create?

> It is rare (non-existent in iwd & oFono) that we need timers that contain 
> both seconds and microseconds, it is usually one-or-the-other situation.
>
> Do we have any users of the nanosecond version? iwd doesn't use it currently.

Yes, that's why I cc'd Brian.

The nanosecond version has separate parameters for seconds and 
nanoseconds, and they are in use together to specify timeouts greater than 
one second with subsecond accuracy. So my proposal is to get rid of the 
seconds-only version and always include the nanoseconds (which can be 
easily set to 0).

struct l_timeout *l_timeout_create(time_t seconds,
 			long nanoseconds, l_timeout_notify_cb_t callback,
 			void *user_data, l_timeout_destroy_cb_t destroy);
void l_timeout_modify(struct l_timeout *timeout,
                                 unsigned int seconds, long nanoseconds);

>
>> 
>> 
>> 2. timeout: Why not use time_t for seconds? Yeah, 4 billion seconds
>> should be enough for anyone, but the same argument could be made for the
>> underlying struct timespec that's used for the system calls.
>
> What's the advantage besides requiring an extra #include? time_t is a long in 
> the end.  So on 32-bit you're not buying anything, and on 64-bit you're in 
> overkill territory.

For relative timestamps, it's not a big deal. 136 years is a long time, 
even if it's not always a long int.

>> 3. main: We recently changed l_main_exit() to be a cleanup function. It
>> used to trigger an exit action. It would be more consistent to name the
>> function for what it does rather than when it should be run
>> (l_main_cleanup()?).
>
> We tend to use _init to init modules and _exit to tear them down.  So the 
> naming sort of makes sense.  I also like the symmetry between l_main_init and 
> l_main_exit.
>
>> 
>> 
>> 4. uintset: Why does uintset_new() create a set with min == 1? C
>> programmers are more likely to assume it's 0-indexed. Maybe we don't
>> need both l_uintset_new and l_uintset_new_from_range. One function that
>> takes min and max seems sufficient.
>> 
>
> This class was a straight up port from ofono's idmap utility.  It is 
> generally used where 0 is not a valid value.  But I agree that having two 
> constructors is redundant.
>
>> 
>> Anything else you've noticed?
>> 
>
> Thanks for doing this btw, its still on my TODO list.



--
Mat Martineau
Intel OTC

Re: ELL API changes before stabilizing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3907 bytes --]

Hi Mat,

On 10/06/2016 11:36 AM, Mat Martineau wrote:
>
> On Wed, 5 Oct 2016, Denis Kenzior wrote:
>
>> Hi Mat,
>>
>> On 10/05/2016 04:45 PM, Mat Martineau wrote:
>>>
>>> We will soon reach a point where breaking changes to the ELL APIs will
>>> get a lot more painful - changes already create a headache for the
>>> handful of projects we know about. Given that, I took a look over the
>>> public headers to see if there were any obvious adjustments to be made,
>>> and only found a few issues that jumped out at me.
>>>
>>>
>>> 1. timeout: extra nanosecond APIs with extremely_long_function_names. We
>>> could scrap the *_with_nanosecond calls and include nanoseconds in
>>> l_timeout_create and l_timeout_modify.
>>
>> So the background on this is that oFono/ConnMan/BlueZ tend to use
>> g_timeout_add_seconds since it was supposedly cheaper than using the
>> g_timeout_add version.  Hence in ell we used the seconds as the
>> 'preferred' version and introduced l_idle class as a g_timeout_add(0,
>> ...) replacement.
>
> Until just now, I thought l_idle callbacks only ran when the event loop
> ran out of things to do (you know, when "idle"). It's good that it works
> the way it does since we don't want idle callbacks to get starved when
> there's lots of I/O, but add "l_idle" naming to my list as a (very)
> minor annoyance.

l_idle is simply invoked the next time the event loop runs.  If we have 
idle objects scheduled, then the event loop will not sleep inside epoll, 
but instead run a tight-loop polling for file descriptor events. 
g_timeout_add(0, ...) is generally used to get out of re-entrancy 
conditions, which are tricky and lead to crashes.

>
> I think there's still a use case for a zero timeout when you want some
> recurring work to be triggered by either an event or the timer. It would

l_idle will call the callback until l_idle_remove is called. 
l_idle_oneshot is somewhat equivalent of g_timeout_add(0, ...)

> be simpler to modify an existing timer with l_timeout_modify(0, ...)
> than it would be to remove the timer, use l_idle, and create an
> l_timeout again later. Right now this can be done by using a
> 1-nanosecond timeout, which isn't pretty. How about allowing 0
> second/nanosecond values in l_timeout_modify but not _create?
>

Do we have an actual usecase ?  I'm failing to see the utility, but I 
may be missing something.

>> It is rare (non-existent in iwd & oFono) that we need timers that
>> contain both seconds and microseconds, it is usually one-or-the-other
>> situation.
>>
>> Do we have any users of the nanosecond version? iwd doesn't use it
>> currently.
>
> Yes, that's why I cc'd Brian.
>
> The nanosecond version has separate parameters for seconds and
> nanoseconds, and they are in use together to specify timeouts greater
> than one second with subsecond accuracy. So my proposal is to get rid of
> the seconds-only version and always include the nanoseconds (which can
> be easily set to 0).
>
> struct l_timeout *l_timeout_create(time_t seconds,
>              long nanoseconds, l_timeout_notify_cb_t callback,
>              void *user_data, l_timeout_destroy_cb_t destroy);
> void l_timeout_modify(struct l_timeout *timeout,
>                                  unsigned int seconds, long nanoseconds);
>

So then everyone pays the price of specifying seconds and nanoseconds 
when in reality 75% of calls will be the second version, 20% will be 
nanosecond version.  And the 5% outlier actually wants to specify both.

Not strictly against this, but just pointing out that ell API is 
supposed to be optimized for our actual usecases.  Perhaps we should just do
l_timeout_create, l_timeout_create_ms (oFono abuses sub-second timers, 
but it has zero need for nanosecond precision) and l_timeout_create_full 
to shorten the name.

Regards,
-Denis

Re: ELL API changes before stabilizing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 5371 bytes --]


On Thu, 6 Oct 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 10/06/2016 11:36 AM, Mat Martineau wrote:
>> 
>> On Wed, 5 Oct 2016, Denis Kenzior wrote:
>> 
>>> Hi Mat,
>>> 
>>> On 10/05/2016 04:45 PM, Mat Martineau wrote:
>>>> 
>>>> We will soon reach a point where breaking changes to the ELL APIs will
>>>> get a lot more painful - changes already create a headache for the
>>>> handful of projects we know about. Given that, I took a look over the
>>>> public headers to see if there were any obvious adjustments to be made,
>>>> and only found a few issues that jumped out at me.
>>>> 
>>>> 
>>>> 1. timeout: extra nanosecond APIs with extremely_long_function_names. We
>>>> could scrap the *_with_nanosecond calls and include nanoseconds in
>>>> l_timeout_create and l_timeout_modify.
>>> 
>>> So the background on this is that oFono/ConnMan/BlueZ tend to use
>>> g_timeout_add_seconds since it was supposedly cheaper than using the
>>> g_timeout_add version.  Hence in ell we used the seconds as the
>>> 'preferred' version and introduced l_idle class as a g_timeout_add(0,
>>> ...) replacement.
>> 
>> Until just now, I thought l_idle callbacks only ran when the event loop
>> ran out of things to do (you know, when "idle"). It's good that it works
>> the way it does since we don't want idle callbacks to get starved when
>> there's lots of I/O, but add "l_idle" naming to my list as a (very)
>> minor annoyance.
>
> l_idle is simply invoked the next time the event loop runs.  If we have idle 
> objects scheduled, then the event loop will not sleep inside epoll, but 
> instead run a tight-loop polling for file descriptor events. g_timeout_add(0, 
> ...) is generally used to get out of re-entrancy conditions, which are tricky 
> and lead to crashes.

Now that I've read the timerfd_create man page more closely... nevermind. 
Setting both tv_sec and tv_nsec to 0 disarms the timer.

>
>> 
>> I think there's still a use case for a zero timeout when you want some
>> recurring work to be triggered by either an event or the timer. It would
>
> l_idle will call the callback until l_idle_remove is called. l_idle_oneshot 
> is somewhat equivalent of g_timeout_add(0, ...)
>
>> be simpler to modify an existing timer with l_timeout_modify(0, ...)
>> than it would be to remove the timer, use l_idle, and create an
>> l_timeout again later. Right now this can be done by using a
>> 1-nanosecond timeout, which isn't pretty. How about allowing 0
>> second/nanosecond values in l_timeout_modify but not _create?
>> 
>
> Do we have an actual usecase ?  I'm failing to see the utility, but I may be 
> missing something.

Since a zero timeout disables the timer, the "immediate timeout" use case 
is moot. l_idle_oneshot does the trick where something needs to be 
scheduled immediately but not on the current stack.

>>> It is rare (non-existent in iwd & oFono) that we need timers that
>>> contain both seconds and microseconds, it is usually one-or-the-other
>>> situation.
>>> 
>>> Do we have any users of the nanosecond version? iwd doesn't use it
>>> currently.
>> 
>> Yes, that's why I cc'd Brian.
>> 
>> The nanosecond version has separate parameters for seconds and
>> nanoseconds, and they are in use together to specify timeouts greater
>> than one second with subsecond accuracy. So my proposal is to get rid of
>> the seconds-only version and always include the nanoseconds (which can
>> be easily set to 0).
>> 
>> struct l_timeout *l_timeout_create(time_t seconds,
>>              long nanoseconds, l_timeout_notify_cb_t callback,
>>              void *user_data, l_timeout_destroy_cb_t destroy);
>> void l_timeout_modify(struct l_timeout *timeout,
>>                                  unsigned int seconds, long nanoseconds);
>> 
>
> So then everyone pays the price of specifying seconds and nanoseconds when in 
> reality 75% of calls will be the second version, 20% will be nanosecond 
> version.  And the 5% outlier actually wants to specify both.

The nanoseconds calls don't pay any price, they *already* have both 
seconds and nanoseconds (nanoseconds value must be less than 1000000000). 
Look at the existing timeout.h - all I did in those function prototypes 
above is remove "_with_nanoseconds".

My whole point here is that I see it as a favorable tradeoff to pass an 
extra '0' to the timeout APIs when subsecond resolution is not required 
rather than have superfluous API calls. It's only a matter of what ELL 
users see in their code, not code efficiency: the current whole-second 
APIs are just wrappers around full-resolution common code anyway. If the 
whole-seconds use case is overly impacted by the extra parameter, then the 
tradeoff may not be favorable.

> Not strictly against this, but just pointing out that ell API is supposed to 
> be optimized for our actual usecases.  Perhaps we should just do
> l_timeout_create, l_timeout_create_ms (oFono abuses sub-second timers, but it 
> has zero need for nanosecond precision) and l_timeout_create_full to shorten 
> the name.

I agree that nanoseconds are overkill, but I don't mind seeing the 
platform detail filter up to the ELL API level. In practice, milliseconds 
are likely sufficient and would save a lot of '0' keypresses.

--
Mat Martineau
Intel OTC

Re: ELL API changes before stabilizing

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 5653 bytes --]

Hi Mat,

>>>>> We will soon reach a point where breaking changes to the ELL APIs will
>>>>> get a lot more painful - changes already create a headache for the
>>>>> handful of projects we know about. Given that, I took a look over the
>>>>> public headers to see if there were any obvious adjustments to be made,
>>>>> and only found a few issues that jumped out at me.
>>>>> 1. timeout: extra nanosecond APIs with extremely_long_function_names. We
>>>>> could scrap the *_with_nanosecond calls and include nanoseconds in
>>>>> l_timeout_create and l_timeout_modify.
>>>> So the background on this is that oFono/ConnMan/BlueZ tend to use
>>>> g_timeout_add_seconds since it was supposedly cheaper than using the
>>>> g_timeout_add version.  Hence in ell we used the seconds as the
>>>> 'preferred' version and introduced l_idle class as a g_timeout_add(0,
>>>> ...) replacement.
>>> Until just now, I thought l_idle callbacks only ran when the event loop
>>> ran out of things to do (you know, when "idle"). It's good that it works
>>> the way it does since we don't want idle callbacks to get starved when
>>> there's lots of I/O, but add "l_idle" naming to my list as a (very)
>>> minor annoyance.
>> 
>> l_idle is simply invoked the next time the event loop runs.  If we have idle objects scheduled, then the event loop will not sleep inside epoll, but instead run a tight-loop polling for file descriptor events. g_timeout_add(0, ...) is generally used to get out of re-entrancy conditions, which are tricky and lead to crashes.
> 
> Now that I've read the timerfd_create man page more closely... nevermind. Setting both tv_sec and tv_nsec to 0 disarms the timer.
> 
>> 
>>> I think there's still a use case for a zero timeout when you want some
>>> recurring work to be triggered by either an event or the timer. It would
>> 
>> l_idle will call the callback until l_idle_remove is called. l_idle_oneshot is somewhat equivalent of g_timeout_add(0, ...)
>> 
>>> be simpler to modify an existing timer with l_timeout_modify(0, ...)
>>> than it would be to remove the timer, use l_idle, and create an
>>> l_timeout again later. Right now this can be done by using a
>>> 1-nanosecond timeout, which isn't pretty. How about allowing 0
>>> second/nanosecond values in l_timeout_modify but not _create?
>> 
>> Do we have an actual usecase ?  I'm failing to see the utility, but I may be missing something.
> 
> Since a zero timeout disables the timer, the "immediate timeout" use case is moot. l_idle_oneshot does the trick where something needs to be scheduled immediately but not on the current stack.
> 
>>>> It is rare (non-existent in iwd & oFono) that we need timers that
>>>> contain both seconds and microseconds, it is usually one-or-the-other
>>>> situation.
>>>> Do we have any users of the nanosecond version? iwd doesn't use it
>>>> currently.
>>> Yes, that's why I cc'd Brian.
>>> The nanosecond version has separate parameters for seconds and
>>> nanoseconds, and they are in use together to specify timeouts greater
>>> than one second with subsecond accuracy. So my proposal is to get rid of
>>> the seconds-only version and always include the nanoseconds (which can
>>> be easily set to 0).
>>> struct l_timeout *l_timeout_create(time_t seconds,
>>>             long nanoseconds, l_timeout_notify_cb_t callback,
>>>             void *user_data, l_timeout_destroy_cb_t destroy);
>>> void l_timeout_modify(struct l_timeout *timeout,
>>>                                 unsigned int seconds, long nanoseconds);
>> 
>> So then everyone pays the price of specifying seconds and nanoseconds when in reality 75% of calls will be the second version, 20% will be nanosecond version. And the 5% outlier actually wants to specify both.
> 
> The nanoseconds calls don't pay any price, they *already* have both seconds and nanoseconds (nanoseconds value must be less than 1000000000). Look at the existing timeout.h - all I did in those function prototypes above is remove "_with_nanoseconds".
> 
> My whole point here is that I see it as a favorable tradeoff to pass an extra '0' to the timeout APIs when subsecond resolution is not required rather than have superfluous API calls. It's only a matter of what ELL users see in their code, not code efficiency: the current whole-second APIs are just wrappers around full-resolution common code anyway. If the whole-seconds use case is overly impacted by the extra parameter, then the tradeoff may not be favorable.
> 
>> Not strictly against this, but just pointing out that ell API is supposed to be optimized for our actual usecases.  Perhaps we should just do
>> l_timeout_create, l_timeout_create_ms (oFono abuses sub-second timers, but it has zero need for nanosecond precision) and l_timeout_create_full to shorten the name.
> 
> I agree that nanoseconds are overkill, but I don't mind seeing the platform detail filter up to the ELL API level. In practice, milliseconds are likely sufficient and would save a lot of '0' keypresses.

we actually wanted the timerfd API to expose also range timers. In the kernel it is possible to specify a range and then the kernel will coalesce the wakeup time. That is what we really want. One that coalesces for us and another that is precise. The seconds vs nanoseconds is most likely not a good representation of that and we should just switch to using milliseconds, but as range timeout and one as precise timeout.

I am also not convinced that creating a timerfd for each struct is a good idea. It might be better to share timerfd and update timeout wakeups.

Regards

Marcel

RE: ELL API changes before stabilizing

[Gix, Brian]

[-- Attachment #1: Type: text/plain, Size: 5968 bytes --]

To advocate for Bluetooth Mesh, we have a number of situations where we have Random Timers between 0 - 1.5 seconds, or 0-10 seconds, where we want sub-second increments to keep multiple and many rf devices from talking over each other.

-----Original Message-----
From: Mat Martineau [mailto:mathew.j.martineau(a)linux.intel.com] 
Sent: Thursday, October 6, 2016 11:16 AM
To: Denis Kenzior <denkenz@gmail.com>
Cc: ell(a)lists.01.org; Gix, Brian <brian.gix@intel.com>; Zaborowski, Andrew <andrew.zaborowski@intel.com>
Subject: Re: ELL API changes before stabilizing


On Thu, 6 Oct 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 10/06/2016 11:36 AM, Mat Martineau wrote:
>> 
>> On Wed, 5 Oct 2016, Denis Kenzior wrote:
>> 
>>> Hi Mat,
>>> 
>>> On 10/05/2016 04:45 PM, Mat Martineau wrote:
>>>> 
>>>> We will soon reach a point where breaking changes to the ELL APIs 
>>>> will get a lot more painful - changes already create a headache for 
>>>> the handful of projects we know about. Given that, I took a look 
>>>> over the public headers to see if there were any obvious 
>>>> adjustments to be made, and only found a few issues that jumped out at me.
>>>> 
>>>> 
>>>> 1. timeout: extra nanosecond APIs with 
>>>> extremely_long_function_names. We could scrap the *_with_nanosecond 
>>>> calls and include nanoseconds in l_timeout_create and l_timeout_modify.
>>> 
>>> So the background on this is that oFono/ConnMan/BlueZ tend to use 
>>> g_timeout_add_seconds since it was supposedly cheaper than using the 
>>> g_timeout_add version.  Hence in ell we used the seconds as the 
>>> 'preferred' version and introduced l_idle class as a 
>>> g_timeout_add(0,
>>> ...) replacement.
>> 
>> Until just now, I thought l_idle callbacks only ran when the event 
>> loop ran out of things to do (you know, when "idle"). It's good that 
>> it works the way it does since we don't want idle callbacks to get 
>> starved when there's lots of I/O, but add "l_idle" naming to my list 
>> as a (very) minor annoyance.
>
> l_idle is simply invoked the next time the event loop runs.  If we 
> have idle objects scheduled, then the event loop will not sleep inside 
> epoll, but instead run a tight-loop polling for file descriptor 
> events. g_timeout_add(0,
> ...) is generally used to get out of re-entrancy conditions, which are 
> tricky and lead to crashes.

Now that I've read the timerfd_create man page more closely... nevermind. 
Setting both tv_sec and tv_nsec to 0 disarms the timer.

>
>> 
>> I think there's still a use case for a zero timeout when you want 
>> some recurring work to be triggered by either an event or the timer. 
>> It would
>
> l_idle will call the callback until l_idle_remove is called. 
> l_idle_oneshot is somewhat equivalent of g_timeout_add(0, ...)
>
>> be simpler to modify an existing timer with l_timeout_modify(0, ...) 
>> than it would be to remove the timer, use l_idle, and create an 
>> l_timeout again later. Right now this can be done by using a 
>> 1-nanosecond timeout, which isn't pretty. How about allowing 0 
>> second/nanosecond values in l_timeout_modify but not _create?
>> 
>
> Do we have an actual usecase ?  I'm failing to see the utility, but I 
> may be missing something.

Since a zero timeout disables the timer, the "immediate timeout" use case is moot. l_idle_oneshot does the trick where something needs to be scheduled immediately but not on the current stack.

>>> It is rare (non-existent in iwd & oFono) that we need timers that 
>>> contain both seconds and microseconds, it is usually 
>>> one-or-the-other situation.
>>> 
>>> Do we have any users of the nanosecond version? iwd doesn't use it 
>>> currently.
>> 
>> Yes, that's why I cc'd Brian.
>> 
>> The nanosecond version has separate parameters for seconds and 
>> nanoseconds, and they are in use together to specify timeouts greater 
>> than one second with subsecond accuracy. So my proposal is to get rid 
>> of the seconds-only version and always include the nanoseconds (which 
>> can be easily set to 0).
>> 
>> struct l_timeout *l_timeout_create(time_t seconds,
>>              long nanoseconds, l_timeout_notify_cb_t callback,
>>              void *user_data, l_timeout_destroy_cb_t destroy); void 
>> l_timeout_modify(struct l_timeout *timeout,
>>                                  unsigned int seconds, long 
>> nanoseconds);
>> 
>
> So then everyone pays the price of specifying seconds and nanoseconds 
> when in reality 75% of calls will be the second version, 20% will be 
> nanosecond version.  And the 5% outlier actually wants to specify both.

The nanoseconds calls don't pay any price, they *already* have both seconds and nanoseconds (nanoseconds value must be less than 1000000000). 
Look at the existing timeout.h - all I did in those function prototypes above is remove "_with_nanoseconds".

My whole point here is that I see it as a favorable tradeoff to pass an extra '0' to the timeout APIs when subsecond resolution is not required rather than have superfluous API calls. It's only a matter of what ELL users see in their code, not code efficiency: the current whole-second APIs are just wrappers around full-resolution common code anyway. If the whole-seconds use case is overly impacted by the extra parameter, then the tradeoff may not be favorable.

> Not strictly against this, but just pointing out that ell API is 
> supposed to be optimized for our actual usecases.  Perhaps we should 
> just do l_timeout_create, l_timeout_create_ms (oFono abuses sub-second 
> timers, but it has zero need for nanosecond precision) and 
> l_timeout_create_full to shorten the name.

I agree that nanoseconds are overkill, but I don't mind seeing the platform detail filter up to the ELL API level. In practice, milliseconds are likely sufficient and would save a lot of '0' keypresses.

--
Mat Martineau
Intel OTC

Re: ELL API changes before stabilizing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

Hi Mat,

 >
> My whole point here is that I see it as a favorable tradeoff to pass an
> extra '0' to the timeout APIs when subsecond resolution is not required
> rather than have superfluous API calls. It's only a matter of what ELL
> users see in their code, not code efficiency: the current whole-second
> APIs are just wrappers around full-resolution common code anyway. If the
> whole-seconds use case is overly impacted by the extra parameter, then
> the tradeoff may not be favorable.

That is really what I meant.  I want to save the users from typing the 
extra 0 parameter unnecessarily.  We already have 4 arguments to 
l_timeout_create, with lots of potential zeros.

> I agree that nanoseconds are overkill, but I don't mind seeing the
> platform detail filter up to the ELL API level. In practice,
> milliseconds are likely sufficient and would save a lot of '0' keypresses.
>

Hence my question.  Do we actually need to expose nanoseconds?  I can't 
believe someone needs that level of precision.  MS is fine.

If we add the range stuff in, then that's another argument or two, 
depending on representation.

I actually don't mind multiple constructors. They're just shorthands and 
are easier to read than trying to parse the arguments.  We can include a 
range fudge factor as part of the API.  e.g. second version of 
timeout_create fudges by +/- 500ms.  MS versions can fudge by +/- 500 
nanoseconds, etc.

And if someone wants full control, then a full-blown provide-everything 
constructor can be added.

Regards,
-Denis

Re: ELL API changes before stabilizing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2192 bytes --]


On Thu, 6 Oct 2016, Denis Kenzior wrote:

> Hi Mat,
>
>>
>> My whole point here is that I see it as a favorable tradeoff to pass an
>> extra '0' to the timeout APIs when subsecond resolution is not required
>> rather than have superfluous API calls. It's only a matter of what ELL
>> users see in their code, not code efficiency: the current whole-second
>> APIs are just wrappers around full-resolution common code anyway. If the
>> whole-seconds use case is overly impacted by the extra parameter, then
>> the tradeoff may not be favorable.
>
> That is really what I meant.  I want to save the users from typing the extra 
> 0 parameter unnecessarily.  We already have 4 arguments to l_timeout_create, 
> with lots of potential zeros.

Ok, sounds fine.

>> I agree that nanoseconds are overkill, but I don't mind seeing the
>> platform detail filter up to the ELL API level. In practice,
>> milliseconds are likely sufficient and would save a lot of '0' keypresses.
>> 
>
> Hence my question.  Do we actually need to expose nanoseconds?  I can't 
> believe someone needs that level of precision.  MS is fine.

Sounds like we're in agreement that nanoseconds are overkill and ms is 
sufficient.

> If we add the range stuff in, then that's another argument or two, depending 
> on representation.
>
> I actually don't mind multiple constructors. They're just shorthands and are 
> easier to read than trying to parse the arguments.  We can include a range 
> fudge factor as part of the API.  e.g. second version of timeout_create 
> fudges by +/- 500ms.  MS versions can fudge by +/- 500 nanoseconds, etc.
>
> And if someone wants full control, then a full-blown provide-everything 
> constructor can be added.

Works for me. Do we have a consensus around removing the nanosecond 
versions and adding:

struct l_timeout *l_timeout_create_ms(unsigned long milliseconds,
 					l_timeout_notify_cb_t callback,
                         		void *user_data,
 					l_timeout_destroy_cb_t destroy);
void l_timeout_modify_ms(struct l_timeout *timeout,
                                 unsigned long milliseconds);


--
Mat Martineau
Intel OTC

Re: ELL API changes before stabilizing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

Hi Mat,

 >
> Works for me. Do we have a consensus around removing the nanosecond
> versions and adding:
>
> struct l_timeout *l_timeout_create_ms(unsigned long milliseconds,
>                      l_timeout_notify_cb_t callback,
>                                  void *user_data,
>                      l_timeout_destroy_cb_t destroy);
> void l_timeout_modify_ms(struct l_timeout *timeout,
>                                  unsigned long milliseconds);
>

This looks fine to me.

Regards,
-Denis

Re: ELL API changes before stabilizing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1481 bytes --]


On Thu, 6 Oct 2016, Marcel Holtmann wrote:

> we actually wanted the timerfd API to expose also range timers. In the 
> kernel it is possible to specify a range and then the kernel will 
> coalesce the wakeup time. That is what we really want. One that 
> coalesces for us and another that is precise. The seconds vs nanoseconds 
> is most likely not a good representation of that and we should just 
> switch to using milliseconds, but as range timeout and one as precise 
> timeout.

Denis makes the case that multiple constructors aren't so bad, and coarse 
(1 sec resolution) timeouts seem to be a common enough need.

timerfd needs some work to use the underlying hrtimer range capabilities. 
One approach is to add a TFD_SLACK flag that would make that timer use the 
per-thread slack value set by prctl(PR_SET_TIMERSLACK, ...). That only 
gives us one precise timer pool and one slack timer pool, though.

> I am also not convinced that creating a timerfd for each struct is a 
> good idea. It might be better to share timerfd and update timeout 
> wakeups.

It would make sense to consume an fd only for the next timer to expire. We 
would probably switch to using TFD_TIMER_ABSTIME instead of relative time 
values in timerfd_settime to prevent the later timers from drifting, which 
would require additional clock_gettime() syscalls. Another benefit would 
be the opportunity to handle some coalescing in ELL.

--
Mat Martineau
Intel OTC