On Thu, Aug 03, 2023 at 11:53:11AM +0200, Mateusz Guzik wrote:
> On Mon, Jul 24, 2023 at 09:59:15AM -0700, Linus Torvalds wrote:
> > I really hate making the traditional unix single-threaded file
> > descriptor case take that lock.
> >
> > Maybe it doesn't matter. Obviously it can't have contention, and your
> > patch in that sense is pretty benign.
> >
> > But locking is just fundamentally expensive in the first place, and it
> > annoys me that I never realized that pidfd_getfd() did that thing that
> > I knew was broken for /proc.
> >
>
> So I got curious what the impact is and checked on quite a modern CPU
> (Sapphire Rapid), so nobody can claim it's some old yeller and atomics
> are nowhere near as expensive on modern uarchs.
>
> I used read1_processes from will-it-scale -- it is doing 4KB reads at a
> time over a 1MB file and dodges refing the file, but it does not dodge
> the lock with the patch at hand.
>
> In short, I got a drop of about 5% (~5778843 -> ~5500871 ops/s).
>
> The kernel was patched with a toggle to force or elide the proposed
> mandatory locking, like so:
> @@ -1042,8 +1044,10 @@ unsigned long __fdget_pos(unsigned int fd)
> struct file *file = (struct file *)(v & ~3);
>
> if (file && (file->f_mode & FMODE_ATOMIC_POS)) {
> - v |= FDPUT_POS_UNLOCK;
> - mutex_lock(&file->f_pos_lock);
> + if (file_count(file) > 1 || fdget_pos_mutex) {
> + v |= FDPUT_POS_UNLOCK;
> + mutex_lock(&file->f_pos_lock);
> + }
> }
> return v;
> }
>
> I got rather unstable single-threaded perf, going up and down several %
> between runs, I don't know yet what's that about. But toggling back and
We've had the whole lkp intel performance testsuite run on this for a
long time to see whether there any performance regressions that aren't
in the noise. This includes all of will-it-scale. Also with a focus on
single-thread performance. So I'm a little skeptical about the
reliability of manual performance runs in comparison.
If Linus thinks it matters then I think we should only take the
f_pos_lock unconditionally on directory fds as this is where it really
matters. For read/write we're only doing it because of posix anyway and
using pidfd_getfd() is squarely out of posix anyway. So we can document
that using pidfd_getfd() on a regular file descriptor outside of a
seccomp stopped task is not recommended.
