Skip to content

Commit

Permalink
Disable caching start-time for all clocktypes except GPR_CLOCK_MONOTONIC
Browse files Browse the repository at this point in the history
Caching the start-time for GPR_CLOCK_REALTIME has been causing errors in
cases where the system time is changed (after caching the time). In such
cases, the following functions produce incorrect results (and are off by
how much ever the system time was changed)
  grpc_millis_to_timespec() and grpc_timespec_to_millis_round_down()

This can cause problems especially when using the above functions to
get timer deadlines or completion queue timeouts.
(In the worst case scenarios, the timeouts/deadlines will always occur (if the
timeout inverval / deadline was less than the system change delta)

Ideally we should be reverting grpc#11866
but since that is a large change (which introduced new APIs in
exec_ctx.cc), I am doing this change to effectively revert to the old
behavior (while still keeping the new APIs introduced in exec_ctx)
  • Loading branch information
sreecha committed Nov 17, 2017
1 parent d9186a7 commit 9f4759a
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 77 deletions.
81 changes: 10 additions & 71 deletions src/core/lib/iomgr/exec_ctx.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,6 @@
#include "src/core/lib/iomgr/combiner.h"
#include "src/core/lib/profiling/timers.h"

#define GRPC_START_TIME_UPDATE_INTERVAL 10000
extern "C" grpc_tracer_flag grpc_timer_check_trace;

bool grpc_exec_ctx_ready_to_finish(grpc_exec_ctx* exec_ctx) {
if ((exec_ctx->flags & GRPC_EXEC_CTX_FLAG_IS_FINISHED) == 0) {
if (exec_ctx->check_ready_to_finish(exec_ctx,
Expand Down Expand Up @@ -107,49 +104,16 @@ static void exec_ctx_sched(grpc_exec_ctx* exec_ctx, grpc_closure* closure,
grpc_closure_list_append(&exec_ctx->closure_list, closure, error);
}

/* This time pair is not entirely thread-safe as store/load of tv_sec and
* tv_nsec are performed separately. However g_start_time do not need to have
* sub-second precision, so it is ok if the value of tv_nsec is off in this
* case. */
typedef struct time_atm_pair {
gpr_atm tv_sec;
gpr_atm tv_nsec;
} time_atm_pair;

static time_atm_pair
g_start_time[GPR_TIMESPAN + 1]; // assumes GPR_TIMESPAN is the
// last enum value in
// gpr_clock_type
static grpc_millis g_last_start_time_update;

static gpr_timespec timespec_from_time_atm_pair(const time_atm_pair* src,
gpr_clock_type clock_type) {
gpr_timespec time;
time.tv_nsec = (int32_t)gpr_atm_no_barrier_load(&src->tv_nsec);
time.tv_sec = (int64_t)gpr_atm_no_barrier_load(&src->tv_sec);
time.clock_type = clock_type;
return time;
}

static void time_atm_pair_store(time_atm_pair* dst, const gpr_timespec src) {
gpr_atm_no_barrier_store(&dst->tv_sec, src.tv_sec);
gpr_atm_no_barrier_store(&dst->tv_nsec, src.tv_nsec);
}
static gpr_timespec g_start_time;

void grpc_exec_ctx_global_init(void) {
for (int i = 0; i < GPR_TIMESPAN; i++) {
time_atm_pair_store(&g_start_time[i], gpr_now((gpr_clock_type)i));
}
// allows uniform treatment in conversion functions
time_atm_pair_store(&g_start_time[GPR_TIMESPAN], gpr_time_0(GPR_TIMESPAN));
g_start_time = gpr_now(GPR_CLOCK_MONOTONIC);
}

void grpc_exec_ctx_global_shutdown(void) {}

static gpr_atm timespec_to_atm_round_down(gpr_timespec ts) {
gpr_timespec start_time =
timespec_from_time_atm_pair(&g_start_time[ts.clock_type], ts.clock_type);
ts = gpr_time_sub(ts, start_time);
ts = gpr_time_sub(ts, g_start_time);
double x =
GPR_MS_PER_SEC * (double)ts.tv_sec + (double)ts.tv_nsec / GPR_NS_PER_MS;
if (x < 0) return 0;
Expand All @@ -158,9 +122,7 @@ static gpr_atm timespec_to_atm_round_down(gpr_timespec ts) {
}

static gpr_atm timespec_to_atm_round_up(gpr_timespec ts) {
gpr_timespec start_time =
timespec_from_time_atm_pair(&g_start_time[ts.clock_type], ts.clock_type);
ts = gpr_time_sub(ts, start_time);
ts = gpr_time_sub(ts, g_start_time);
double x = GPR_MS_PER_SEC * (double)ts.tv_sec +
(double)ts.tv_nsec / GPR_NS_PER_MS +
(double)(GPR_NS_PER_SEC - 1) / (double)GPR_NS_PER_SEC;
Expand Down Expand Up @@ -195,41 +157,18 @@ gpr_timespec grpc_millis_to_timespec(grpc_millis millis,
if (clock_type == GPR_TIMESPAN) {
return gpr_time_from_millis(millis, GPR_TIMESPAN);
}
gpr_timespec start_time =
timespec_from_time_atm_pair(&g_start_time[clock_type], clock_type);
return gpr_time_add(start_time, gpr_time_from_millis(millis, GPR_TIMESPAN));
return gpr_time_add(gpr_convert_clock_type(g_start_time, clock_type),
gpr_time_from_millis(millis, GPR_TIMESPAN));
}

grpc_millis grpc_timespec_to_millis_round_down(gpr_timespec ts) {
return timespec_to_atm_round_down(ts);
return timespec_to_atm_round_down(
gpr_convert_clock_type(ts, g_start_time.clock_type));
}

grpc_millis grpc_timespec_to_millis_round_up(gpr_timespec ts) {
return timespec_to_atm_round_up(ts);
}

void grpc_exec_ctx_maybe_update_start_time(grpc_exec_ctx* exec_ctx) {
grpc_millis now = grpc_exec_ctx_now(exec_ctx);
grpc_millis last_start_time_update =
gpr_atm_no_barrier_load(&g_last_start_time_update);

if (now > last_start_time_update &&
now - last_start_time_update > GRPC_START_TIME_UPDATE_INTERVAL) {
/* Get the current system time and subtract \a now from it, where \a now is
* the relative time from grpc_init() from monotonic clock. This calibrates
* the time when grpc_exec_ctx_global_init was called based on current
* system clock. */
gpr_atm_no_barrier_store(&g_last_start_time_update, now);
gpr_timespec real_now = gpr_now(GPR_CLOCK_REALTIME);
gpr_timespec real_start_time =
gpr_time_sub(real_now, gpr_time_from_millis(now, GPR_TIMESPAN));
time_atm_pair_store(&g_start_time[GPR_CLOCK_REALTIME], real_start_time);

if (GRPC_TRACER_ON(grpc_timer_check_trace)) {
gpr_log(GPR_DEBUG, "Update realtime clock start time: %" PRId64 "s %dns",
real_start_time.tv_sec, real_start_time.tv_nsec);
}
}
return timespec_to_atm_round_up(
gpr_convert_clock_type(ts, g_start_time.clock_type));
}

static const grpc_closure_scheduler_vtable exec_ctx_scheduler_vtable = {
Expand Down
2 changes: 0 additions & 2 deletions src/core/lib/iomgr/exec_ctx.h
Original file line number Diff line number Diff line change
Expand Up @@ -124,8 +124,6 @@ gpr_timespec grpc_millis_to_timespec(grpc_millis millis, gpr_clock_type clock);
grpc_millis grpc_timespec_to_millis_round_down(gpr_timespec timespec);
grpc_millis grpc_timespec_to_millis_round_up(gpr_timespec timespec);

void grpc_exec_ctx_maybe_update_start_time(grpc_exec_ctx* exec_ctx);

#ifdef __cplusplus
}
#endif
Expand Down
4 changes: 0 additions & 4 deletions src/core/lib/iomgr/timer_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,6 @@ static void timer_main_loop(grpc_exec_ctx* exec_ctx) {
grpc_millis next = GRPC_MILLIS_INF_FUTURE;
grpc_exec_ctx_invalidate_now(exec_ctx);

/* Calibrate g_start_time in exec_ctx.cc with a regular interval in case the
* system clock has changed */
grpc_exec_ctx_maybe_update_start_time(exec_ctx);

// check timer state, updates next to the next time to run a check
switch (grpc_timer_check(exec_ctx, &next)) {
case GRPC_TIMERS_FIRED:
Expand Down

0 comments on commit 9f4759a

Please sign in to comment.