* [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned @ 2016-02-16 22:49 Rasmus Villemoes 2016-02-16 22:49 ` [PATCH 2/2] vfs: don't always include audit-specific members of struct filename Rasmus Villemoes 2016-02-16 22:57 ` [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Al Viro 0 siblings, 2 replies; 6+ messages in thread From: Rasmus Villemoes @ 2016-02-16 22:49 UTC (permalink / raw) To: Alexander Viro, Jeff Layton, J. Bruce Fields Cc: Rasmus Villemoes, Linus Torvalds, linux-fsdevel, linux-kernel I noticed that offsetof(struct filename, iname) is actually 28 on 64 bit platforms, so we always pass an unaligned pointer to strncpy_from_user. This is mostly a problem for those 64 bit platforms without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned accesses carry a penalty, especially when done in a loop. Let's try to ensure we always pass an aligned destination pointer to strncpy_from_user. I considered making refcnt a long instead of doing the union thing, and mostly ended up tossing a coin. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- Cc'ing Linus, not because it's urgent in any way, but because he's usually interested in strncpy_from_user and he can probably tell me why this is completely immaterial. fs/namei.c | 2 ++ include/linux/fs.h | 5 ++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/namei.c b/fs/namei.c index f624d132e01e..bd150fa799a2 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -35,6 +35,7 @@ #include <linux/fs_struct.h> #include <linux/posix_acl.h> #include <linux/hash.h> +#include <linux/bug.h> #include <asm/uaccess.h> #include "internal.h" @@ -127,6 +128,7 @@ getname_flags(const char __user *filename, int flags, int *empty) struct filename *result; char *kname; int len; + BUILD_BUG_ON(offsetof(struct filename, iname) % sizeof(long) != 0); result = audit_reusename(filename); if (result) diff --git a/include/linux/fs.h b/include/linux/fs.h index ae681002100a..d522e6391855 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2245,7 +2245,10 @@ struct filename { const char *name; /* pointer to actual string */ const __user char *uptr; /* original userland pointer */ struct audit_names *aname; - int refcnt; + union { + int refcnt; + long __padding; + }; const char iname[]; }; -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] vfs: don't always include audit-specific members of struct filename 2016-02-16 22:49 [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Rasmus Villemoes @ 2016-02-16 22:49 ` Rasmus Villemoes 2016-02-16 22:57 ` [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Al Viro 1 sibling, 0 replies; 6+ messages in thread From: Rasmus Villemoes @ 2016-02-16 22:49 UTC (permalink / raw) To: Alexander Viro, Paul Moore, Eric Paris, Jeff Layton, J. Bruce Fields Cc: Rasmus Villemoes, linux-fsdevel, linux-kernel, linux-audit The three members uptr, aname and refcnt are only used when CONFIG_AUDITSYSCALL, a fact which is not obvious from the header file or namei.c alone. So aside from eliminating a few useless instructions in getname_flags and making EMBEDDED_NAME_MAX a little larger, this patch also serves to document whoe the actual user of these members is. Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> --- fs/namei.c | 10 ++++------ include/linux/audit.h | 9 +++++++++ include/linux/fs.h | 2 ++ 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/fs/namei.c b/fs/namei.c index bd150fa799a2..21410db25814 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -185,7 +185,6 @@ getname_flags(const char __user *filename, int flags, int *empty) } } - result->refcnt = 1; /* The empty path is special. */ if (unlikely(!len)) { if (empty) @@ -196,8 +195,7 @@ getname_flags(const char __user *filename, int flags, int *empty) } } - result->uptr = filename; - result->aname = NULL; + audit_init_filename(result, filename); audit_getname(result); return result; } @@ -235,9 +233,7 @@ getname_kernel(const char * filename) return ERR_PTR(-ENAMETOOLONG); } memcpy((char *)result->name, filename, len); - result->uptr = NULL; - result->aname = NULL; - result->refcnt = 1; + audit_init_filename(result, NULL); audit_getname(result); return result; @@ -245,10 +241,12 @@ getname_kernel(const char * filename) void putname(struct filename *name) { +#ifdef CONFIG_AUDITSYSCALL BUG_ON(name->refcnt <= 0); if (--name->refcnt > 0) return; +#endif if (name->name != name->iname) { __putname(name->name); diff --git a/include/linux/audit.h b/include/linux/audit.h index b40ed5df5542..7d7143674d85 100644 --- a/include/linux/audit.h +++ b/include/linux/audit.h @@ -232,6 +232,12 @@ extern void __audit_syscall_entry(int major, unsigned long a0, unsigned long a1, extern void __audit_syscall_exit(int ret_success, long ret_value); extern struct filename *__audit_reusename(const __user char *uptr); extern void __audit_getname(struct filename *name); +static inline void audit_init_filename(struct filename *name, const __user char *uptr) +{ + name->refcnt = 1; + name->aname = NULL; + name->uptr = uptr; +} #define AUDIT_INODE_PARENT 1 /* dentry represents the parent */ #define AUDIT_INODE_HIDDEN 2 /* audit record should be hidden */ @@ -459,6 +465,9 @@ static inline struct filename *audit_reusename(const __user char *name) } static inline void audit_getname(struct filename *name) { } +static inline void audit_init_filename(struct filename *name, const __user char *uptr) +{ } + static inline void __audit_inode(struct filename *name, const struct dentry *dentry, unsigned int flags) diff --git a/include/linux/fs.h b/include/linux/fs.h index d522e6391855..df769f738695 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2243,12 +2243,14 @@ static inline int break_layout(struct inode *inode, bool wait) struct audit_names; struct filename { const char *name; /* pointer to actual string */ +#ifdef CONFIG_AUDITSYSCALL const __user char *uptr; /* original userland pointer */ struct audit_names *aname; union { int refcnt; long __padding; }; +#endif const char iname[]; }; -- 2.1.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned 2016-02-16 22:49 [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Rasmus Villemoes 2016-02-16 22:49 ` [PATCH 2/2] vfs: don't always include audit-specific members of struct filename Rasmus Villemoes @ 2016-02-16 22:57 ` Al Viro 2016-02-18 20:10 ` Rasmus Villemoes 1 sibling, 1 reply; 6+ messages in thread From: Al Viro @ 2016-02-16 22:57 UTC (permalink / raw) To: Rasmus Villemoes Cc: Jeff Layton, J. Bruce Fields, Linus Torvalds, linux-fsdevel, linux-kernel On Tue, Feb 16, 2016 at 11:49:24PM +0100, Rasmus Villemoes wrote: > I noticed that offsetof(struct filename, iname) is actually 28 on 64 > bit platforms, so we always pass an unaligned pointer to > strncpy_from_user. This is mostly a problem for those 64 bit platforms > without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned > accesses carry a penalty, especially when done in a loop. > > Let's try to ensure we always pass an aligned destination pointer to > strncpy_from_user. I considered making refcnt a long instead of doing > the union thing, and mostly ended up tossing a coin. Why not swap it with the previous field, then? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned 2016-02-16 22:57 ` [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Al Viro @ 2016-02-18 20:10 ` Rasmus Villemoes 2016-02-19 15:00 ` Theodore Ts'o 0 siblings, 1 reply; 6+ messages in thread From: Rasmus Villemoes @ 2016-02-18 20:10 UTC (permalink / raw) To: Al Viro Cc: Jeff Layton, J. Bruce Fields, Linus Torvalds, linux-fsdevel, linux-kernel On Tue, Feb 16 2016, Al Viro <viro@ZenIV.linux.org.uk> wrote: > On Tue, Feb 16, 2016 at 11:49:24PM +0100, Rasmus Villemoes wrote: >> I noticed that offsetof(struct filename, iname) is actually 28 on 64 >> bit platforms, so we always pass an unaligned pointer to >> strncpy_from_user. This is mostly a problem for those 64 bit platforms >> without HAVE_EFFICIENT_UNALIGNED_ACCESS, but even on x86_64, unaligned >> accesses carry a penalty, especially when done in a loop. >> >> Let's try to ensure we always pass an aligned destination pointer to >> strncpy_from_user. I considered making refcnt a long instead of doing >> the union thing, and mostly ended up tossing a coin. > > Why not swap it with the previous field, then? Sure, that would work as well. I don't really care how ->iname is pushed out to offset 32, but I'd like to know if it's worth it. Rasmus ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned 2016-02-18 20:10 ` Rasmus Villemoes @ 2016-02-19 15:00 ` Theodore Ts'o 2016-02-22 23:59 ` Rasmus Villemoes 0 siblings, 1 reply; 6+ messages in thread From: Theodore Ts'o @ 2016-02-19 15:00 UTC (permalink / raw) To: Rasmus Villemoes Cc: Al Viro, Jeff Layton, J. Bruce Fields, Linus Torvalds, linux-fsdevel, linux-kernel On Thu, Feb 18, 2016 at 09:10:21PM +0100, Rasmus Villemoes wrote: > > Sure, that would work as well. I don't really care how ->iname is pushed > out to offset 32, but I'd like to know if it's worth it. Do you have access to one of these platforms where unaligned access is really painful? The usual thing is to benchmark something like "git stat" which has to stat every single file in a repository's working directory. If you can't see it there, it seems unlikely you'd see it anywhere else, yes? - Ted ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned 2016-02-19 15:00 ` Theodore Ts'o @ 2016-02-22 23:59 ` Rasmus Villemoes 0 siblings, 0 replies; 6+ messages in thread From: Rasmus Villemoes @ 2016-02-22 23:59 UTC (permalink / raw) To: Theodore Ts'o Cc: Al Viro, Jeff Layton, J. Bruce Fields, Linus Torvalds, linux-fsdevel, linux-kernel On Fri, Feb 19 2016, Theodore Ts'o <tytso@mit.edu> wrote: > On Thu, Feb 18, 2016 at 09:10:21PM +0100, Rasmus Villemoes wrote: >> >> Sure, that would work as well. I don't really care how ->iname is pushed >> out to offset 32, but I'd like to know if it's worth it. > > Do you have access to one of these platforms where unaligned access is > really painful? No. But FWIW, I did a microbenchmark on my aging Core2, doing nothing but lstat() on the same "aaaa..." string in a loop. 'before' is 4.4.2 with a few unrelated patches, 'after' is that plus 1/2 and 2/2. In perf_x_y, x is length of "aaa..." string and y is alignment mod 8 in userspace. $ grep strncpy_from_user *.report perf_30_0_after.report: 5.47% s_f_u [k] strncpy_from_user perf_30_0_before.report: 7.40% s_f_u [k] strncpy_from_user perf_30_3_after.report: 5.05% s_f_u [k] strncpy_from_user perf_30_3_before.report: 7.29% s_f_u [k] strncpy_from_user perf_30_4_after.report: 4.88% s_f_u [k] strncpy_from_user perf_30_4_before.report: 7.28% s_f_u [k] strncpy_from_user perf_30_6_after.report: 5.43% s_f_u [k] strncpy_from_user perf_30_6_before.report: 6.74% s_f_u [k] strncpy_from_user perf_40_0_after.report: 5.68% s_f_u [k] strncpy_from_user perf_40_0_before.report: 10.99% s_f_u [k] strncpy_from_user perf_40_3_after.report: 5.37% s_f_u [k] strncpy_from_user perf_40_3_before.report: 10.62% s_f_u [k] strncpy_from_user perf_40_4_after.report: 5.61% s_f_u [k] strncpy_from_user perf_40_4_before.report: 10.91% s_f_u [k] strncpy_from_user perf_40_6_after.report: 5.81% s_f_u [k] strncpy_from_user perf_40_6_before.report: 10.84% s_f_u [k] strncpy_from_user perf_50_0_after.report: 6.29% s_f_u [k] strncpy_from_user perf_50_0_before.report: 12.46% s_f_u [k] strncpy_from_user perf_50_3_after.report: 7.15% s_f_u [k] strncpy_from_user perf_50_3_before.report: 14.09% s_f_u [k] strncpy_from_user perf_50_4_after.report: 7.64% s_f_u [k] strncpy_from_user perf_50_4_before.report: 14.10% s_f_u [k] strncpy_from_user perf_50_6_after.report: 7.30% s_f_u [k] strncpy_from_user perf_50_6_before.report: 14.10% s_f_u [k] strncpy_from_user perf_60_0_after.report: 6.81% s_f_u [k] strncpy_from_user perf_60_0_before.report: 13.25% s_f_u [k] strncpy_from_user perf_60_3_after.report: 9.48% s_f_u [k] strncpy_from_user perf_60_3_before.report: 13.26% s_f_u [k] strncpy_from_user perf_60_4_after.report: 9.90% s_f_u [k] strncpy_from_user perf_60_4_before.report: 15.09% s_f_u [k] strncpy_from_user perf_60_6_after.report: 9.91% s_f_u [k] strncpy_from_user perf_60_6_before.report: 13.85% s_f_u [k] strncpy_from_user So the numbers vary and it's a bit odd that some of the userspace-unaligned cases seem faster than the corresponding aligned ones, but overall I think it's ok to conclude there's a measurable difference. Note the huge jump from 30_y_before to 40_y_before. I suppose that's because we do an unaligned store crossing a cache line boundary when the string is > 32 bytes. I suppose 2/2 is also responsible for some of the above, since it not only aligns the kernel-side stores, but also means we stay within a single cacheline for strings up to 56 bytes. I should measure the effect of 1/2 by itself, but compiling a kernel takes forever for me, so I won't get to that tonight. [It turns out that 32 is the median length from 'git ls-files' in the kernel tree, with 33.2 being the mean, so even though I used relatively long paths above to get strncpy_from_user to stand out, such path lengths are not totally uncommon.] > The usual thing is to benchmark something like "git > stat" which has to stat every single file in a repository's working > directory. I tried that as well; strncpy_from_user was around 0.5% both before and after. Rasmus ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-02-22 23:59 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-02-16 22:49 [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Rasmus Villemoes 2016-02-16 22:49 ` [PATCH 2/2] vfs: don't always include audit-specific members of struct filename Rasmus Villemoes 2016-02-16 22:57 ` [PATCH 1/2] vfs: make sure struct filename->iname is word-aligned Al Viro 2016-02-18 20:10 ` Rasmus Villemoes 2016-02-19 15:00 ` Theodore Ts'o 2016-02-22 23:59 ` Rasmus Villemoes
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).