Re: Linux kernel, possible useless continue

Ulrich Eckhardt wrote:

Bin Chen wrote:
Below code I am curious in (A)'s continue will bring flow to (B) but
from (B) to (A) no code updates the value of both pfn and max_low_pfn.
So the condition of 'if' will always true. Is it useless?

Yes it is useless.

(B) for (; pgd_idx < PTRS_PER_PGD; pgd++, pgd_idx++) {
pmd = one_md_table_init(pgd);
if (pfn >= max_low_pfn)
(A) continue;
for (pmd_idx = 0;
pmd_idx < PTRS_PER_PMD && pfn < max_low_pfn;
pmd++, pmd_idx++) {
[...]
}
}

The if-comparison before (A) is the same as the second predicate for the
loop. The only thing this does additionally is set pmd_idx to zero, but if
that code relied on this side-effect of a loop variable it is broken
anyways and either needs to be rewritten or accurately documented.
However, it doesn't. Question is if the comparison in the loop is better
removed (i.e. if the comparison's outcome could change with an inner
iteration) or the outer.

Other than that, Iwo Mergler's comment that one_md_table_init() is called
with the next index indeed applies, and since max_low_pfn is not a local
variable to that function it could indeed be modified.


It's not quite what I meant.

The original code:

static void __init kernel_physical_mapping_init(pgd_t *pgd_base)
{
unsigned long pfn;
pgd_t *pgd;
pmd_t *pmd;
pte_t *pte;
int pgd_idx, pmd_idx, pte_ofs;

pgd_idx = pgd_index(PAGE_OFFSET);
pgd = pgd_base + pgd_idx;
pfn = 0;

for (; pgd_idx < PTRS_PER_PGD; pgd++, pgd_idx++) {
pmd = one_md_table_init(pgd);
if (pfn >= max_low_pfn)
continue;

/* Some code which modifies pfn */
}
}

is equivalent to this:

static void __init kernel_physical_mapping_init(pgd_t *pgd_base)
{
unsigned long pfn;
pgd_t *pgd;
pmd_t *pmd;
pte_t *pte;
int pgd_idx, pmd_idx, pte_ofs;

pgd_idx = pgd_index(PAGE_OFFSET);
pgd = pgd_base + pgd_idx;
pfn = 0;

for (; pgd_idx < PTRS_PER_PGD; pgd++, pgd_idx++) {
pmd = one_md_table_init(pgd);

if (pfn < max_low_pfn)
{
/* Some code which modifies pfn */
}
}
}

Would you object to the second version?

All it does is to save an additional level of indentation
and thus making the code more readable IMHO.

Rainer said it - the code populates all page entries (with
one_md_table_init()), but only maps the ones below max_low_pfn
into kernel space. As soon as pfn exceeds that limit, the loop
reverts to only calling one_md_table_init().

Kind regards,

Iwo

.

Relevant Pages