Linux Kernel Mailing List wrote:
> Gitweb: http://git.kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=b8522ead3534c6cd06752b47a3bc380956191a2a
> Commit: b8522ead3534c6cd06752b47a3bc380956191a2a
> Parent: b41eeef14d7c73af6d16c7d02b7a939082a137ff
> Author: Andrew Morton <[email protected]>
> AuthorDate: Wed May 9 02:34:58 2007 -0700
> Committer: Linus Torvalds <[email protected]>
> CommitDate: Wed May 9 12:30:54 2007 -0700
>
> aio is unlikely
>
> Stick an unlikely() around is_aio(): I assert that most IO is synchronous.
>
> Cc: Suparna Bhattacharya <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Benjamin LaHaise <[email protected]>
> Cc: Zach Brown <[email protected]>
> Cc: Ulrich Drepper <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Signed-off-by: Andrew Morton <[email protected]>
> Signed-off-by: Linus Torvalds <[email protected]>
> ---
> include/linux/aio.h | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/aio.h b/include/linux/aio.h
> index a30ef13..43dc2eb 100644
> --- a/include/linux/aio.h
> +++ b/include/linux/aio.h
> @@ -226,7 +226,8 @@ int FASTCALL(io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
> __put_ioctx(kioctx); \
> } while (0)
>
> -#define in_aio() !is_sync_wait(current->io_wait)
> +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
Please revert. Workload-dependent "likelihood" should not cause
programmers to add such markers.
This is a common misunderstanding about unlikely() and likely(). The
branch prediction used for each assumes 99% unlikely or 99% likely,
which is not true at all for workload-dependent code.
Even if only 1% of Linux users use AIO, for that 1%, the 'unlikely'
marker causes repeated branch mispredictions.
likely() and unlikely() should be used for cases where code is
likely/unlikely for EVERYBODY.
Jeff
On Wed, 09 May 2007 18:06:58 -0400
Jeff Garzik <[email protected]> wrote:
> > -#define in_aio() !is_sync_wait(current->io_wait)
> > +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
>
> Please revert. Workload-dependent "likelihood" should not cause
> programmers to add such markers.
>
> This is a common misunderstanding about unlikely() and likely(). The
> branch prediction used for each assumes 99% unlikely or 99% likely,
> which is not true at all for workload-dependent code.
>
> Even if only 1% of Linux users use AIO, for that 1%, the 'unlikely'
> marker causes repeated branch mispredictions.
>
> likely() and unlikely() should be used for cases where code is
> likely/unlikely for EVERYBODY.
a) disagree with the above
b) if in_aio() ever returns true we do
printk(KERN_ERR "%s(%s:%d) called in async context!\n",
__FUNCTION__, __FILE__, __LINE__);
so I sure hope it's unlikely for all workloads.
Andrew Morton wrote:
> a) disagree with the above
>
> b) if in_aio() ever returns true we do
>
> printk(KERN_ERR "%s(%s:%d) called in async context!\n",
> __FUNCTION__, __FILE__, __LINE__);
>
> so I sure hope it's unlikely for all workloads.
hrm, indeed. Ignore me.
Jeff
Andrew Morton wrote:
> aio is unlikely
> Stick an unlikely() around is_aio(): I assert that most IO is
synchronous.
>
> -#define in_aio() !is_sync_wait(current->io_wait)
> +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
> Jeff Garzik <[email protected]> wrote:
>
> > > -#define in_aio() !is_sync_wait(current->io_wait)
> > > +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
> >
> > Please revert. Workload-dependent "likelihood" should not cause
> > programmers to add such markers.
> a) disagree with the above
>
> b) if in_aio() ever returns true we do
>
> printk(KERN_ERR "%s(%s:%d) called in async context!\n",
> __FUNCTION__, __FILE__, __LINE__);
>
> so I sure hope it's unlikely for all workloads.
Shouldn't unlikely() go where in_aio() is actually used, if we printk(error)
there?
Isn't putting likely/unlikely into a boolean function-like macro itself
asking for later trouble?
--Alex.
On Fri, 18 May 2007 16:49:49 -0400
"Alex Volkov" <[email protected]> wrote:
>
> Andrew Morton wrote:
> > aio is unlikely
> > Stick an unlikely() around is_aio(): I assert that most IO is
> synchronous.
> >
> > -#define in_aio() !is_sync_wait(current->io_wait)
> > +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
>
> > Jeff Garzik <[email protected]> wrote:
> >
> > > > -#define in_aio() !is_sync_wait(current->io_wait)
> > > > +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
> > >
> > > Please revert. Workload-dependent "likelihood" should not cause
> > > programmers to add such markers.
> > a) disagree with the above
> >
> > b) if in_aio() ever returns true we do
> >
> > printk(KERN_ERR "%s(%s:%d) called in async context!\n",
> > __FUNCTION__, __FILE__, __LINE__);
> >
> > so I sure hope it's unlikely for all workloads.
>
> Shouldn't unlikely() go where in_aio() is actually used, if we printk(error)
> there?
> Isn't putting likely/unlikely into a boolean function-like macro itself
> asking for later trouble?
>
Yes, if you agree with Jeff's original point.
But I don't, actually. Sure, on some machines+workloads, AIO is more
common than sync IO. But I expect that when we sum across all the
machines+workloads in the world, sync IO is more common and is hence the
case we should optimise for.
That's assuming that the unlikely() actually does something.
Andrew Morton wrote:
> Yes, if you agree with Jeff's original point.
>
> But I don't, actually. Sure, on some machines+workloads, AIO is more
> common than sync IO. But I expect that when we sum across all the
> machines+workloads in the world, sync IO is more common and is hence the
> case we should optimise for.
>
> That's assuming that the unlikely() actually does something.
For the record, I agreed with your counter-point, and retract[ed?] my
disagreement...
Jeff
In article <058f01c7998e$1406e370$650df7cd@MUMBA> you wrote:
>> -#define in_aio() !is_sync_wait(current->io_wait)
>> +#define in_aio() (unlikely(!is_sync_wait(current->io_wait)))
>
> Shouldn't unlikely() go where in_aio() is actually used, if we printk(error)
> there?
Actually I would just remove that define and use the method directly, if
this is the only place where it is used. If it is used in multiple places,
the unlikely is most likely wrong .)
Gruss
Bernd
Andrew Morton wrote:
> Yes, if you agree with Jeff's original point.
>
> But I don't, actually. Sure, on some machines+workloads, AIO is more
> common than sync IO. But I expect that when we sum across all the
> machines+workloads in the world, sync IO is more common and is hence the
> case we should optimise for.
>
> That's assuming that the unlikely() actually does something.
But as Jeff said, that's not what unlikely is for. It should only be
used when it is unlikely for everybody, all the time, because when it is
right, it helps rather little, but when it is wrong, it hurts a lot.
On Fri, 18 May 2007 17:54:32 -0400
Phillip Susi <[email protected]> wrote:
> Andrew Morton wrote:
> > Yes, if you agree with Jeff's original point.
> >
> > But I don't, actually. Sure, on some machines+workloads, AIO is more
> > common than sync IO. But I expect that when we sum across all the
> > machines+workloads in the world, sync IO is more common and is hence the
> > case we should optimise for.
> >
> > That's assuming that the unlikely() actually does something.
>
> But as Jeff said, that's not what unlikely is for. It should only be
> used when it is unlikely for everybody, all the time, because when it is
> right, it helps rather little, but when it is wrong, it hurts a lot.
It does? Tell us more.
Andrew Morton wrote:
> On Fri, 18 May 2007 17:54:32 -0400
> Phillip Susi <[email protected]> wrote:
>
>> Andrew Morton wrote:
>>> Yes, if you agree with Jeff's original point.
>>>
>>> But I don't, actually. Sure, on some machines+workloads, AIO is more
>>> common than sync IO. But I expect that when we sum across all the
>>> machines+workloads in the world, sync IO is more common and is hence the
>>> case we should optimise for.
>>>
>>> That's assuming that the unlikely() actually does something.
>> But as Jeff said, that's not what unlikely is for. It should only be
>> used when it is unlikely for everybody, all the time, because when it is
>> right, it helps rather little, but when it is wrong, it hurts a lot.
>
> It does? Tell us more.
It is difficult to quantify either way. The details are both
CPU-specific and compiler-specific. The best information can be culled
from the gcc list archives, which is where I obtained my knowledge on
the subject (which is now ~2 years old).
Under the hood, likely() and unlikely() are implemented as percentage
predictions. likely() is implemented in the kernel as a 99-100% chance
of success, and unlikely() is implemented as a 0-1% chance of success.
As such, for our purposes, likely() and unlikely() should only be used
when a situation is [likely | unlikely] across all runtime
configurations. So if you mark a branch unlikely() when it is hit often
by 1% of your users, that is an incorrect usage.
The effects are probably most dramatic on older CPUs. Repeatedly
hitting an unlikely() can cause a pipeline stall on every single access.
Branch delay slots are filled improperly, with obvious implications.
But on modern hardware, I would /guess/ that the effect of repeatedly
hitting an unlikely() would be mitigated by smarter branch prediction.
We really need a GCC expert to answer this question in any more detail.
Jeff
Jeff Garzik wrote:
> Andrew Morton wrote:
>
>> On Fri, 18 May 2007 17:54:32 -0400
>> Phillip Susi <[email protected]> wrote:
>>> But as Jeff said, that's not what unlikely is for. It should only be
>>> used when it is unlikely for everybody, all the time, because when it
>>> is right, it helps rather little, but when it is wrong, it hurts a lot.
>>
>>
>> It does? Tell us more.
>
>
> It is difficult to quantify either way. The details are both
> CPU-specific and compiler-specific. The best information can be culled
> from the gcc list archives, which is where I obtained my knowledge on
> the subject (which is now ~2 years old).
>
> Under the hood, likely() and unlikely() are implemented as percentage
> predictions. likely() is implemented in the kernel as a 99-100% chance
> of success, and unlikely() is implemented as a 0-1% chance of success.
>
> As such, for our purposes, likely() and unlikely() should only be used
> when a situation is [likely | unlikely] across all runtime
> configurations. So if you mark a branch unlikely() when it is hit often
> by 1% of your users, that is an incorrect usage.
>
> The effects are probably most dramatic on older CPUs. Repeatedly
> hitting an unlikely() can cause a pipeline stall on every single access.
> Branch delay slots are filled improperly, with obvious implications.
>
> But on modern hardware, I would /guess/ that the effect of repeatedly
> hitting an unlikely() would be mitigated by smarter branch prediction.
>
> We really need a GCC expert to answer this question in any more detail.
Aside from using branch constructs or hints that help the predictor
guess the right way... I think gcc will move unlikely paths right past
the end of the "likely" fastpath, so it can increase code size and be
somewhat suboptimal in terms of icache usage.
I don't know particularly why it would hurt a lot more when it goes
wrong than it helps when it goes right, though.
Also, I don't think I agree that it should be used where it is correct
for all users. We make rt_task unlikely in the scheduler, and I measured
that a very long time ago was IIRC good for nearly 5% pipe based context
switching peformance. Systems running a lot of rt tasks aren't going to
like it, but bugger them :)
--
SUSE Labs, Novell Inc.
Nick Piggin wrote:
> Aside from using branch constructs or hints that help the predictor
> guess the right way... I think gcc will move unlikely paths right past
> the end of the "likely" fastpath, so it can increase code size and be
> somewhat suboptimal in terms of icache usage.
Thanks for the reminder. GCC definitely does code movement.
ISTR the code movement might even be "extreme", once the unit-at-a-time
support arrived, placing "cold" code at the end of the compiled module.
Jeff