* [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-23 0:00 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).