From: Michal Koutný Date: Thu, 6 Jun 2019 21:27:20 +0000 (+0200) Subject: core: Extract job ordering logic X-Git-Tag: v243-rc1~152^2~3 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=e602f15282bcb413857e5ed06b0b02985cea1cbe;p=thirdparty%2Fsystemd.git core: Extract job ordering logic The job ordering logic is spread at multiple places of the code, making it hard to maintain and also a bit to understand. The actual execution order of two jobs always depends on their types and the ordering contraint between their units. Extract this logic to a new function job_compare. The second change is simplification of the order evaluation, JOB_STOP takes always precedence (as documented), unless two units are both stopping, then the ordering constraint is taken into account. --- diff --git a/src/core/job.c b/src/core/job.c index 81f5f9cb722..780f92c4d20 100644 --- a/src/core/job.c +++ b/src/core/job.c @@ -477,36 +477,14 @@ static bool job_is_runnable(Job *j) { if (j->type == JOB_NOP) return true; - if (IN_SET(j->type, JOB_START, JOB_VERIFY_ACTIVE, JOB_RELOAD)) { - /* Immediate result is that the job is or might be - * started. In this case let's wait for the - * dependencies, regardless whether they are - * starting or stopping something. */ - - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) - if (other->job) - return false; - } - - /* Also, if something else is being stopped and we should - * change state after it, then let's wait. */ + HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) + if (other->job && job_compare(j, other->job, UNIT_AFTER) > 0) + return false; HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE], i) - if (other->job && - IN_SET(other->job->type, JOB_STOP, JOB_RESTART)) + if (other->job && job_compare(j, other->job, UNIT_BEFORE) > 0) return false; - /* This means that for a service a and a service b where b - * shall be started after a: - * - * start a + start b → 1st step start a, 2nd step start b - * start a + stop b → 1st step stop b, 2nd step start a - * stop a + start b → 1st step stop a, 2nd step start b - * stop a + stop b → 1st step stop b, 2nd step stop a - * - * This has the side effect that restarts are properly - * synchronized too. */ - return true; } @@ -1455,46 +1433,14 @@ bool job_may_gc(Job *j) { if (j->type == JOB_NOP) return false; - /* If a job is ordered after ours, and is to be started, then it needs to wait for us, regardless if we stop or - * start, hence let's not GC in that case. */ - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE], i) { - if (!other->job) - continue; - - if (other->job->ignore_order) - continue; - - if (IN_SET(other->job->type, JOB_START, JOB_VERIFY_ACTIVE, JOB_RELOAD)) + /* The logic is inverse to job_is_runnable, we cannot GC as long as we block any job. */ + HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE], i) + if (other->job && job_compare(j, other->job, UNIT_BEFORE) < 0) return false; - } - - /* If we are going down, but something else is ordered After= us, then it needs to wait for us */ - if (IN_SET(j->type, JOB_STOP, JOB_RESTART)) - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) { - if (!other->job) - continue; - - if (other->job->ignore_order) - continue; + HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) + if (other->job && job_compare(j, other->job, UNIT_AFTER) < 0) return false; - } - - /* The logic above is kinda the inverse of the job_is_runnable() logic. Specifically, if the job "we" is - * ordered before the job "other": - * - * we start + other start → stay - * we start + other stop → gc - * we stop + other start → stay - * we stop + other stop → gc - * - * "we" are ordered after "other": - * - * we start + other start → gc - * we start + other stop → gc - * we stop + other start → stay - * we stop + other stop → stay - */ return true; } @@ -1512,7 +1458,7 @@ void job_add_to_gc_queue(Job *j) { j->in_gc_queue = true; } -static int job_compare(Job * const *a, Job * const *b) { +static int job_compare_id(Job * const *a, Job * const *b) { return CMP((*a)->id, (*b)->id); } @@ -1521,7 +1467,7 @@ static size_t sort_job_list(Job **list, size_t n) { size_t a, b; /* Order by numeric IDs */ - typesafe_qsort(list, n, job_compare); + typesafe_qsort(list, n, job_compare_id); /* Filter out duplicates */ for (a = 0, b = 0; a < n; a++) { @@ -1552,23 +1498,21 @@ int job_get_before(Job *j, Job*** ret) { return 0; } - if (IN_SET(j->type, JOB_START, JOB_VERIFY_ACTIVE, JOB_RELOAD)) { - - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) { - if (!other->job) - continue; + HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) { + if (!other->job) + continue; + if (job_compare(j, other->job, UNIT_AFTER) <= 0) + continue; - if (!GREEDY_REALLOC(list, n_allocated, n+1)) - return -ENOMEM; - list[n++] = other->job; - } + if (!GREEDY_REALLOC(list, n_allocated, n+1)) + return -ENOMEM; + list[n++] = other->job; } HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_BEFORE], i) { if (!other->job) continue; - - if (!IN_SET(other->job->type, JOB_STOP, JOB_RESTART)) + if (job_compare(j, other->job, UNIT_BEFORE) <= 0) continue; if (!GREEDY_REALLOC(list, n_allocated, n+1)) @@ -1602,7 +1546,7 @@ int job_get_after(Job *j, Job*** ret) { if (other->job->ignore_order) continue; - if (!IN_SET(other->job->type, JOB_START, JOB_VERIFY_ACTIVE, JOB_RELOAD)) + if (job_compare(j, other->job, UNIT_BEFORE) >= 0) continue; if (!GREEDY_REALLOC(list, n_allocated, n+1)) @@ -1610,19 +1554,20 @@ int job_get_after(Job *j, Job*** ret) { list[n++] = other->job; } - if (IN_SET(j->type, JOB_STOP, JOB_RESTART)) { - HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) { - if (!other->job) - continue; + HASHMAP_FOREACH_KEY(v, other, j->unit->dependencies[UNIT_AFTER], i) { + if (!other->job) + continue; + + if (other->job->ignore_order) + continue; - if (other->job->ignore_order) - continue; + if (job_compare(j, other->job, UNIT_AFTER) >= 0) + continue; - if (!GREEDY_REALLOC(list, n_allocated, n+1)) - return -ENOMEM; - list[n++] = other->job; - } + if (!GREEDY_REALLOC(list, n_allocated, n+1)) + return -ENOMEM; + list[n++] = other->job; } n = sort_job_list(list, n); @@ -1692,3 +1637,45 @@ const char* job_type_to_access_method(JobType t) { else return "reload"; } + +/* + * assume_dep assumed dependency between units (a is before/after b) + * + * Returns + * 0 jobs are independent, + * >0 a should run after b, + * <0 a should run before b, + * + * The logic means that for a service a and a service b where b.After=a: + * + * start a + start b → 1st step start a, 2nd step start b + * start a + stop b → 1st step stop b, 2nd step start a + * stop a + start b → 1st step stop a, 2nd step start b + * stop a + stop b → 1st step stop b, 2nd step stop a + * + * This has the side effect that restarts are properly + * synchronized too. + */ +int job_compare(Job *a, Job *b, UnitDependency assume_dep) { + assert(a->type < _JOB_TYPE_MAX_IN_TRANSACTION); + assert(b->type < _JOB_TYPE_MAX_IN_TRANSACTION); + assert(IN_SET(assume_dep, UNIT_AFTER, UNIT_BEFORE)); + + /* Trivial cases first */ + if (a->type == JOB_NOP || b->type == JOB_NOP) + return 0; + + if (a->ignore_order || b->ignore_order) + return 0; + + if (assume_dep == UNIT_AFTER) + return -job_compare(b, a, UNIT_BEFORE); + + /* Let's make it simple, JOB_STOP goes always first (in case both ua and ub stop, + * then ub's stop goes first anyway). + * JOB_RESTART is JOB_STOP in disguise (before it is patched to JOB_START). */ + if (IN_SET(b->type, JOB_STOP, JOB_RESTART)) + return 1; + else + return -1; +} diff --git a/src/core/job.h b/src/core/job.h index 0f15cbf821c..9c79c873d15 100644 --- a/src/core/job.h +++ b/src/core/job.h @@ -237,3 +237,5 @@ const char* job_result_to_string(JobResult t) _const_; JobResult job_result_from_string(const char *s) _pure_; const char* job_type_to_access_method(JobType t); + +int job_compare(Job *a, Job *b, UnitDependency assume_dep);