On 8/8/23, Christian Brauner <brauner@xxxxxxxxxx> wrote: >> I don't think perf tax on something becomes more sensible the longer >> it is there. > > One does need to answer the question why it does suddenly become > relevant after all these years though. > There is some work I'm considering doing, but before that happens I'm sanity checking performance of various syscalls and I keep finding problems, some of which are trivially avoidable. I'm genuinely confused with the strong opposition to the very notion of making close(2) a special case (which I consider conceptually trivial), but as you noted below I'm not ultimately the person on the hook for any problems. > The original discussion was triggered by fifo ordering in task work > which led to a noticable regression and why it was ultimately reverted. > The sync proposal for fput() was an orthogonal proposal and the > conclusion was that it wasn't safe generally > https://lore.kernel.org/all/20150905051915.GC22011@xxxxxxxxxxxxxxxxxx > even though it wasn't a direct response to the patch you linked. > Ok, I missed this e-mail. It further discourages patching filp_close, but does not make an argument against *just* close(2) rolling with sync which is what I'm proposing. > If you care about it enough send a patch that just makes close(2) go > sync. But this is precisely what the submitted patch is doing. It adds file_fput_sync, then adds close_fd_sync which is the only consumer and only makes close(2) use it. *nobody* else has sync added. One can argue the way this is sorted out is crap and I'm not going to defend it. I am saying making *just* close(2) roll with sync is very easy, there are numerous ways to do it and anyone involved with maintaining vfs can write their own variant in minutes. Basically I don't see *technical* problems here. > We'll stuff it in a branch and we'll see what LKP has to say about > it or whether this gets lost in noise. I really don't think letting > micro-benchmarks become a decisive factor for code churn is a good > idea. > That would be nice. Given the patch is already doing what you asked, can you just take it as is? I'll note though what I mentioned elsewhere (https://lore.kernel.org/all/CAGudoHEG7vtCRWjn0yR5LMUsaw3KJANfa+Hkke9gy0imXQz6tg@xxxxxxxxxxxxxx/): can they make sure to whack CONFIG_RANDOMIZE_KSTACK_OFFSET=y from their kernel config? It is an *optional* measure and it comes at a massive premium, so single-threaded changes are easily diminished. -- Mateusz Guzik <mjguzik gmail.com>
