8 ms·

What the proposed patch does is delay a specific latent operation to an asynchronous context so that close() doesn’t block on that operation (which is freeing s

What the proposed patch does is delay a specific latent operation to an asynchronous context so that close() doesn’t block on that operation (which is freeing some memory). The proposed patch isn’t a comprehensive fix, it admits there are still other sources of relatively high close() latency. So that got me thinking, there is no way to fix this “bug” because there is no specification on how long close() should take to complete. As far as we are promised in user-land, close() is not an instantaneous operation. close() is a blocking operation! Even worse, it’s an IO operation. So now I think the bug is in the application. If you want to avoid the latency of close() you should do it asynchronously in another thread. This is similar to the rule that you should not do blocking IO on the main thread in an event-loop based application.

touisteur 5y ago
Or, io_uring the thing. One could probably wrap close() with LD_PRELOAD and not touch the binary...
- CyberRabbi 5y ago
  While tempting, you can’t generally fix this by simply patching close() with some function that converts it to an unchecked asynchronous operation. If that were the case, you could just do that in the kernel. Close() is expected to complete synchronously. This matters because posix guarantees that open()/pipe() etc. will return the lowest file descriptor not in use[1]. I.e. this should work: close(0); fd = open(“/foo/bar”, …); // fd is guaranteed to be 0 If you made close() just dispatch an asynchronous operation and not wait on the result, then the code above would break. Any code that uses dup() likely has code that expects close() to behave that way. The other issue is that close() can return errors. Most applications ignore close errors but to be a robust solution you’d need to ensure the target application ignores those errors as well. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_14 https://pubs.opengroup.org/onlinepubs/9699919799/functions/V...
  - deleted 5y ago
    [deleted]
  - treis 5y ago
    >This matters because posix guarantees that open()/pipe() etc. will return the lowest file descriptor not in use[1]. I.e. this should work: close(0); fd = open(“/foo/bar”, …); // fd is guaranteed to be 0 On a multi threaded system that isn't guaranteed is it? Meaning, another thread could call open in-between your close & open.
    - CyberRabbi 5y ago
      It is guaranteed whether multi-threaded or not. It’s a process level guarantee. If your application is designed such that you don’t know what your other threads are doing then POSIX cannot help you.
    - loeg 5y ago
      What you’re getting at is that an individual thread cannot really use this property without some form of synchronization with other threads in the process. Eg, to use this property, other threads either do not allocate fds, or you take some central lock around all fd allocations. Most well-written programs do not rely on it.
  - infogulch 5y ago
    I did not know this, and for some reason it really annoys me. Why are our process contexts littered with useless little synchronous properties? How many other tedious and slow bookkeeping tasks does the OS have to do just to meet some outdated spec that was probably just an ossified implementation detail in the first place? I feel compelled to make it so that new fds are explicitly randomized just so you can't do this, like how Go randomizes map iteration order.
    - kennylevinsen 5y ago
      This argument does not make sense - the kernel already needs to track per-process file descriptors. It just looks for the first hole instead of giving the "next" value. Go's random map iteration does not apply here. Not only is this not an iterable map, the kernel has no problem providing this insertion guarantee so adding additional costly randomization has no benefit and just burns additional cycles. Go would also be better off without, but they are catering to a different audience and different degree of specification, and apparently need to actively deter developers from ignoring documentation.
londons_explore 5y ago
close() is typically a blocking operation. But when it happens in devfs, procfs, tmpfs, or some other ram only filesystem I expect it to be fast unless documented otherwise.
- CyberRabbi 5y ago
  > I expect it to be fast unless documented otherwise. Logically you should expect it to block indefinitely unless documented otherwise. The exception would be completing within a time bound, the rule is blocking indefinitely.
  - loeg 5y ago
    > Logically you should expect it to block indefinitely Frankly, that’s completely insane. It should block if and only if there is actual io in flight which could produce a failure return that an application needs. Syscalls should be fast unless there is a very good reason not to be.
    - CyberRabbi 5y ago
      > It should block if and only if there is actual io in flight which could produce a failure return that an application needs. Blocking simply means that the specification does not guarantee an upper bound on the completion time. There is no other meaningful definition. POSIX is not an RTOS therefore nearly all system calls block. The alternative is that the specification guarantees an upper bound on completion time. In that case what is an acceptable upper bound for close() to complete in? 1ms? 10ms? 100ms? Any answer diminishes the versatility of the POSIX VFS. > Syscalls should be fast unless there is a very good reason not to be. I think this is an instance of confusing what should be with what is. We’ve been through this before with O_PONIES. The reality is that system calls aren’t “fast” and they can’t portably or dynamically be guaranteed to be fast. So far the only exception to this is gettimeofday() and friends. Robust systems aren’t built on undocumented assumptions. Again, POSIX is not an RTOS. Anything you build that assumes a deterministic upper bound to a blocking system call execution time will inevitably break, evidenced by OP.
throwaway81523 5y ago
You should use io_uring to open and close files asynchronously, instead of open/close.
- kennylevinsen 5y ago
  You would not use io_uring for things like that. Not only will you still use regular file operations on device files for various reasons, should you chose to use io_uring you would want it to run your entire eventloop and all you I/O rather than single operations here and there. Otherwise it just adds complexity with no benefit.
  - throwaway81523 5y ago
    I don't see the big issue. There is no other way in Linux or Posix to open a file asynchronously (not sure about closing). Dan Bernstein complained about that 20 years ago(?) and io_uring finally fixes it. Before that, runtimes with lightweight processes/threads (Erlang, GHC) used a Posix threadpool to open files in the background. That seems just as messy as using io_uring, which at least keeps everything in the same thread. http://cr.yp.to/unix/asyncdisk.html http://cr.yp.to/unix/asyncdisk.html
kennylevinsen 5y ago
Patch author here. It is important to not conflate POSIX requirements with expected behavior, especially for device files which require very specific knowledge of their implementation to use (DRM ioctl's and resources anyone?). You might think that as a well-behaved game should not be opening/closing evdev fds during gameplay at all, this is clearly just an application bug. However, games are not the main user of evdev devices, your display server is! This bug causes input device closure during session switching (e.g. VT switching) to take abnormally long - on the machine I discovered the bug on, it ends up adding over a second to the session switch time, significantly impacting responsiveness. This is absolutely a kernel bug. I did not push the patch further as I had other priorities, and testing this kind of patch is quite time-consuming when it only reproduces in a measurable way on single physical machine. Other machines end up with a much shorter synchronize_rcu wait and often have many fewer input devices, explaining why the issue was not discovered/fixed earlier. call_rcu is intended to be used wherever you do not want the writer to block, while alternative fixes involve synchronize_rcu_expedited (very fast but expensive), identifying if the long synchronize_rcu wait is itself a bug that could be fixed (might be correct), or possibly refactoring evdev (which is quite a simple device file). As for putting things in threads, I would consider it a huge hack to move open/close. Threads are not and will never be mandatory to have great responsiveness.
- CyberRabbi 5y ago
  > As for putting things in threads, I would consider it a huge hack to move open/close. Threads are not and will never be mandatory to have great responsiveness. The POSIX interface was invented for batch processing. Long running non-interactive jobs. This is why it lacks timing requirements. All well-designed interactive GUI applications do not interact with the file system on their main thread. This is especially true for game display loops. The fundamental problem here is that they are doing unbounded work on a thread that has specific timing requirements (usually 16.6ms per loop). As I’ve said elsewhere, this bug will still manifest itself no matter how fast you make close(), just depends on how many device files are present on that particular system. It’s a poor design. Well designed games account for every line of code run in their drawing loop. > This is absolutely a kernel bug. I don’t think that is proven unless the original author can chime in. It’s your best guess and opinion that the author intended to not block on synchronize_rcu but it’s perfectly possible they did indeed intend the code as written. synchronize_rcu is used in plenty of other critical system call paths in similar ways, not every one of those uses is a bug. I would guess you might be slightly suffering from tunnel vision a bit here given how the behavior was discovered. If it is indeed the case the synchronize_rcu is taking up to 50ms I would suspect there is a deeper issue at play on this machine. By search/replacing the call with call_rcu or similar you may just be masking the problem. RCU updates should not be taking that long.

touisteur 5y ago

Or, io_uring the thing. One could probably wrap close() with LD_PRELOAD and not touch the binary...

CyberRabbi 5y ago
While tempting, you can’t generally fix this by simply patching close() with some function that converts it to an unchecked asynchronous operation. If that were the case, you could just do that in the kernel. Close() is expected to complete synchronously. This matters because posix guarantees that open()/pipe() etc. will return the lowest file descriptor not in use[1]. I.e. this should work: close(0); fd = open(“/foo/bar”, …); // fd is guaranteed to be 0 If you made close() just dispatch an asynchronous operation and not wait on the result, then the code above would break. Any code that uses dup() likely has code that expects close() to behave that way. The other issue is that close() can return errors. Most applications ignore close errors but to be a robust solution you’d need to ensure the target application ignores those errors as well. [1]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_14 https://pubs.opengroup.org/onlinepubs/9699919799/functions/V...
- deleted 5y ago
  [deleted]
- treis 5y ago
  >This matters because posix guarantees that open()/pipe() etc. will return the lowest file descriptor not in use[1]. I.e. this should work: close(0); fd = open(“/foo/bar”, …); // fd is guaranteed to be 0 On a multi threaded system that isn't guaranteed is it? Meaning, another thread could call open in-between your close & open.
  - CyberRabbi 5y ago
    It is guaranteed whether multi-threaded or not. It’s a process level guarantee. If your application is designed such that you don’t know what your other threads are doing then POSIX cannot help you.
  - loeg 5y ago
    What you’re getting at is that an individual thread cannot really use this property without some form of synchronization with other threads in the process. Eg, to use this property, other threads either do not allocate fds, or you take some central lock around all fd allocations. Most well-written programs do not rely on it.
- infogulch 5y ago
  I did not know this, and for some reason it really annoys me. Why are our process contexts littered with useless little synchronous properties? How many other tedious and slow bookkeeping tasks does the OS have to do just to meet some outdated spec that was probably just an ossified implementation detail in the first place? I feel compelled to make it so that new fds are explicitly randomized just so you can't do this, like how Go randomizes map iteration order.
  - kennylevinsen 5y ago
    This argument does not make sense - the kernel already needs to track per-process file descriptors. It just looks for the first hole instead of giving the "next" value. Go's random map iteration does not apply here. Not only is this not an iterable map, the kernel has no problem providing this insertion guarantee so adding additional costly randomization has no benefit and just burns additional cycles. Go would also be better off without, but they are catering to a different audience and different degree of specification, and apparently need to actively deter developers from ignoring documentation.

londons_explore 5y ago

close() is typically a blocking operation. But when it happens in devfs, procfs, tmpfs, or some other ram only filesystem I expect it to be fast unless documented otherwise.

CyberRabbi 5y ago
> I expect it to be fast unless documented otherwise. Logically you should expect it to block indefinitely unless documented otherwise. The exception would be completing within a time bound, the rule is blocking indefinitely.
- loeg 5y ago
  > Logically you should expect it to block indefinitely Frankly, that’s completely insane. It should block if and only if there is actual io in flight which could produce a failure return that an application needs. Syscalls should be fast unless there is a very good reason not to be.
  - CyberRabbi 5y ago
    > It should block if and only if there is actual io in flight which could produce a failure return that an application needs. Blocking simply means that the specification does not guarantee an upper bound on the completion time. There is no other meaningful definition. POSIX is not an RTOS therefore nearly all system calls block. The alternative is that the specification guarantees an upper bound on completion time. In that case what is an acceptable upper bound for close() to complete in? 1ms? 10ms? 100ms? Any answer diminishes the versatility of the POSIX VFS. > Syscalls should be fast unless there is a very good reason not to be. I think this is an instance of confusing what should be with what is. We’ve been through this before with O_PONIES. The reality is that system calls aren’t “fast” and they can’t portably or dynamically be guaranteed to be fast. So far the only exception to this is gettimeofday() and friends. Robust systems aren’t built on undocumented assumptions. Again, POSIX is not an RTOS. Anything you build that assumes a deterministic upper bound to a blocking system call execution time will inevitably break, evidenced by OP.

throwaway81523 5y ago

You should use io_uring to open and close files asynchronously, instead of open/close.

kennylevinsen 5y ago
You would not use io_uring for things like that. Not only will you still use regular file operations on device files for various reasons, should you chose to use io_uring you would want it to run your entire eventloop and all you I/O rather than single operations here and there. Otherwise it just adds complexity with no benefit.
- throwaway81523 5y ago
  I don't see the big issue. There is no other way in Linux or Posix to open a file asynchronously (not sure about closing). Dan Bernstein complained about that 20 years ago(?) and io_uring finally fixes it. Before that, runtimes with lightweight processes/threads (Erlang, GHC) used a Posix threadpool to open files in the background. That seems just as messy as using io_uring, which at least keeps everything in the same thread. http://cr.yp.to/unix/asyncdisk.html http://cr.yp.to/unix/asyncdisk.html

kennylevinsen 5y ago

Patch author here. It is important to not conflate POSIX requirements with expected behavior, especially for device files which require very specific knowledge of their implementation to use (DRM ioctl's and resources anyone?). You might think that as a well-behaved game should not be opening/closing evdev fds during gameplay at all, this is clearly just an application bug. However, games are not the main user of evdev devices, your display server is! This bug causes input device closure during session switching (e.g. VT switching) to take abnormally long - on the machine I discovered the bug on, it ends up adding over a second to the session switch time, significantly impacting responsiveness. This is absolutely a kernel bug. I did not push the patch further as I had other priorities, and testing this kind of patch is quite time-consuming when it only reproduces in a measurable way on single physical machine. Other machines end up with a much shorter synchronize_rcu wait and often have many fewer input devices, explaining why the issue was not discovered/fixed earlier. call_rcu is intended to be used wherever you do not want the writer to block, while alternative fixes involve synchronize_rcu_expedited (very fast but expensive), identifying if the long synchronize_rcu wait is itself a bug that could be fixed (might be correct), or possibly refactoring evdev (which is quite a simple device file). As for putting things in threads, I would consider it a huge hack to move open/close. Threads are not and will never be mandatory to have great responsiveness.

CyberRabbi 5y ago
> As for putting things in threads, I would consider it a huge hack to move open/close. Threads are not and will never be mandatory to have great responsiveness. The POSIX interface was invented for batch processing. Long running non-interactive jobs. This is why it lacks timing requirements. All well-designed interactive GUI applications do not interact with the file system on their main thread. This is especially true for game display loops. The fundamental problem here is that they are doing unbounded work on a thread that has specific timing requirements (usually 16.6ms per loop). As I’ve said elsewhere, this bug will still manifest itself no matter how fast you make close(), just depends on how many device files are present on that particular system. It’s a poor design. Well designed games account for every line of code run in their drawing loop. > This is absolutely a kernel bug. I don’t think that is proven unless the original author can chime in. It’s your best guess and opinion that the author intended to not block on synchronize_rcu but it’s perfectly possible they did indeed intend the code as written. synchronize_rcu is used in plenty of other critical system call paths in similar ways, not every one of those uses is a bug. I would guess you might be slightly suffering from tunnel vision a bit here given how the behavior was discovered. If it is indeed the case the synchronize_rcu is taking up to 50ms I would suspect there is a deeper issue at play on this machine. By search/replacing the call with call_rcu or similar you may just be masking the problem. RCU updates should not be taking that long.