linux-kernel - Re: [PATCH v3] Add generic exponentially weighted moving average (EWMA) function

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: <20101022100316.53A3.A69D9226@jp.fujitsu.com>
Date:	Fri, 22 Oct 2010 10:11:38 +0900 (JST)
From:	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>
To:	Bruno Randolf <br1@...fach.org>
Cc:	kosaki.motohiro@...fujitsu.com, randy.dunlap@...cle.com,
	akpm@...ux-foundation.org, kevin.granade@...il.com,
	Lars_Ericsson@...ia.com, blp@...stanford.edu,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Add generic exponentially weighted moving average (EWMA) function

> > and nit, I don't understand what you intend by 'const unsigned int'. I
> > think we can remove this const safely. it's more easy readable.
> 
> O.K. Will resend.
> 
> Apart from that, what didn't work as you expected?

It works. I meant this const has no meaning.


few additional reviewing comments is here.


> +struct ewma {
> +	unsigned int internal;
> +	unsigned int factor;
> +	unsigned int weight;
> +};

I think unsigned long is better because long is natual register size
on both 32bit and 64bit arch.
and, example, almost linux resource limit is using long or ulong. then
uint may have overflow risk if we are using this one on 64bit arch.
Does uint has any benefit? (note: scheduler loadavg has already used ulong)

> +struct ewma*
> +ewma_add(struct ewma *avg, const unsigned int val)
> +{
> +	avg->internal = avg->internal  ?
> +		(((avg->internal * (avg->weight - 1)) +
> +			(val * avg->factor)) / avg->weight) :
> +		(val * avg->factor);
> +	return avg;

Hm, if ewma_add has this function prototype, we almost always need to
typing "new = ewma_get(ewma_add(&ewma, val))". Is this intentional?
if so, why?

Why can't we simple do following?

unsigned long ewma_add(struct ewma *avg, const unsigned int val)
{
(snip)
	return ewma_get(avg);
}



--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/