Comment 1 Andriy Gapon 2017-02-12 19:10:23 UTC

(In reply to hisamitu from comment #0)
For a start, could you please point out the exact place where, in your opinion, cs_load is used while uninitialized?
Thank you.

(In reply to Andriy Gapon from comment #1)

Those are in sys/kern/sched_ule.c.
Could you please refer to 2 lines with the comment "<--- here. ..." ?

--------

static __always_inline int
cpu_search(const struct cpu_group *cg, struct cpu_search *low,
    struct cpu_search *high, const int match)
{
    struct cpu_search lgroup;
    struct cpu_search hgroup;
    cpuset_t cpumask;
    struct cpu_group *child;
    struct tdq *tdq;
    int cpu, i, hload, lload, load, total, rnd;

    total = 0;
    cpumask = cg->cg_mask;
    if (match & CPU_SEARCH_LOWEST) {
        lload = INT_MAX;
        lgroup = *low;
    }
    if (match & CPU_SEARCH_HIGHEST) {
        hload = INT_MIN;
        hgroup = *high;
    }

    /* Iterate through the child CPU groups and then remaining CPUs. */
    for (i = cg->cg_children, cpu = mp_maxid; ; ) {
        if (i == 0) {
#ifdef HAVE_INLINE_FFSL
            cpu = CPU_FFS(&cpumask) - 1;
#else
            while (cpu >= 0 && !CPU_ISSET(cpu, &cpumask))
                cpu--;
#endif
            if (cpu < 0)
                break;
            child = NULL;
        } else
            child = &cg->cg_child[i - 1];

        if (match & CPU_SEARCH_LOWEST)
            lgroup.cs_cpu = -1;
        if (match & CPU_SEARCH_HIGHEST)
            hgroup.cs_cpu = -1;
        if (child) {            /* Handle child CPU group. */
            CPU_NAND(&cpumask, &child->cg_mask);
            switch (match) {
            case CPU_SEARCH_LOWEST:
                load = cpu_search_lowest(child, &lgroup);
                break;
            case CPU_SEARCH_HIGHEST:
                load = cpu_search_highest(child, &hgroup);
                break;
            case CPU_SEARCH_BOTH:
                load = cpu_search_both(child, &lgroup, &hgroup);
                break;
            }
        } else {            /* Handle child CPU. */
            CPU_CLR(cpu, &cpumask);
            tdq = TDQ_CPU(cpu); 
            load = tdq->tdq_load * 256;
            rnd = sched_random() % 32;
            if (match & CPU_SEARCH_LOWEST) {
                if (cpu == low->cs_prefer)
                    load -= 64;
                /* If that CPU is allowed and get data. */
                if (tdq->tdq_lowpri > lgroup.cs_pri &&
                    tdq->tdq_load <= lgroup.cs_limit &&
                    CPU_ISSET(cpu, &lgroup.cs_mask)) {
                    lgroup.cs_cpu = cpu; 
                    lgroup.cs_load = load - rnd;
                } 
            } 
            if (match & CPU_SEARCH_HIGHEST)
                if (tdq->tdq_load >= hgroup.cs_limit && 
                    tdq->tdq_transferable &&
                    CPU_ISSET(cpu, &hgroup.cs_mask)) {
                    hgroup.cs_cpu = cpu;
                    hgroup.cs_load = load - rnd;
                }
        }
        total += load;

        /* We have info about child item. Compare it. */
        if (match & CPU_SEARCH_LOWEST) {
            if (lgroup.cs_cpu >= 0 &&
                (load < lload ||
                 (load == lload && lgroup.cs_load < low->cs_load))) { <--- here. cs_load in "low" is not set value.
                lload = load;
                low->cs_cpu = lgroup.cs_cpu;
                low->cs_load = lgroup.cs_load;
            }
        }
        if (match & CPU_SEARCH_HIGHEST)
            if (hgroup.cs_cpu >= 0 &&
                (load > hload ||
                 (load == hload && hgroup.cs_load > high->cs_load))) { <--- here. cs_load in "high" is not set value.
                hload = load;
                high->cs_cpu = hgroup.cs_cpu;
                high->cs_load = hgroup.cs_load;
            }
        if (child) {
            i--;
            if (i == 0 && CPU_EMPTY(&cpumask))
                break;
        }
#ifndef HAVE_INLINE_FFSL
        else
            cpu--;
#endif
    }
    return (total);
}

/*
 * cpu_search instantiations must pass constants to maintain the inline
 * optimization.
 */
int
cpu_search_lowest(const struct cpu_group *cg, struct cpu_search *low)
{
    return cpu_search(cg, low, NULL, CPU_SEARCH_LOWEST);
}

int
cpu_search_highest(const struct cpu_group *cg, struct cpu_search *high)
{
    return cpu_search(cg, NULL, high, CPU_SEARCH_HIGHEST);
}

int
cpu_search_both(const struct cpu_group *cg, struct cpu_search *low,
    struct cpu_search *high)
{
    return cpu_search(cg, low, high, CPU_SEARCH_BOTH);
}

/*
 * Find the cpu with the least load via the least loaded path that has a
 * lowpri greater than pri  pri.  A pri of -1 indicates any priority is
 * acceptable.
 */
static inline int
sched_lowest(const struct cpu_group *cg, cpuset_t mask, int pri, int maxload,
    int prefer)
{
	struct cpu_search low;

	low.cs_cpu = -1;
	low.cs_prefer = prefer;
	low.cs_mask = mask;
	low.cs_pri = pri;
	low.cs_limit = maxload;
	cpu_search_lowest(cg, &low);  <--- cs_load in "low" is not set a value.
	return low.cs_cpu;
}

/*
 * Find the cpu with the highest load via the highest loaded path.
 */
static inline int
sched_highest(const struct cpu_group *cg, cpuset_t mask, int minload)
{
	struct cpu_search high;

	high.cs_cpu = -1;
	high.cs_mask = mask;
	high.cs_limit = minload;
	cpu_search_highest(cg, &high); <--- cs_load in "high" is not set a value.
	return high.cs_cpu;
}

--------

Both variable "low" and variable "high" are arguments of the function cpu_search. And their member variable cs_load do not have a valid value, because any invoker of function cpu_search does not set a valid value.
The function sched_lowest that sets an initial value to each member variable of struct cpu_search do not set a valid value to cs_load, too. Function sched_highest is also same for sched_lowest.

So, I think that a member variable cs_load of struct cpu_search are used in an uninitialized state when a value of auto variable "load" equals either an auto variable "lload" or an auto variable "hload".

Comment 3 Andriy Gapon 2017-02-13 21:52:38 UTC

(In reply to hisamitu from comment #2)
The code is not very readable, I admit that.  But I think that there is no problem.  Please note that in both cases cs_load is accessed after '||' operator.  lload and hload are initialized with such values that the left-hand side of the operator is going to be true when the condition is evaluated for the first time, so the right-hand side is not going to be evaluated.  After that cs_load gets assigned and the next time the condition is evaluated there won't be any uninitialized values.

(In reply to Andriy Gapon from comment #3)

I couldn't understand this function sufficiently.
I drop my patch.
Thank you very much for polite explanation.

# > The code is not very readable,
# I have thought that this code is very smart code when I have read the function comment.