Re: aio is unlikely

Re: aio is unlikely

[Jeff Garzik]

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 <akpm@osdl.org>
> AuthorDate: Wed May 9 02:34:58 2007 -0700
> Committer:  Linus Torvalds <torvalds@woody.linux-foundation.org>
> 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 <suparna@in.ibm.com>
>     Cc: Ingo Molnar <mingo@elte.hu>
>     Cc: Benjamin LaHaise <bcrl@kvack.org>
>     Cc: Zach Brown <zach.brown@oracle.com>
>     Cc: Ulrich Drepper <drepper@redhat.com>
>     Cc: Christoph Hellwig <hch@lst.de>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> ---
>  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

Re: aio is unlikely

[Andrew Morton]

On Wed, 09 May 2007 18:06:58 -0400
Jeff Garzik <jeff@garzik.org> 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.
	

Re: aio is unlikely

[Jeff Garzik]

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

RE: aio is unlikely

[Alex Volkov]

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 <jeff@garzik.org> 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.

Re: aio is unlikely

[Andrew Morton]

On Fri, 18 May 2007 16:49:49 -0400
"Alex Volkov" <avcp-lkmail@usa.net> 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 <jeff@garzik.org> 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.

Re: aio is unlikely

[Jeff Garzik]

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

Re: aio is unlikely

[Phillip Susi]

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.

Re: aio is unlikely

[Andrew Morton]

On Fri, 18 May 2007 17:54:32 -0400
Phillip Susi <psusi@cfl.rr.com> 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.

Re: aio is unlikely

[Jeff Garzik]

Andrew Morton wrote:
> On Fri, 18 May 2007 17:54:32 -0400
> Phillip Susi <psusi@cfl.rr.com> 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

Re: aio is unlikely

[Nick Piggin]

Jeff Garzik wrote:
> Andrew Morton wrote:
> 
>> On Fri, 18 May 2007 17:54:32 -0400
>> Phillip Susi <psusi@cfl.rr.com> 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.

Re: aio is unlikely

[Jeff Garzik]

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

Re: aio is unlikely

[Bernd Eckenfels]

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