archive mirror
 help / color / mirror / Atom feed
From: Linus Torvalds <>
To: Jann Horn <>
Cc: kernel test robot <>,
	Miklos Szeredi <>,
	LKML <>,, kernel test robot <>,
	"Huang, Ying" <>,
	Feng Tang <>,
	Zhengjun Xing <>,
Subject: Re: [fget] 054aa8d439: will-it-scale.per_thread_ops -5.7% regression
Date: Fri, 10 Dec 2021 13:25:56 -0800	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Fri, Dec 10, 2021 at 12:30 PM Jann Horn <> 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

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".


  reply	other threads:[~2021-12-10 21:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-10  5:37 [fget] 054aa8d439: will-it-scale.per_thread_ops -5.7% regression kernel test robot
2021-12-10 18:33 ` Linus Torvalds
2021-12-10 20:29   ` Jann Horn
2021-12-10 21:25     ` Linus Torvalds [this message]
2021-12-10 21:59       ` Linus Torvalds
2021-12-10 23:29         ` Jann Horn
2021-12-11  1:01           ` Linus Torvalds
2021-12-11  1:32         ` Linus Torvalds
2021-12-13  8:31         ` [LKP] " Carel Si
2021-12-13 18:37           ` Linus Torvalds
2021-12-13 19:44             ` Linus Torvalds
2021-12-15 12:54               ` Greg KH
2021-12-13 10:57   ` Carel Si

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='' \ \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).