netdev - Re: [PATCH] pkt_sched: gen_estimator: use 64 bits intermediate counters for bps

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 19 May 2009 09:31:36 +0200
From:	Eric Dumazet <dada1@...mosbay.com>
To:	Jarek Poplawski <jarkao2@...il.com>
CC:	David Miller <davem@...emloft.net>, vexwek@...il.com,
	netdev@...r.kernel.org, kaber@...sh.net, devik@....cz
Subject: Re: [PATCH] pkt_sched: gen_estimator: use 64 bits intermediate	counters
 for bps

Jarek Poplawski a écrit :
> On Tue, May 19, 2009 at 01:59:55AM +0200, Eric Dumazet wrote:
> ...
>> diff --git a/net/core/gen_estimator.c b/net/core/gen_estimator.c
> ...
>> -		e->avbps += ((long)rate - (long)e->avbps) >> e->ewma_log;
>> +		e->avbps += ((s64)(brate - e->avbps)) >> e->ewma_log;
> 
> Btw., I'm a bit concerned about the syntax here: isn't such shifting
> of signed ints implementation dependant?
> 

You are right Jarek, I very often forget to never ever use signed quantities
at all ! (But also note original code has same undefined behavior)


Quoting wikipedia : (http://en.wikipedia.org/wiki/Arithmetic_shift)

The (1999) ISO standard for the, C programming language defines the C language's 
right shift operator in terms of divisions by powers of 2. Because of the 
aforementioned non-equivalence, the standard explicitly excludes from that
 definition the right shifts of signed numbers that have negative values.
 It doesn't specify the behaviour of the right shift operator in such circumstances,
 but instead requires each individual C compiler to specify the behaviour of shifting 
negative values right.

Apparently gcc does the *right* thing on x86_32, but we probably want something
stronger here. I could not find gcc documentation statement on right shifts of 
negative values.


 436:   8b 4b 14                mov    0x14(%ebx),%ecx
 439:   89 73 18                mov    %esi,0x18(%ebx)
 43c:   89 7b 1c                mov    %edi,0x1c(%ebx)
 43f:   8b 73 20                mov    0x20(%ebx),%esi
 442:   8b 7b 24                mov    0x24(%ebx),%edi
 445:   29 f0                   sub    %esi,%eax
 447:   19 fa                   sbb    %edi,%edx
 449:   0f ad d0                shrd   %cl,%edx,%eax
 44c:   d3 fa                   sar    %cl,%edx         << good >>
 44e:   f6 c1 20                test   $0x20,%cl
 451:   74 05                   je     458 <est_timer+0xb8>
 453:   89 d0                   mov    %edx,%eax
 455:   c1 fa 1f                sar    $0x1f,%edx       
 458:   01 f0                   add    %esi,%eax
 45a:   8b 4b 0c                mov    0xc(%ebx),%ecx
 45d:   89 43 20                mov    %eax,0x20(%ebx)
 460:   11 fa                   adc    %edi,%edx
 462:   83 c0 0f                add    $0xf,%eax
 465:   89 53 24                mov    %edx,0x24(%ebx)
 468:   83 d2 00                adc    $0x0,%edx
 46b:   0f ac d0 05             shrd   $0x5,%edx,%eax
 46f:   89 01                   mov    %eax,(%ecx)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html