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

[KOSAKI Motohiro]

> > 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/