Are you saying we should be good to merge this now?
On 19 June 2017 at 17:42, Bill Fischofer <bill.fischo...@linaro.org> wrote: > On Mon, Jun 19, 2017 at 4:19 PM, Honnappa Nagarahalli > <honnappa.nagaraha...@linaro.org> wrote: >> Hi Bill/Maxim, >> I do not see any further comments, can we merge this to api-next? >> >> Thanks, >> Honnappa > > Now that v1.15 is tagged we can unblock some of this pending work. > Next step is to merge the v1.15 changes back into api-next so that we > can continue with the merge of new APIs and features in preparation > for v1.16. > >> >> On 16 June 2017 at 10:59, Honnappa Nagarahalli >> <honnappa.nagaraha...@linaro.org> wrote: >>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> >>> >>> On 14 June 2017 at 14:34, Brian Brooks <brian.bro...@arm.com> wrote: >>>> Run timer pool processing on worker cores if the application hints >>>> that the scheduler will be used. This reduces the latency and jitter >>>> of the point at which timer pool processing begins. See [1] for details. >>>> >>>> [1] >>>> https://docs.google.com/document/d/1sY7rOxqCNu-bMqjBiT5_keAIohrX1ZW-eL0oGLAQ4OM/edit?usp=sharing >>>> >>>> Signed-off-by: Brian Brooks <brian.bro...@arm.com> >>>> Reviewed-by: Ola Liljedahl <ola.liljed...@arm.com> >>>> Reviewed-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com> >>>> --- >>>> >>>> ** There is a false positive checkpatch.pl warning ** >>>> >>>> v4: >>>> - Rebase against Bill's feature init patch >>>> >>>> v3: >>>> - Add rate limiting by scheduling rounds >>>> >>>> v2: >>>> - Reword 'worker_timers' to 'use_scheduler' >>>> - Use time instead of ticks >>>> >>>> platform/linux-generic/include/odp_internal.h | 2 +- >>>> .../linux-generic/include/odp_timer_internal.h | 24 +++++ >>>> platform/linux-generic/odp_init.c | 2 +- >>>> platform/linux-generic/odp_schedule.c | 3 + >>>> platform/linux-generic/odp_schedule_iquery.c | 3 + >>>> platform/linux-generic/odp_schedule_sp.c | 3 + >>>> platform/linux-generic/odp_timer.c | 112 >>>> +++++++++++++++++++-- >>>> 7 files changed, 138 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/platform/linux-generic/include/odp_internal.h >>>> b/platform/linux-generic/include/odp_internal.h >>>> index 90e2a629..404792cf 100644 >>>> --- a/platform/linux-generic/include/odp_internal.h >>>> +++ b/platform/linux-generic/include/odp_internal.h >>>> @@ -108,7 +108,7 @@ int odp_queue_term_global(void); >>>> int odp_crypto_init_global(void); >>>> int odp_crypto_term_global(void); >>>> >>>> -int odp_timer_init_global(void); >>>> +int odp_timer_init_global(const odp_init_t *params); >>>> int odp_timer_term_global(void); >>>> int odp_timer_disarm_all(void); >>>> >>>> diff --git a/platform/linux-generic/include/odp_timer_internal.h >>>> b/platform/linux-generic/include/odp_timer_internal.h >>>> index 91b12c54..67ee9fef 100644 >>>> --- a/platform/linux-generic/include/odp_timer_internal.h >>>> +++ b/platform/linux-generic/include/odp_timer_internal.h >>>> @@ -20,6 +20,12 @@ >>>> #include <odp_pool_internal.h> >>>> #include <odp/api/timer.h> >>>> >>>> +/* Minimum number of nanoseconds between checking timer pools. */ >>>> +#define CONFIG_TIMER_RUN_RATELIMIT_NS 100 >>>> + >>>> +/* Minimum number of scheduling rounds between checking timer pools. */ >>>> +#define CONFIG_TIMER_RUN_RATELIMIT_ROUNDS 1 >>>> + >>>> /** >>>> * Internal Timeout header >>>> */ >>>> @@ -35,4 +41,22 @@ typedef struct { >>>> odp_timer_t timer; >>>> } odp_timeout_hdr_t; >>>> >>>> +/* >>>> + * Whether to run timer pool processing 'inline' (on worker cores) or in >>>> + * background threads (thread-per-timerpool). >>>> + * >>>> + * If the application will use both scheduler and timer this flag is set >>>> + * to true, otherwise false. This application conveys this information via >>>> + * the 'not_used' bits in odp_init_t which are passed to >>>> odp_global_init(). >>>> + */ >>>> +extern odp_bool_t inline_timers; >>>> + >>>> +unsigned _timer_run(void); >>>> + >>>> +/* Static inline wrapper to minimize modification of schedulers. */ >>>> +static inline unsigned timer_run(void) >>>> +{ >>>> + return inline_timers ? _timer_run() : 0; >>>> +} >>>> + >>>> #endif >>>> diff --git a/platform/linux-generic/odp_init.c >>>> b/platform/linux-generic/odp_init.c >>>> index 62a1fbc2..8c17cbb0 100644 >>>> --- a/platform/linux-generic/odp_init.c >>>> +++ b/platform/linux-generic/odp_init.c >>>> @@ -241,7 +241,7 @@ int odp_init_global(odp_instance_t *instance, >>>> } >>>> stage = PKTIO_INIT; >>>> >>>> - if (odp_timer_init_global()) { >>>> + if (odp_timer_init_global(params)) { >>>> ODP_ERR("ODP timer init failed.\n"); >>>> goto init_failed; >>>> } >>>> diff --git a/platform/linux-generic/odp_schedule.c >>>> b/platform/linux-generic/odp_schedule.c >>>> index 011d4dc4..04d09981 100644 >>>> --- a/platform/linux-generic/odp_schedule.c >>>> +++ b/platform/linux-generic/odp_schedule.c >>>> @@ -23,6 +23,7 @@ >>>> #include <odp/api/packet_io.h> >>>> #include <odp_ring_internal.h> >>>> #include <odp_queue_internal.h> >>>> +#include <odp_timer_internal.h> >>>> >>>> /* Number of priority levels */ >>>> #define NUM_PRIO 8 >>>> @@ -998,6 +999,8 @@ static int schedule_loop(odp_queue_t *out_queue, >>>> uint64_t wait, >>>> int ret; >>>> >>>> while (1) { >>>> + timer_run(); >>>> + >>>> ret = do_schedule(out_queue, out_ev, max_num); >>>> >>>> if (ret) >>>> diff --git a/platform/linux-generic/odp_schedule_iquery.c >>>> b/platform/linux-generic/odp_schedule_iquery.c >>>> index bdf1a460..f7c411f6 100644 >>>> --- a/platform/linux-generic/odp_schedule_iquery.c >>>> +++ b/platform/linux-generic/odp_schedule_iquery.c >>>> @@ -23,6 +23,7 @@ >>>> #include <odp/api/thrmask.h> >>>> #include <odp/api/packet_io.h> >>>> #include <odp_config_internal.h> >>>> +#include <odp_timer_internal.h> >>>> >>>> /* Number of priority levels */ >>>> #define NUM_SCHED_PRIO 8 >>>> @@ -719,6 +720,8 @@ static int schedule_loop(odp_queue_t *out_queue, >>>> uint64_t wait, >>>> odp_time_t next, wtime; >>>> >>>> while (1) { >>>> + timer_run(); >>>> + >>>> count = do_schedule(out_queue, out_ev, max_num); >>>> >>>> if (count) >>>> diff --git a/platform/linux-generic/odp_schedule_sp.c >>>> b/platform/linux-generic/odp_schedule_sp.c >>>> index 91d70e3a..252d128d 100644 >>>> --- a/platform/linux-generic/odp_schedule_sp.c >>>> +++ b/platform/linux-generic/odp_schedule_sp.c >>>> @@ -15,6 +15,7 @@ >>>> #include <odp_align_internal.h> >>>> #include <odp_config_internal.h> >>>> #include <odp_ring_internal.h> >>>> +#include <odp_timer_internal.h> >>>> >>>> #define NUM_THREAD ODP_THREAD_COUNT_MAX >>>> #define NUM_QUEUE ODP_CONFIG_QUEUES >>>> @@ -517,6 +518,8 @@ static int schedule_multi(odp_queue_t *from, uint64_t >>>> wait, >>>> uint32_t qi; >>>> int num; >>>> >>>> + timer_run(); >>>> + >>>> cmd = sched_cmd(); >>>> >>>> if (cmd && cmd->s.type == CMD_PKTIO) { >>>> diff --git a/platform/linux-generic/odp_timer.c >>>> b/platform/linux-generic/odp_timer.c >>>> index cf610bfa..bf7f1acd 100644 >>>> --- a/platform/linux-generic/odp_timer.c >>>> +++ b/platform/linux-generic/odp_timer.c >>>> @@ -52,6 +52,7 @@ >>>> #include <odp/api/sync.h> >>>> #include <odp/api/time.h> >>>> #include <odp/api/timer.h> >>>> +#include <odp_time_internal.h> >>>> #include <odp_timer_internal.h> >>>> >>>> #define TMO_UNUSED ((uint64_t)0xFFFFFFFFFFFFFFFF) >>>> @@ -60,6 +61,8 @@ >>>> * for checking the freshness of received timeouts */ >>>> #define TMO_INACTIVE ((uint64_t)0x8000000000000000) >>>> >>>> +odp_bool_t inline_timers = false; >>>> + >>>> >>>> /****************************************************************************** >>>> * Mutual exclusion in the absence of CAS16 >>>> >>>> *****************************************************************************/ >>>> @@ -165,7 +168,8 @@ static inline void set_next_free(odp_timer *tim, >>>> uint32_t nf) >>>> >>>> *****************************************************************************/ >>>> >>>> typedef struct odp_timer_pool_s { >>>> -/* Put frequently accessed fields in the first cache line */ >>>> + odp_time_t prev_scan; /* Time when previous scan started */ >>>> + odp_time_t time_per_tick; /* Time per timer pool tick */ >>>> odp_atomic_u64_t cur_tick;/* Current tick value */ >>>> uint64_t min_rel_tck; >>>> uint64_t max_rel_tck; >>>> @@ -242,6 +246,8 @@ static odp_timer_pool_t odp_timer_pool_new(const char >>>> *name, >>>> ODP_ABORT("%s: timer pool shm-alloc(%zuKB) failed\n", >>>> name, (sz0 + sz1 + sz2) / 1024); >>>> odp_timer_pool *tp = (odp_timer_pool *)odp_shm_addr(shm); >>>> + tp->prev_scan = odp_time_global(); >>>> + tp->time_per_tick = odp_time_global_from_ns(param->res_ns); >>>> odp_atomic_init_u64(&tp->cur_tick, 0); >>>> >>>> if (name == NULL) { >>>> @@ -276,8 +282,10 @@ static odp_timer_pool_t odp_timer_pool_new(const char >>>> *name, >>>> tp->tp_idx = tp_idx; >>>> odp_spinlock_init(&tp->lock); >>>> timer_pool[tp_idx] = tp; >>>> - if (tp->param.clk_src == ODP_CLOCK_CPU) >>>> - itimer_init(tp); >>>> + if (!inline_timers) { >>>> + if (tp->param.clk_src == ODP_CLOCK_CPU) >>>> + itimer_init(tp); >>>> + } >>>> return tp; >>>> } >>>> >>>> @@ -306,11 +314,13 @@ static void odp_timer_pool_del(odp_timer_pool *tp) >>>> odp_spinlock_lock(&tp->lock); >>>> timer_pool[tp->tp_idx] = NULL; >>>> >>>> - /* Stop timer triggering */ >>>> - if (tp->param.clk_src == ODP_CLOCK_CPU) >>>> - itimer_fini(tp); >>>> + if (!inline_timers) { >>>> + /* Stop POSIX itimer signals */ >>>> + if (tp->param.clk_src == ODP_CLOCK_CPU) >>>> + itimer_fini(tp); >>>> >>>> - stop_timer_thread(tp); >>>> + stop_timer_thread(tp); >>>> + } >>>> >>>> if (tp->num_alloc != 0) { >>>> /* It's a programming error to attempt to destroy a */ >>>> @@ -671,6 +681,81 @@ static unsigned >>>> odp_timer_pool_expire(odp_timer_pool_t tpid, uint64_t tick) >>>> } >>>> >>>> >>>> /****************************************************************************** >>>> + * Inline timer processing >>>> + >>>> *****************************************************************************/ >>>> + >>>> +static unsigned process_timer_pools(void) >>>> +{ >>>> + odp_timer_pool *tp; >>>> + odp_time_t prev_scan, now; >>>> + uint64_t nticks; >>>> + unsigned nexp = 0; >>>> + >>>> + for (size_t i = 0; i < MAX_TIMER_POOLS; i++) { >>>> + tp = timer_pool[i]; >>>> + >>>> + if (tp == NULL) >>>> + continue; >>>> + >>>> + /* >>>> + * Check the last time this timer pool was expired. If one >>>> + * or more periods have passed, attempt to expire it. >>>> + */ >>>> + prev_scan = tp->prev_scan; >>>> + now = odp_time_global(); >>>> + >>>> + nticks = (now.u64 - prev_scan.u64) / tp->time_per_tick.u64; >>>> + >>>> + if (nticks < 1) >>>> + continue; >>>> + >>>> + if (__atomic_compare_exchange_n( >>>> + &tp->prev_scan.u64, &prev_scan.u64, >>>> + prev_scan.u64 + (tp->time_per_tick.u64 * >>>> nticks), >>>> + false, __ATOMIC_RELAXED, __ATOMIC_RELAXED)) { >>>> + uint64_t tp_tick = _odp_atomic_u64_fetch_add_mm( >>>> + &tp->cur_tick, nticks, _ODP_MEMMODEL_RLX); >>>> + >>>> + if (tp->notify_overrun && nticks > 1) { >>>> + ODP_ERR("\n\t%d ticks overrun on timer >>>> pool " >>>> + "\"%s\", timer resolution too >>>> high\n", >>>> + nticks - 1, tp->name); >>>> + tp->notify_overrun = 0; >>>> + } >>>> + nexp += odp_timer_pool_expire(tp, tp_tick + >>>> nticks); >>>> + } >>>> + } >>>> + return nexp; >>>> +} >>>> + >>>> +static odp_time_t time_per_ratelimit_period; >>>> + >>>> +unsigned _timer_run(void) >>>> +{ >>>> + static __thread odp_time_t last_timer_run; >>>> + static __thread unsigned timer_run_cnt = >>>> + CONFIG_TIMER_RUN_RATELIMIT_ROUNDS; >>>> + odp_time_t now; >>>> + >>>> + /* Rate limit how often this thread checks the timer pools. */ >>>> + >>>> + if (CONFIG_TIMER_RUN_RATELIMIT_ROUNDS > 1) { >>>> + if (--timer_run_cnt) >>>> + return 0; >>>> + timer_run_cnt = CONFIG_TIMER_RUN_RATELIMIT_ROUNDS; >>>> + } >>>> + >>>> + now = odp_time_global(); >>>> + if (odp_time_cmp(odp_time_diff(now, last_timer_run), >>>> + time_per_ratelimit_period) == -1) >>>> + return 0; >>>> + last_timer_run = now; >>>> + >>>> + /* Check the timer pools. */ >>>> + return process_timer_pools(); >>>> +} >>>> + >>>> +/****************************************************************************** >>>> * POSIX timer support >>>> * Functions that use Linux/POSIX per-process timers and related >>>> facilities >>>> >>>> *****************************************************************************/ >>>> @@ -989,7 +1074,7 @@ void odp_timeout_free(odp_timeout_t tmo) >>>> odp_buffer_free(odp_buffer_from_event(ev)); >>>> } >>>> >>>> -int odp_timer_init_global(void) >>>> +int odp_timer_init_global(const odp_init_t *params) >>>> { >>>> #ifndef ODP_ATOMIC_U128 >>>> uint32_t i; >>>> @@ -1000,7 +1085,16 @@ int odp_timer_init_global(void) >>>> #endif >>>> odp_atomic_init_u32(&num_timer_pools, 0); >>>> >>>> - block_sigalarm(); >>>> + if (params) >>>> + inline_timers = >>>> + !params->not_used.feat.schedule && >>>> + !params->not_used.feat.timer; >>>> + >>>> + time_per_ratelimit_period = >>>> + odp_time_global_from_ns(CONFIG_TIMER_RUN_RATELIMIT_NS); >>>> + >>>> + if (!inline_timers) >>>> + block_sigalarm(); >>>> >>>> return 0; >>>> } >>>> -- >>>> 2.13.1 >>>>
