On Sat, 25 May 2024 at 20:45, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> Checking the theory that the important part in sockfd_lookup_light() is
> avoiding needless file refcount operations, not the marginal reduction
> of the register pressure from not keeping a struct file pointer in the
> caller.
Yeah, the register pressure thing is not likely an issue. That said, I
think we can fix it.
> If that's true, we should get the same benefits from straight fdget()/fdput().
Agreed.
That said, your patch is just horrendous.
> +static inline bool fd_empty(struct fd f)
> +{
> + // better code with gcc that way
> + return unlikely(!(f.flags | (unsigned long)f.file));
> +}
Ok, this is disgusting. I went all "WTF?"
And then looked at why you would say that testing two different fields
in a 'struct fd' would possibly be better than just checking one.
And I see why that's the case - it basically short-circuits the
(inlined) __to_fd() logic, and the compiler can undo it and look at
the original single-word value.
But it's still disgusting. If there is anything else in between, the
compiler wouldn't then notice any more.
What we *really* should do is have 'struct fd' just *be* that
single-word value, never expand it to two words at all, and instead of
doing "fd.file" we'd do "fd_file(fd)" and "fd_flags(fd)".
Maybe it's not too late to do that?
This is *particularly* true for the socket code that doesn't even want
the 'struct file *' at all outside of that "check that it's a socket,
then turn it into a socket pointer". So _particularly_ in that
context, having a "fd_file()" helper to do the (trivial) unpacking
would work very well.
But even without changing 'struct fd', maybe we could just have
"__fdget()" and friends not return a "unsigned long", but a "struct
rawfd".
Which would is a struct with an unsigned long.
There's actually a possible future standard C++ extension for what we
want to do ("C++ tagged pointers") and while it might make it into C
eventually, we'd have to do it manully with ugly inline helpers (LLVM
does it manually in C++ with a PointerIntPair class).
IOW, we could do something like the attached. I think it's actually
almost a cleanup, and now your socket things can use "struct rawfd"
and that fd_empty() becomes
static inline bool rawfd_empty(struct rawfd raw)
{ return !raw.word; }
instead. Still somewhat disgusting, but now it's a "C doesn't have
tagged pointers, so we do this by hand" _understandable_ disgusting.
Hmm? The attached patch compiles. It looks "ObviouslyCorrect(tm)". But
it might be "TotallyBroken(tm)". You get the idea.
Linus
fs/file.c | 22 +++++++++++-----------
include/linux/file.h | 22 +++++++++++++++++-----
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 8076aef9c210..a1f44760ddbf 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -1128,7 +1128,7 @@ EXPORT_SYMBOL(task_lookup_next_fdget_rcu);
* The fput_needed flag returned by fget_light should be passed to the
* corresponding fput_light.
*/
-static unsigned long __fget_light(unsigned int fd, fmode_t mask)
+static struct rawfd __fget_light(unsigned int fd, fmode_t mask)
{
struct files_struct *files = current->files;
struct file *file;
@@ -1145,22 +1145,22 @@ static unsigned long __fget_light(unsigned int fd, fmode_t mask)
if (likely(atomic_read_acquire(&files->count) == 1)) {
file = files_lookup_fd_raw(files, fd);
if (!file || unlikely(file->f_mode & mask))
- return 0;
- return (unsigned long)file;
+ return EMPTY_RAWFD;
+ return (struct rawfd) { (unsigned long)file };
} else {
file = __fget_files(files, fd, mask);
if (!file)
- return 0;
- return FDPUT_FPUT | (unsigned long)file;
+ return EMPTY_RAWFD;
+ return (struct rawfd) { FDPUT_FPUT | (unsigned long)file };
}
}
-unsigned long __fdget(unsigned int fd)
+struct rawfd __fdget(unsigned int fd)
{
return __fget_light(fd, FMODE_PATH);
}
EXPORT_SYMBOL(__fdget);
-unsigned long __fdget_raw(unsigned int fd)
+struct rawfd __fdget_raw(unsigned int fd)
{
return __fget_light(fd, 0);
}
@@ -1181,13 +1181,13 @@ static inline bool file_needs_f_pos_lock(struct file *file)
(file_count(file) > 1 || file->f_op->iterate_shared);
}
-unsigned long __fdget_pos(unsigned int fd)
+struct rawfd __fdget_pos(unsigned int fd)
{
- unsigned long v = __fdget(fd);
- struct file *file = (struct file *)(v & ~3);
+ struct rawfd v = __fdget(fd);
+ struct file *file = fdfile(v);
if (file && file_needs_f_pos_lock(file)) {
- v |= FDPUT_POS_UNLOCK;
+ v.word |= FDPUT_POS_UNLOCK;
mutex_lock(&file->f_pos_lock);
}
return v;
diff --git a/include/linux/file.h b/include/linux/file.h
index 45d0f4800abd..603aa32f985c 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -42,6 +42,18 @@ struct fd {
#define FDPUT_FPUT 1
#define FDPUT_POS_UNLOCK 2
+/* "Tagged file pointer" */
+struct rawfd {
+ unsigned long word;
+};
+#define EMPTY_RAWFD (struct rawfd) { 0 }
+
+static inline struct file *fdfile(struct rawfd raw)
+{ return (struct file *) (raw.word & ~3); }
+
+static inline unsigned int fdflags(struct rawfd raw)
+{ return raw.word & 3; }
+
static inline void fdput(struct fd fd)
{
if (fd.flags & FDPUT_FPUT)
@@ -51,14 +63,14 @@ static inline void fdput(struct fd fd)
extern struct file *fget(unsigned int fd);
extern struct file *fget_raw(unsigned int fd);
extern struct file *fget_task(struct task_struct *task, unsigned int fd);
-extern unsigned long __fdget(unsigned int fd);
-extern unsigned long __fdget_raw(unsigned int fd);
-extern unsigned long __fdget_pos(unsigned int fd);
+extern struct rawfd __fdget(unsigned int fd);
+extern struct rawfd __fdget_raw(unsigned int fd);
+extern struct rawfd __fdget_pos(unsigned int fd);
extern void __f_unlock_pos(struct file *);
-static inline struct fd __to_fd(unsigned long v)
+static inline struct fd __to_fd(struct rawfd raw)
{
- return (struct fd){(struct file *)(v & ~3),v & 3};
+ return (struct fd){fdfile(raw),fdflags(raw)};
}
static inline struct fd fdget(unsigned int fd)
