Re: [fget] 054aa8d439: will-it-scale.per_thread_ops -5.7% regression

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Dec 10, 2021 at 12:30 PM Jann Horn <jannh@google.com> wrote:
>
> Oh, and I just realized that your patch probably actually also fixes
> an issue entirely unrelated to unix sockets. __fdget_pos() does this:

Hmm. I was going to say that you're wrong, because that case doesn't
actually have that "offset" thing that the unix domain GC code has,
and a "true zero" reference count is special and will never come back
to life.

But I think you're right - the "close and resurrect" situation wrt the
file count can happen _after_ the __fdget() in __fdget_pos() (where we
have that implicit offset of our own ref), and thus show a false case
of "we're the only user".

So I think you've convinced me - doing it in __fget_files() was the
right thing to do, but I really don't like that 5% regression.

Maybe it's purely on that artificial benchmark, but a multi-threaded
poll loop doesn't sound super-unusual (I think a single-threaded one
is already protected from this all by our "__fget_light()" logic).

Sadly, looking at my gcc code generation, adding that "unlikely()"
does move the "fput_many()" call out to it's own out-of-line code
section, but gcc will still end up doing the stack frame around the
whole function.

So if it's all due to just extra code and stack references due to the
now necessary stack-frame, it doesn't look obvious how to improve on
that.

We could make a special light-weight version of files_lookup_fd_raw(),
I guess. We don't need the *whole* "look it up again".  We don't need
to re-check the array bounds, and we don't need to do the nospec
lookup - we would have triggered a NULL file pointer if that happened
the first time around.

So all we'd need to do is "check that fdt is the same, and check that
fdt->fd[fd] is the same".

                 Linus