Re: [PATCH] select: do_pollfd: add unlikely branch hint return path

[Mateusz Guzik]

On Wed, Apr 9, 2025 at 5:23 PM Colin King (gmail)
<colin.i.king@xxxxxxxxx> wrote:
>
> On 09/04/2025 16:18, Mateusz Guzik wrote:
> > On Wed, Apr 09, 2025 at 03:31:38PM +0100, Colin Ian King wrote:
> >> Adding an unlikely() hint on the fd < 0 comparison return path improves
> >> run-time performance of the mincore system call. gcov based coverage
> >> analysis shows that this path return path is highly unlikely.
> >>
> >> Benchmarking on an Debian based Intel(R) Core(TM) Ultra 9 285K with
> >> a 6.15-rc1 kernel and a poll of 1024 file descriptors with zero timeout
> >> shows an call reduction from 32818 ns down to 32635 ns, which is a ~0.5%
> >> performance improvement.
> >>
> >> Results based on running 25 tests with turbo disabled (to reduce clock
> >> freq turbo changes), with 30 second run per test and comparing the number
> >> of poll() calls per second. The % standard deviation of the 25 tests
> >> was 0.08%, so results are reliable.
> >>
> >
> > I don't think adding a branch hint warrants benchmarking of the sort.
> >
> > Instead the thing to do is to check if the prediction matches real world
> > uses.
> >
> > While it is impossible to check this for all programs out there, it
> > should not be a significant time investment to look to check some of the
> > popular ones out there. Normally I would do it with bpftrace, but this
> > comes from a user-backed area instead of func args, so involved hackery
> > may be needed which is not warranted the change. Perhaps running strace
> > on a bunch of network progs would also do it (ssh, browser?).
> >
> > I have to say I did not even know one can legally pass a fd < 0 to poll
> > and I never seen it in action, so I don't expect many users. ;)
>
> I did check this based on gcov coverage (mentioned in the commit
> message) and this is based on running gcov data from running stress-ng
> and kernel builds and I've been looking for branch hint performance wins
> based on the top 250 if statements that are not already hinted using
> likely/unlikely.
>

Well now that you mention it, the commit message claims *mincore*. :)
I presume not edited from a different submission.

You did not specify what you fed to gcov in there though.

The kernel build is fine.

However, stress-ng intentionally does weird things and can't be used
as an argument here. It just may or may not accidentally line up with
reality and any wins/loses have to be analyzed for legitimacy.

I just straced ssh and firefox, both of which poll and only with
non-negative fds [that I have seen], so I think the patch is fine.
-- 
Mateusz Guzik <mjguzik gmail.com>