linux-kernel - Re: PATCH: __bprm_mm_init(): remove uneeded goto

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 4 Nov 2008 11:47:03 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	"Luiz Fernando N. Capitulino" <lcapitulino@...driva.com.br>
Cc:	linux-kernel@...r.kernel.org
Subject: Re: PATCH: __bprm_mm_init(): remove uneeded goto

On Tue, 4 Nov 2008 17:14:14 -0200
"Luiz Fernando N. Capitulino" <lcapitulino@...driva.com.br> wrote:

> Em Tue, 4 Nov 2008 10:57:07 -0800
> Andrew Morton <akpm@...ux-foundation.org> escreveu:
> 
> | The above code now uses the most common pattern for a kernel
> | function.  One we've learned from hard experience!
> 
>  Wow, I have no words to thank you enough for this full explanation!

How about "don't be so anal"?

I have more!

The code as we have it now looks like this:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err)
		goto err;
	...
	return 0;
err:
	up(sem);
	kfree(mem);
	return err;
}

it is legitimate (and arguably better) to do:

foo()
{
	if (!(mem = kmalloc(...))) {
		err = -ENOMEM;
		goto err;
	}

	down(sem);
	err = something();
	if (err)
		goto err_locked;
	...
	return 0;
err_locked:
	up(sem);
	kfree(mem);
err:
	return err;
}

so we now have a single `return' point and we've maximised
maintainability.  But that's a fairly minor detail, and we often leave
those initial `return's in place.

Secondly, there are instruction-cache concerns.

This code:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err) {
		up(sem);
		kfree(mem);
		goto err;
	}
	...
	return 0;
}

might cause the instructions for the `up' and the `kfree' to be laid
out in the middle of the function fastpath.  This will, on average,
cause the function to consume additional instruction cache lines.

Doing this:

foo()
{
	if (!(mem = kmalloc(...)))
		return -ENOMEM;

	down(sem);
	err = something();
	if (err)
		goto err;
	...
	return 0;
err:
	up(sem);
	kfree(mem);
	return err;
}

will, we hope, help the compiler to move the rarely-executed error-path
instructions out of line, thus maybe reducing the function's average
icache footprint.  The fastpath now spans a smaller address range.

We used to do this trick a *lot* in the kernel (back in the 2.2 days?)
for this performance reason.  Nowdays gcc is a lot more complex and we
hope that it can sometimes work these things out for itself and we hope
that `unlikely' might cause the compiler to move the unlikely code out
of line.  But I don't know how successful the compiler is at doing
this, and it'll be dependent upon the gcc version, the wind direction,
etc.

As long as it doesn't muck up the code readability, I expect that it's
still beneficial to provide this layout hint to the compiler.  A bit of
poking around in the .s files would be instructive..

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/