On Sat, Feb 13, 2021 at 10:28:32AM EST, Richard Cochran wrote:
>On Mon, Feb 01, 2021 at 05:55:41PM -0500, vincent.cheng...@renesas.com wrote:
>
>> + /* Set up timer expire notification mechanism */
>> + se.sigev_notify = SIGEV_THREAD;
>> + se.sigev_value.sival_ptr = (void *)c;
>> + se.sigev_notify_function = clock_clear_free_running;
>> + se.sigev_notify_attributes = NULL;
>> +
>> + if (timer_create(CLOCK_MONOTONIC, &se, &c->step_timer_id)) {
>> + pr_err("failed to create step timer_id");
>> + return NULL;
>> + }
>
>I agree with the premise of this patch, but I dislike the
>implementation as it uses a timer with asynchronous signal.
>
>The code already has multiple timers, and these are managed using the
>timerfd API. This is the modern way of handling asynchronous timers.
>
>I can see two better ways of implementing this new feature.
>
>1. If you really want a to use a timer and keep the option value as a
> time value, then increase N_CLOCK_PFD and add the timer to
> clock.pollfd.
>
> If you choose this option, please increase the resolution of the
> step_window to milliseconds. After all, the window of expected
> stale time stamps may be much shorter than one second!
>
>2. You could define the step_window in terms of the number of Sync
> events to skip. That reduces the implementation to a simple
> counter. The option would still relate to time, because a Sync
> rate of X means 2^X seconds.
>
>I prefer #2 because it is much simpler.
>
>Thoughts?
I will choose door #2 and get back to you.
Thank-you for the good feedback.
Vincent
_______________________________________________
Linuxptp-devel mailing list
Linuxptp-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxptp-devel
