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]

Message-ID: <4A12F46C.4070605@cosmosbay.com>
Date:	Tue, 19 May 2009 20:03:24 +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 07:42:47AM +0000, Jarek Poplawski wrote:
>> On Tue, May 19, 2009 at 09:31:36AM +0200, Eric Dumazet wrote:
>>> 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)
>> Sure, I've meant the original code including 5 lines below.
>>
>>> 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.
>> I guess gcc and most of others do this "right"; but it looks
>> "unkosher" anyway.
> 
> I might have missed your point here, but would it be so costly to do
> these shifts separately here?

You replied to yourself Jarek :)

As I said earlier, I found your concern right, so please submit a patch ?

I found many occurrences of a right shift on a signed int/long in kernel.
One example being :

arch/x86/mm/init_64.c

int kern_addr_valid(unsigned long addr)
{
	unsigned long above = ((long)addr) >> __VIRTUAL_MASK_SHIFT;


and another rate estimator in drivers/atm/idt77252.c

static void
idt77252_est_timer(unsigned long data)


We could aso check net/netfilter/ipvs/ip_vs_est.c (estimation_timer())

--
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