statx: optimize copy of struct statx to userspace

From: Eric Biggers <[email protected]>

I found that statx() was significantly slower than stat(). As a microbenchmark, I compared 10,000,000 invocations of fstat() on a tmpfs file to the same with statx() passed a NULL path:

fs/stat.c +15 -0

+	struct statx tmp;
+
+	tmp.stx_mask = stat->result_mask;
+	tmp.stx_blksize = stat->blksize;
+	tmp.stx_attributes = stat->attributes;
+	tmp.stx_nlink = stat->nlink;
+	tmp.stx_uid = from_kuid_munged(current_user_ns(), stat->uid);
+	tmp.stx_gid = from_kgid_munged(current_user_ns(), stat->gid);
+	tmp.stx_mode = stat->mode;
+	memset(tmp.__spare0, 0, sizeof(tmp.__spare0));
+	tmp.stx_ino = stat->ino;
+	tmp.stx_size = stat->size;
+	tmp.stx_blocks = stat->blocks;
+	memset(tmp.__spare1, 0, sizeof(tmp.__spare1));
+	init_statx_timestamp(&tmp.stx_atime, &stat->atime);

Quoted 5 lines · Eric Biggers

From: Eric Biggers <[email protected]>

I found that statx() was significantly slower than stat(). As a microbenchmark, I compared 10,000,000 invocations of fstat() on a tmpfs file to the same with statx() passed a NULL path:

Umm...

fs/stat.c SYSCALL_DEFINE4(fstatat64, int, dfd, const char __user *, filename, · line 546

Quoted 5 lines

fs/stat.c +5 -0

+	tmp.stx_dev_major = MAJOR(stat->dev);
+	tmp.stx_dev_minor = MINOR(stat->dev);
+	memset(tmp.__spare2, 0, sizeof(tmp.__spare2));
+
+	return copy_to_user(buffer, &tmp, sizeof(tmp)) ? -EFAULT : 0;

That relies upon there being no padding in the damn structure. It's true and probably will be true on any target, but
a) it's bloody well worth stating explicitly
and

	b) struct statx tmp = {.stx_mask = stat->result_mask};

will get rid of those memset() you've got there by implicit zeroing of fields missing in partial structure initializer. Padding is *not* included into that, but you are relying upon having no padding anyway...

Hi Al,

Quoted 7 lines · Al Viro

Quoted 5 lines · Eric Biggers

From: Eric Biggers <[email protected]>

I found that statx() was significantly slower than stat(). As a microbenchmark, I compared 10,000,000 invocations of fstat() on a tmpfs file to the same with statx() passed a NULL path:

Quoted 2 lines

Umm...

Well, it's a silly benchmark, but stat performance is important, and usually things are cached already so most of the time is just overhead --- which this measures. And since nothing actually uses statx() yet, you can't do a benchmark just by running some command like 'git status' or whatever.

If padding is a concern at all (AFAICS it's not actually an issue now with struct statx, but people tend to have different opinions on how careful they want to be with padding), then I think we'll just have to start by memsetting the whole struct to 0.

Oh, I agree that multiple __put_user() are wrong; I also agree that bulk copy is the right approach (when we get the unsafe stuff right, we can revisit that, but I suspect that on quite a few architectures a bulk copy will still give better time, no matter what).

Quoted 4 lines

If padding is a concern at all (AFAICS it's not actually an issue now with struct statx, but people tend to have different opinions on how careful they want to be with padding), then I think we'll just have to start by memsetting the whole struct to 0.

My point is simply that it's worth a comment in that code.

Quoted 6 lines · Al Viro

Oh, I agree that multiple __put_user() are wrong; I also agree that bulk copy is the right approach (when we get the unsafe stuff right, we can revisit that, but I suspect that on quite a few architectures a bulk copy will still give better time, no matter what).

Quoted 6 lines

Quoted 4 lines

If padding is a concern at all (AFAICS it's not actually an issue now with struct statx, but people tend to have different opinions on how careful they want to be with padding), then I think we'll just have to start by memsetting the whole struct to 0.

My point is simply that it's worth a comment in that code.

Okay, thanks. I'll add a comment about the padding assumption, and I think I'll take the suggestion to use a designated initializer. Then at least all *fields* get initialized by default. And if in the future someone wants to conditionally initialize fields, then they can use ?: or they can do it after the initializer. Either way, at least they won't be able to forget to zero some field.

- Eric

Quoted 12 lines · Eric Biggers

Quoted 11 lines · Al Viro

Oh, I agree that multiple __put_user() are wrong; I also agree that bulk copy is the right approach (when we get the unsafe stuff right, we can revisit that, but I suspect that on quite a few architectures a bulk copy will still give better time, no matter what).

Quoted 4 lines

If padding is a concern at all (AFAICS it's not actually an issue now with struct statx, but people tend to have different opinions on how careful they want to be with padding), then I think we'll just have to start by memsetting the whole struct to 0.

Quoted 8 lines

Quoted 1 line

My point is simply that it's worth a comment in that code.

Okay, thanks. I'll add a comment about the padding assumption, and I think I'll take the suggestion to use a designated initializer. Then at least all *fields* get initialized by default. And if in the future someone wants to conditionally initialize fields, then they can use ?: or they can do it after the initializer. Either way, at least they won't be able to forget to zero some field.

Okay, well, I may have changed my mind again... I tried the designated initializer on x86_64 with gcc 4.8 and 6.3, and also on arm64 with gcc 4.8. In each case, it was compiled into first zeroing all 256 bytes of the struct, just like memset(&tmp, 0, sizeof(tmp)). Yes, this was with CC_OPTIMIZE_FOR_PERFORMANCE=y. So I think we might as well just write the full memset(), making it completely clear that everything is initialized. (This is especially useful for people who are auditing code paths like this for information leaks.) Also, a smart compiler could potentially optimize away parts of the memset() anyway...

Not that it is a huge deal either way, but I'd think it is harder for the compiler to optimize across a function call boundary like memset() vs. a struct initialization in the same function where it can see that all but a few of the fields are being overwritten immediately before they are used.

I don't think the designated initializer is any less clear to the reader that the struct is zeroed out compared to using memset(). Possibly the best compromise is to use a designated initializer that specifies all of the known fields, and leaves it to the compiler to initialize unset fields or padding.

Quoted 4 lines · Andreas Dilger

Not that it is a huge deal either way, but I'd think it is harder for the compiler to optimize across a function call boundary like memset() vs. a struct initialization in the same function where it can see that all but a few of the fields are being overwritten immediately before they are used.

GCC treats memset as a function call only if options such as -ffreestanding or -fno-builtin are enabled, or if memset is redefined in a header file. Does the kernel do this?