* Re: change '%' to condition
2013-09-11 23:18 ` change '%' to condition Mikulas Patocka
@ 2013-09-11 23:37 ` Mike Snitzer
2013-09-12 5:39 ` NeilBrown
1 sibling, 0 replies; 3+ messages in thread
From: Mike Snitzer @ 2013-09-11 23:37 UTC (permalink / raw)
To: Mikulas Patocka
Cc: linux-raid, NeilBrown, Jes Sorensen, Jonathan Brassow,
rhkernel-list
On Wed, Sep 11 2013 at 7:18pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:
> Hi
>
> I think you should revert the patch
> 4c0ca26bd260dddf3b9781758cb5e2df3f74d4a3 that did this change:
>
> for (f = 1; f < geo->far_copies; f++) {
> d += geo->near_copies;
> - if (d >= geo->raid_disks)
> - d -= geo->raid_disks;
> + d %= geo->raid_disks;
> s += geo->stride;
> r10bio->devs[slot].devnum = d;
> r10bio->devs[slot].addr = s;
>
>
> On most processors, the divide and modulo operations are slower than a
> possibly misprediteced branch or conditional move instruction.
>
> So, replacing a condition with modulo doesn't make sense.
>
> A benchmark on AMD K10 shows that mispredicted branch is 8.7 times faster
> than a divide operation:
>
> for (i = 0; i < 10000000; i++) {
> q++;
> if (q >= 8)
> q -= 8;
> }
> - 11607us (it compiles to cmov and runs at a rate of 3 ticks per
> iteration)
>
> for (i = 0; i < 10000000; i++) {
> q++;
> q %= max;
> }
> - 101241us (26 ticks per iteration)
Patch 68 in this series goes on to introduce more use of %= (upstream
commit 9a3152ab024867100f2f50d124b998d05fb1c3f6).
But regardless Jes' original point still stands: if this should be
changed it needs to be done upstream first.
Mike
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: change '%' to condition
2013-09-11 23:18 ` change '%' to condition Mikulas Patocka
2013-09-11 23:37 ` Mike Snitzer
@ 2013-09-12 5:39 ` NeilBrown
1 sibling, 0 replies; 3+ messages in thread
From: NeilBrown @ 2013-09-12 5:39 UTC (permalink / raw)
To: Mikulas Patocka; +Cc: linux-raid, Jes Sorensen, Jonathan Brassow, rhkernel-list
[-- Attachment #1: Type: text/plain, Size: 1640 bytes --]
On Wed, 11 Sep 2013 19:18:57 -0400 (EDT) Mikulas Patocka
<mpatocka@redhat.com> wrote:
> Hi
>
> I think you should revert the patch
> 4c0ca26bd260dddf3b9781758cb5e2df3f74d4a3 that did this change:
>
> for (f = 1; f < geo->far_copies; f++) {
> d += geo->near_copies;
> - if (d >= geo->raid_disks)
> - d -= geo->raid_disks;
> + d %= geo->raid_disks;
> s += geo->stride;
> r10bio->devs[slot].devnum = d;
> r10bio->devs[slot].addr = s;
>
>
> On most processors, the divide and modulo operations are slower than a
> possibly misprediteced branch or conditional move instruction.
>
> So, replacing a condition with modulo doesn't make sense.
>
> A benchmark on AMD K10 shows that mispredicted branch is 8.7 times faster
> than a divide operation:
>
> for (i = 0; i < 10000000; i++) {
> q++;
> if (q >= 8)
> q -= 8;
> }
> - 11607us (it compiles to cmov and runs at a rate of 3 ticks per
> iteration)
>
> for (i = 0; i < 10000000; i++) {
> q++;
> q %= max;
> }
> - 101241us (26 ticks per iteration)
>
>
> Mikulas
I can't seem to get excited about this sort of micro optimisation.
If someone were to send me a nice patch with a good description and a couple
of acked-by:s from relevant people I would probably apply it. But otherwise
I just wouldn't care.
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread