diff mbox series

fs/open.c: micro-optimization by avoiding branch on common path

Message ID 20200919001021.21690-1-mateusznosek0@gmail.com
State New
Headers show
Series fs/open.c: micro-optimization by avoiding branch on common path | expand

Commit Message

Mateusz Nosek Sept. 19, 2020, 12:10 a.m. UTC
From: Mateusz Nosek <mateusznosek0@gmail.com>

If file is a directory it is surely not regular. Therefore, if 'S_ISREG'
check returns false one can be sure that vfs_truncate must returns with
error. Introduced patch refactors code to avoid one branch in 'likely'
control flow path. Moreover, it marks the proper check with 'unlikely'
macro to improve both branch prediction and readability. Changes were
tested with gcc 8.3.0 on x86 architecture and it is confirmed that
slightly better assembly is generated.

Signed-off-by: Mateusz Nosek <mateusznosek0@gmail.com>
---
 fs/open.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Al Viro Sept. 19, 2020, 12:20 a.m. UTC | #1
On Sat, Sep 19, 2020 at 02:10:21AM +0200, mateusznosek0@gmail.com wrote:
> From: Mateusz Nosek <mateusznosek0@gmail.com>
> 
> If file is a directory it is surely not regular. Therefore, if 'S_ISREG'
> check returns false one can be sure that vfs_truncate must returns with
> error. Introduced patch refactors code to avoid one branch in 'likely'
> control flow path. Moreover, it marks the proper check with 'unlikely'
> macro to improve both branch prediction and readability. Changes were
> tested with gcc 8.3.0 on x86 architecture and it is confirmed that
> slightly better assembly is generated.

Does it measurably show in any profiles?

> Signed-off-by: Mateusz Nosek <mateusznosek0@gmail.com>
> ---
>  fs/open.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9af548fb841b..69658ea27530 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -74,10 +74,12 @@ long vfs_truncate(const struct path *path, loff_t length)
>  	inode = path->dentry->d_inode;
>  
>  	/* For directories it's -EISDIR, for other non-regulars - -EINVAL */
> -	if (S_ISDIR(inode->i_mode))
> -		return -EISDIR;
> -	if (!S_ISREG(inode->i_mode))
> -		return -EINVAL;
> +	if (unlikely(!S_ISREG(inode->i_mode))) {
> +		if (S_ISDIR(inode->i_mode))
> +			return -EISDIR;
> +		else
> +			return -EINVAL;
> +	}

If anything, turn the second if into return S_ISDIR(...) ? -EISDIR : -EINVAL;
diff mbox series

Patch

diff --git a/fs/open.c b/fs/open.c
index 9af548fb841b..69658ea27530 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -74,10 +74,12 @@  long vfs_truncate(const struct path *path, loff_t length)
 	inode = path->dentry->d_inode;
 
 	/* For directories it's -EISDIR, for other non-regulars - -EINVAL */
-	if (S_ISDIR(inode->i_mode))
-		return -EISDIR;
-	if (!S_ISREG(inode->i_mode))
-		return -EINVAL;
+	if (unlikely(!S_ISREG(inode->i_mode))) {
+		if (S_ISDIR(inode->i_mode))
+			return -EISDIR;
+		else
+			return -EINVAL;
+	}
 
 	error = mnt_want_write(path->mnt);
 	if (error)