Re: Linux kernel, possible useless continue

Iwo Mergler wrote:
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?

No, I agree that they are equivalent.

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

I have a third version, how about this one:

for(...)
if(x)
continue;
if(!x)
for(...)
..

Obviously, this is equivalent but plain stupid. One check is completely
redundant. My point is that this very check is present as part of the for
loop and as requirement for the continue. So, the above is really rather
like this:

for(...)
if(x)
continue;
for(..; .. && !x; ..)
..

The only difference is the initialisation statement in the inner loop, and
for the case we're talking about this small difference doesn't make a
difference overall, in particular not when it is compiled with a
halfway-decent optimiser.

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().

I pretty much don't care what the code does for the application logic, and
to be honest, I don't even understand it. The only thing that strikes me
is that the same condition is checked twice, in a way that could be merged
into one.

Uli

.

Relevant Pages