The newly used helper also checks for 0-sized buffers.
This avoids path lookup code, lockref management, memory allocation and
in case of NULL path userspace memory access (which can be quite
expensive with SMAP on x86_64).
statx with AT_EMPTY_PATH paired with "" or NULL argument as appropriate
issued on Sapphire Rapids (ops/s):
stock: 4231237
0-check: 5944063 (+40%)
NULL path: 6601619 (+11%/+56%)
Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
fs/internal.h | 2 ++
fs/stat.c | 90 ++++++++++++++++++++++++++++++++++--------------
io_uring/statx.c | 23 +++++++------
3 files changed, 80 insertions(+), 35 deletions(-)
diff --git a/fs/internal.h b/fs/internal.h
index 1caa6a8f666f..0a018ebcaf49 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -244,6 +244,8 @@ extern const struct dentry_operations ns_dentry_operations;
int getname_statx_lookup_flags(int flags);
int do_statx(int dfd, struct filename *filename, unsigned int flags,
unsigned int mask, struct statx __user *buffer);
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+ struct statx __user *buffer);
/*
* fs/splice.c:
diff --git a/fs/stat.c b/fs/stat.c
index 106684034fdb..1214826f3a36 100644
--- a/fs/stat.c
+++ b/fs/stat.c
@@ -214,6 +214,43 @@ int getname_statx_lookup_flags(int flags)
return lookup_flags;
}
+static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
+ u32 request_mask)
+{
+ int error = vfs_getattr(path, stat, request_mask, flags);
+
+ if (request_mask & STATX_MNT_ID_UNIQUE) {
+ stat->mnt_id = real_mount(path->mnt)->mnt_id_unique;
+ stat->result_mask |= STATX_MNT_ID_UNIQUE;
+ } else {
+ stat->mnt_id = real_mount(path->mnt)->mnt_id;
+ stat->result_mask |= STATX_MNT_ID;
+ }
+
+ if (path->mnt->mnt_root == path->dentry)
+ stat->attributes |= STATX_ATTR_MOUNT_ROOT;
+ stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
+
+ /* Handle STATX_DIOALIGN for block devices. */
+ if (request_mask & STATX_DIOALIGN) {
+ struct inode *inode = d_backing_inode(path->dentry);
+
+ if (S_ISBLK(inode->i_mode))
+ bdev_statx_dioalign(inode, stat);
+ }
+
+ return error;
+}
+
+static int vfs_statx_fd(int fd, int flags, struct kstat *stat,
+ u32 request_mask)
+{
+ CLASS(fd_raw, f)(fd);
+ if (!f.file)
+ return -EBADF;
+ return vfs_statx_path(&f.file->f_path, flags, stat, request_mask);
+}
+
/**
* vfs_statx - Get basic and extra attributes by filename
* @dfd: A file descriptor representing the base dir for a relative filename
@@ -243,36 +280,13 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
retry:
error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
if (error)
- goto out;
-
- error = vfs_getattr(&path, stat, request_mask, flags);
-
- if (request_mask & STATX_MNT_ID_UNIQUE) {
- stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
- stat->result_mask |= STATX_MNT_ID_UNIQUE;
- } else {
- stat->mnt_id = real_mount(path.mnt)->mnt_id;
- stat->result_mask |= STATX_MNT_ID;
- }
-
- if (path.mnt->mnt_root == path.dentry)
- stat->attributes |= STATX_ATTR_MOUNT_ROOT;
- stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
-
- /* Handle STATX_DIOALIGN for block devices. */
- if (request_mask & STATX_DIOALIGN) {
- struct inode *inode = d_backing_inode(path.dentry);
-
- if (S_ISBLK(inode->i_mode))
- bdev_statx_dioalign(inode, stat);
- }
-
+ return error;
+ error = vfs_statx_path(&path, flags, stat, request_mask);
path_put(&path);
if (retry_estale(error, lookup_flags)) {
lookup_flags |= LOOKUP_REVAL;
goto retry;
}
-out:
return error;
}
@@ -677,6 +691,29 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,
return cp_statx(&stat, buffer);
}
+int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
+ struct statx __user *buffer)
+{
+ struct kstat stat;
+ int error;
+
+ if (mask & STATX__RESERVED)
+ return -EINVAL;
+ if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
+ return -EINVAL;
+
+ /* STATX_CHANGE_COOKIE is kernel-only for now. Ignore requests
+ * from userland.
+ */
+ mask &= ~STATX_CHANGE_COOKIE;
+
+ error = vfs_statx_fd(fd, flags, &stat, mask);
+ if (error)
+ return error;
+
+ return cp_statx(&stat, buffer);
+}
+
/**
* sys_statx - System call to get enhanced stats
* @dfd: Base directory to pathwalk from *or* fd to stat.
@@ -696,6 +733,9 @@ SYSCALL_DEFINE5(statx,
int ret;
struct filename *name;
+ if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
+ return do_statx_fd(dfd, flags, mask, buffer);
+
name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
ret = do_statx(dfd, name, flags, mask, buffer);
putname(name);
diff --git a/io_uring/statx.c b/io_uring/statx.c
index abb874209caa..fe967ecb1762 100644
--- a/io_uring/statx.c
+++ b/io_uring/statx.c
@@ -23,6 +23,7 @@ struct io_statx {
int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
{
struct io_statx *sx = io_kiocb_to_cmd(req, struct io_statx);
+ struct filename *filename;
const char __user *path;
if (sqe->buf_index || sqe->splice_fd_in)
@@ -36,15 +37,14 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
sx->buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
sx->flags = READ_ONCE(sqe->statx_flags);
- sx->filename = getname_flags(path,
- getname_statx_lookup_flags(sx->flags),
- NULL);
-
- if (IS_ERR(sx->filename)) {
- int ret = PTR_ERR(sx->filename);
-
- sx->filename = NULL;
- return ret;
+ sx->filename = NULL;
+ if (!(sx->flags == AT_EMPTY_PATH && vfs_empty_path(sx->dfd, path))) {
+ filename = getname_flags(path,
+ getname_statx_lookup_flags(sx->flags),
+ NULL);
+ if (IS_ERR(filename))
+ return PTR_ERR(filename);
+ sx->filename = filename;
}
req->flags |= REQ_F_NEED_CLEANUP;
@@ -59,7 +59,10 @@ int io_statx(struct io_kiocb *req, unsigned int issue_flags)
WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
- ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
+ if (sx->filename == NULL)
+ ret = do_statx_fd(sx->dfd, sx->flags, sx->mask, sx->buffer);
+ else
+ ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
io_req_set_res(req, ret, 0);
return IOU_OK;
}
--
2.43.0On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> The newly used helper also checks for 0-sized buffers.
>
> This avoids path lookup code, lockref management, memory allocation and
> in case of NULL path userspace memory access (which can be quite
> expensive with SMAP on x86_64).
>
> statx with AT_EMPTY_PATH paired with "" or NULL argument as appropriate
> issued on Sapphire Rapids (ops/s):
> stock: 4231237
> 0-check: 5944063 (+40%)
> NULL path: 6601619 (+11%/+56%)
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
Hi, Ruoyao,
I'm a bit confused. Ii this patch a replacement of your recent patch?
Huacai
> ---
> fs/internal.h | 2 ++
> fs/stat.c | 90 ++++++++++++++++++++++++++++++++++--------------
> io_uring/statx.c | 23 +++++++------
> 3 files changed, 80 insertions(+), 35 deletions(-)
>
> diff --git a/fs/internal.h b/fs/internal.h
> index 1caa6a8f666f..0a018ebcaf49 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -244,6 +244,8 @@ extern const struct dentry_operations ns_dentry_operations;
> int getname_statx_lookup_flags(int flags);
> int do_statx(int dfd, struct filename *filename, unsigned int flags,
> unsigned int mask, struct statx __user *buffer);
> +int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
> + struct statx __user *buffer);
>
> /*
> * fs/splice.c:
> diff --git a/fs/stat.c b/fs/stat.c
> index 106684034fdb..1214826f3a36 100644
> --- a/fs/stat.c
> +++ b/fs/stat.c
> @@ -214,6 +214,43 @@ int getname_statx_lookup_flags(int flags)
> return lookup_flags;
> }
>
> +static int vfs_statx_path(struct path *path, int flags, struct kstat *stat,
> + u32 request_mask)
> +{
> + int error = vfs_getattr(path, stat, request_mask, flags);
> +
> + if (request_mask & STATX_MNT_ID_UNIQUE) {
> + stat->mnt_id = real_mount(path->mnt)->mnt_id_unique;
> + stat->result_mask |= STATX_MNT_ID_UNIQUE;
> + } else {
> + stat->mnt_id = real_mount(path->mnt)->mnt_id;
> + stat->result_mask |= STATX_MNT_ID;
> + }
> +
> + if (path->mnt->mnt_root == path->dentry)
> + stat->attributes |= STATX_ATTR_MOUNT_ROOT;
> + stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
> +
> + /* Handle STATX_DIOALIGN for block devices. */
> + if (request_mask & STATX_DIOALIGN) {
> + struct inode *inode = d_backing_inode(path->dentry);
> +
> + if (S_ISBLK(inode->i_mode))
> + bdev_statx_dioalign(inode, stat);
> + }
> +
> + return error;
> +}
> +
> +static int vfs_statx_fd(int fd, int flags, struct kstat *stat,
> + u32 request_mask)
> +{
> + CLASS(fd_raw, f)(fd);
> + if (!f.file)
> + return -EBADF;
> + return vfs_statx_path(&f.file->f_path, flags, stat, request_mask);
> +}
> +
> /**
> * vfs_statx - Get basic and extra attributes by filename
> * @dfd: A file descriptor representing the base dir for a relative filename
> @@ -243,36 +280,13 @@ static int vfs_statx(int dfd, struct filename *filename, int flags,
> retry:
> error = filename_lookup(dfd, filename, lookup_flags, &path, NULL);
> if (error)
> - goto out;
> -
> - error = vfs_getattr(&path, stat, request_mask, flags);
> -
> - if (request_mask & STATX_MNT_ID_UNIQUE) {
> - stat->mnt_id = real_mount(path.mnt)->mnt_id_unique;
> - stat->result_mask |= STATX_MNT_ID_UNIQUE;
> - } else {
> - stat->mnt_id = real_mount(path.mnt)->mnt_id;
> - stat->result_mask |= STATX_MNT_ID;
> - }
> -
> - if (path.mnt->mnt_root == path.dentry)
> - stat->attributes |= STATX_ATTR_MOUNT_ROOT;
> - stat->attributes_mask |= STATX_ATTR_MOUNT_ROOT;
> -
> - /* Handle STATX_DIOALIGN for block devices. */
> - if (request_mask & STATX_DIOALIGN) {
> - struct inode *inode = d_backing_inode(path.dentry);
> -
> - if (S_ISBLK(inode->i_mode))
> - bdev_statx_dioalign(inode, stat);
> - }
> -
> + return error;
> + error = vfs_statx_path(&path, flags, stat, request_mask);
> path_put(&path);
> if (retry_estale(error, lookup_flags)) {
> lookup_flags |= LOOKUP_REVAL;
> goto retry;
> }
> -out:
> return error;
> }
>
> @@ -677,6 +691,29 @@ int do_statx(int dfd, struct filename *filename, unsigned int flags,
> return cp_statx(&stat, buffer);
> }
>
> +int do_statx_fd(int fd, unsigned int flags, unsigned int mask,
> + struct statx __user *buffer)
> +{
> + struct kstat stat;
> + int error;
> +
> + if (mask & STATX__RESERVED)
> + return -EINVAL;
> + if ((flags & AT_STATX_SYNC_TYPE) == AT_STATX_SYNC_TYPE)
> + return -EINVAL;
> +
> + /* STATX_CHANGE_COOKIE is kernel-only for now. Ignore requests
> + * from userland.
> + */
> + mask &= ~STATX_CHANGE_COOKIE;
> +
> + error = vfs_statx_fd(fd, flags, &stat, mask);
> + if (error)
> + return error;
> +
> + return cp_statx(&stat, buffer);
> +}
> +
> /**
> * sys_statx - System call to get enhanced stats
> * @dfd: Base directory to pathwalk from *or* fd to stat.
> @@ -696,6 +733,9 @@ SYSCALL_DEFINE5(statx,
> int ret;
> struct filename *name;
>
> + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
> + return do_statx_fd(dfd, flags, mask, buffer);
> +
> name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
> ret = do_statx(dfd, name, flags, mask, buffer);
> putname(name);
> diff --git a/io_uring/statx.c b/io_uring/statx.c
> index abb874209caa..fe967ecb1762 100644
> --- a/io_uring/statx.c
> +++ b/io_uring/statx.c
> @@ -23,6 +23,7 @@ struct io_statx {
> int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> {
> struct io_statx *sx = io_kiocb_to_cmd(req, struct io_statx);
> + struct filename *filename;
> const char __user *path;
>
> if (sqe->buf_index || sqe->splice_fd_in)
> @@ -36,15 +37,14 @@ int io_statx_prep(struct io_kiocb *req, const struct io_uring_sqe *sqe)
> sx->buffer = u64_to_user_ptr(READ_ONCE(sqe->addr2));
> sx->flags = READ_ONCE(sqe->statx_flags);
>
> - sx->filename = getname_flags(path,
> - getname_statx_lookup_flags(sx->flags),
> - NULL);
> -
> - if (IS_ERR(sx->filename)) {
> - int ret = PTR_ERR(sx->filename);
> -
> - sx->filename = NULL;
> - return ret;
> + sx->filename = NULL;
> + if (!(sx->flags == AT_EMPTY_PATH && vfs_empty_path(sx->dfd, path))) {
> + filename = getname_flags(path,
> + getname_statx_lookup_flags(sx->flags),
> + NULL);
> + if (IS_ERR(filename))
> + return PTR_ERR(filename);
> + sx->filename = filename;
> }
>
> req->flags |= REQ_F_NEED_CLEANUP;
> @@ -59,7 +59,10 @@ int io_statx(struct io_kiocb *req, unsigned int issue_flags)
>
> WARN_ON_ONCE(issue_flags & IO_URING_F_NONBLOCK);
>
> - ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
> + if (sx->filename == NULL)
> + ret = do_statx_fd(sx->dfd, sx->flags, sx->mask, sx->buffer);
> + else
> + ret = do_statx(sx->dfd, sx->filename, sx->flags, sx->mask, sx->buffer);
> io_req_set_res(req, ret, 0);
> return IOU_OK;
> }
> --
> 2.43.0
>
>
On Tue, 2024-06-25 at 22:09 +0800, Huacai Chen wrote: > On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> > wrote: > > > > The newly used helper also checks for 0-sized buffers. > > > > This avoids path lookup code, lockref management, memory allocation > > and > > in case of NULL path userspace memory access (which can be quite > > expensive with SMAP on x86_64). > > > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > > appropriate > > issued on Sapphire Rapids (ops/s): > > stock: 4231237 > > 0-check: 5944063 (+40%) > > NULL path: 6601619 (+11%/+56%) > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > Hi, Ruoyao, > > I'm a bit confused. Ii this patch a replacement of your recent patch? Yes, both Linus and Christian hates introducing a new AT_ flag for this. This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave like statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the performance issue and it's also audit-able by seccomp BPF. -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University
On Tue, Jun 25, 2024 at 11:00 PM Xi Ruoyao <xry111@xry111.site> wrote: > > On Tue, 2024-06-25 at 22:09 +0800, Huacai Chen wrote: > > On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> > > wrote: > > > > > > The newly used helper also checks for 0-sized buffers. > > > > > > This avoids path lookup code, lockref management, memory allocation > > > and > > > in case of NULL path userspace memory access (which can be quite > > > expensive with SMAP on x86_64). > > > > > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > > > appropriate > > > issued on Sapphire Rapids (ops/s): > > > stock: 4231237 > > > 0-check: 5944063 (+40%) > > > NULL path: 6601619 (+11%/+56%) > > > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > Hi, Ruoyao, > > > > I'm a bit confused. Ii this patch a replacement of your recent patch? > > Yes, both Linus and Christian hates introducing a new AT_ flag for this. > > This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave like > statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the performance > issue and it's also audit-able by seccomp BPF. To be honest, I still want to restore __ARCH_WANT_NEW_STAT. Because even if statx() becomes audit-able, it is still blacklisted now. Restoring __ARCH_WANT_NEW_STAT is a very small change that doesn't introduce any complexity, but it makes life easier. And I think libLoL also likes __ARCH_WANT_NEW_STAT, though it isn't an upstream project... Huacai > > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University >
On Sun, 2024-06-30 at 09:40 +0800, Huacai Chen wrote: > On Tue, Jun 25, 2024 at 11:00 PM Xi Ruoyao <xry111@xry111.site> wrote: > > > > On Tue, 2024-06-25 at 22:09 +0800, Huacai Chen wrote: > > > On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> > > > wrote: > > > > > > > > The newly used helper also checks for 0-sized buffers. > > > > > > > > This avoids path lookup code, lockref management, memory > > > > allocation > > > > and > > > > in case of NULL path userspace memory access (which can be quite > > > > expensive with SMAP on x86_64). > > > > > > > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > > > > appropriate > > > > issued on Sapphire Rapids (ops/s): > > > > stock: 4231237 > > > > 0-check: 5944063 (+40%) > > > > NULL path: 6601619 (+11%/+56%) > > > > > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > Hi, Ruoyao, > > > > > > I'm a bit confused. Ii this patch a replacement of your recent > > > patch? > > > > Yes, both Linus and Christian hates introducing a new AT_ flag for > > this. > > > > This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave > > like > > statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the > > performance > > issue and it's also audit-able by seccomp BPF. > To be honest, I still want to restore __ARCH_WANT_NEW_STAT. Because > even if statx() becomes audit-able, it is still blacklisted now. Then patch the sandbox to allow it. The sandbox **must** be patched anyway or it'll be broken on all 32-bit systems after 2037. [Unless they'll unsupport all 32-bit systems before 2037.] > Restoring __ARCH_WANT_NEW_STAT is a very small change that doesn't > introduce any complexity, but it makes life easier. And I think libLoL > also likes __ARCH_WANT_NEW_STAT, though it isn't an upstream > project... At least you should not restore it for 32-bit. libLoL also has nothing to do with 32-bit systems anyway. Maybe conditional it with a #if checking __BITS_PER_LONG. And the vendors should really port their software to the upstreamed ABI instead of relying on liblol. <rant>Is a recompiling so difficult, or are the programmers so stupid to invoke plenty of low-level syscalls directly (bypassing Glibc) in their code?</rant> -- Xi Ruoyao <xry111@xry111.site> School of Aerospace Science and Technology, Xidian University
On Sun, Jun 30, 2024, at 04:39, Xi Ruoyao wrote:
> On Sun, 2024-06-30 at 09:40 +0800, Huacai Chen wrote:
>> >
>> > Yes, both Linus and Christian hates introducing a new AT_ flag for
>> > this.
>> >
>> > This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave
>> > like
>> > statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the
>> > performance
>> > issue and it's also audit-able by seccomp BPF.
>> To be honest, I still want to restore __ARCH_WANT_NEW_STAT. Because
>> even if statx() becomes audit-able, it is still blacklisted now.
>
> Then patch the sandbox to allow it.
>
> The sandbox **must** be patched anyway or it'll be broken on all 32-bit
> systems after 2037. [Unless they'll unsupport all 32-bit systems before
> 2037.]
More importantly, the sandbox won't be able to support any 32-bit
targets that support running after 2037, regardless of how long
the sandbox supports them: if you turn off COMPAT_32BIT_TIME today
in order to be sure those don't get called by accident, the
fallback is immediately broken.
Arnd
Hi, Arnd, On Mon, Jul 1, 2024 at 7:59 PM Arnd Bergmann <arnd@arndb.de> wrote: > > On Sun, Jun 30, 2024, at 04:39, Xi Ruoyao wrote: > > On Sun, 2024-06-30 at 09:40 +0800, Huacai Chen wrote: > >> > > >> > Yes, both Linus and Christian hates introducing a new AT_ flag for > >> > this. > >> > > >> > This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave > >> > like > >> > statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the > >> > performance > >> > issue and it's also audit-able by seccomp BPF. > >> To be honest, I still want to restore __ARCH_WANT_NEW_STAT. Because > >> even if statx() becomes audit-able, it is still blacklisted now. > > > > Then patch the sandbox to allow it. > > > > The sandbox **must** be patched anyway or it'll be broken on all 32-bit > > systems after 2037. [Unless they'll unsupport all 32-bit systems before > > 2037.] > > More importantly, the sandbox won't be able to support any 32-bit > targets that support running after 2037, regardless of how long > the sandbox supports them: if you turn off COMPAT_32BIT_TIME today > in order to be sure those don't get called by accident, the > fallback is immediately broken. Would you mind if I restore newstat for LoongArch64 even if this patch exist? Huacai > > Arnd
On Sun, Jun 30, 2024 at 10:40 AM Xi Ruoyao <xry111@xry111.site> wrote: > > On Sun, 2024-06-30 at 09:40 +0800, Huacai Chen wrote: > > On Tue, Jun 25, 2024 at 11:00 PM Xi Ruoyao <xry111@xry111.site> wrote: > > > > > > On Tue, 2024-06-25 at 22:09 +0800, Huacai Chen wrote: > > > > On Tue, Jun 25, 2024 at 7:01 PM Mateusz Guzik <mjguzik@gmail.com> > > > > wrote: > > > > > > > > > > The newly used helper also checks for 0-sized buffers. > > > > > > > > > > This avoids path lookup code, lockref management, memory > > > > > allocation > > > > > and > > > > > in case of NULL path userspace memory access (which can be quite > > > > > expensive with SMAP on x86_64). > > > > > > > > > > statx with AT_EMPTY_PATH paired with "" or NULL argument as > > > > > appropriate > > > > > issued on Sapphire Rapids (ops/s): > > > > > stock: 4231237 > > > > > 0-check: 5944063 (+40%) > > > > > NULL path: 6601619 (+11%/+56%) > > > > > > > > > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com> > > > > Hi, Ruoyao, > > > > > > > > I'm a bit confused. Ii this patch a replacement of your recent > > > > patch? > > > > > > Yes, both Linus and Christian hates introducing a new AT_ flag for > > > this. > > > > > > This patch just makes statx(fd, NULL, AT_EMPTY_PATH, ...) behave > > > like > > > statx(fd, "", AT_EMPTY_PATH, ...) instead. NULL avoids the > > > performance > > > issue and it's also audit-able by seccomp BPF. > > To be honest, I still want to restore __ARCH_WANT_NEW_STAT. Because > > even if statx() becomes audit-able, it is still blacklisted now. > > Then patch the sandbox to allow it. > > The sandbox **must** be patched anyway or it'll be broken on all 32-bit > systems after 2037. [Unless they'll unsupport all 32-bit systems before > 2037.] Yes, but it will not happen immediately. > > > Restoring __ARCH_WANT_NEW_STAT is a very small change that doesn't > > introduce any complexity, but it makes life easier. And I think libLoL > > also likes __ARCH_WANT_NEW_STAT, though it isn't an upstream > > project... > > At least you should not restore it for 32-bit. libLoL also has nothing > to do with 32-bit systems anyway. Maybe conditional it with a #if > checking __BITS_PER_LONG. Agree, but currently LoongArch only support 64bit, so we don't need #ifdef now (Many Kconfig options also need to depend on 64bit, but dependencies are removed when LoongArch get upstream). > > And the vendors should really port their software to the upstreamed ABI > instead of relying on liblol. <rant>Is a recompiling so difficult, or > are the programmers so stupid to invoke plenty of low-level syscalls > directly (bypassing Glibc) in their code?</rant> Unfortunately, libLoL may exist for a very long time. Recompiling isn't difficult, the real problem is "I have already ported to LoongArch, why should I port again?". Huacai > > -- > Xi Ruoyao <xry111@xry111.site> > School of Aerospace Science and Technology, Xidian University >
On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote:
> + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
Could it be
if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename))
instead?
When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at
least Glibc developers think it's needed:
#if FSTATAT_USE_STATX
static inline int
fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
int flag)
{
/* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also
64-bit time_t support is done through statx syscall. */
struct statx tmp;
int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
STATX_BASIC_STATS, &tmp);
so "flags == AT_EMPTY_PATH" won't be true if Glibc implements fstatat
and fstat via statx (on LoongArch and 32-bit platforms).
I was just surprised when I saw a 100%+ improve for statx("",
AT_EMPTY_PATH) but not stat on the Loongson machine.
> + return do_statx_fd(dfd, flags, mask, buffer);
> +
> name = getname_flags(filename, getname_statx_lookup_flags(flags), NULL);
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
On Tue, 2024-06-25 at 21:24 +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote:
> > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
>
> Could it be
>
> if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename))
>
> instead?
>
> When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at
> least Glibc developers think it's needed:
/* snip */
> I was just surprised when I saw a 100%+ improve for statx("",
> AT_EMPTY_PATH) but not stat on the Loongson machine.
^^^^ fstat
I cannot type :(
--
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University
On Tue, Jun 25, 2024 at 3:24 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Tue, 2024-06-25 at 13:00 +0200, Mateusz Guzik wrote:
> > + if (flags == AT_EMPTY_PATH && vfs_empty_path(dfd, filename))
>
> Could it be
>
> if ((flags & AT_EMPTY_PATH) && vfs_empty_path(dfd, filename))
>
> instead?
>
> When fstatat is implemented with statx AT_NO_AUTOMOUNT is needed, or at
> least Glibc developers think it's needed:
>
> #if FSTATAT_USE_STATX
>
> static inline int
> fstatat64_time64_statx (int fd, const char *file, struct __stat64_t64 *buf,
> int flag)
> {
> /* 32-bit kABI with default 64-bit time_t, e.g. arc, riscv32. Also
> 64-bit time_t support is done through statx syscall. */
> struct statx tmp;
> int r = INTERNAL_SYSCALL_CALL (statx, fd, file, AT_NO_AUTOMOUNT | flag,
> STATX_BASIC_STATS, &tmp);
>
> so "flags == AT_EMPTY_PATH" won't be true if Glibc implements fstatat
> and fstat via statx (on LoongArch and 32-bit platforms).
>
> I was just surprised when I saw a 100%+ improve for statx("",
> AT_EMPTY_PATH) but not stat on the Loongson machine.
>
It can't be like that specifically because we still need to catch
bogus AT flags.
I'm going to poke a little bit and send a v2, thanks.
--
Mateusz Guzik <mjguzik gmail.com>
© 2016 - 2025 Red Hat, Inc.