linux-kernel - Re: [PATCH v3 1/2] kstats: kernel metric collector

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: <6bda45c24afb5bbd069af6f57b07880cd2f5af52.camel@redhat.com>
Date:   Wed, 26 Feb 2020 15:48:49 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Luigi Rizzo <lrizzo@...gle.com>, linux-kernel@...r.kernel.org,
        mhiramat@...nel.org, akpm@...ux-foundation.org,
        gregkh@...uxfoundation.org, naveen.n.rao@...ux.ibm.com,
        ardb@...nel.org, rizzo@....unipi.it, giuseppe.lettieri@...pi.it,
        toke@...hat.com, hawk@...nel.org, mingo@...hat.com,
        acme@...nel.org, rostedt@...dmis.org, peterz@...radead.org
Subject: Re: [PATCH v3 1/2] kstats: kernel metric collector

Hi,

I'm sorry for the lack of feedback on earlier iteration, I've simply
been too slow.

On Wed, 2020-02-26 at 05:50 -0800, Luigi Rizzo wrote:
[...]
> +static void ks_print(struct seq_file *p, int slot, int cpu, u64 sum,
> +		     u64 tot, unsigned long samples, unsigned long avg)
> +{
> +	unsigned long frac = (tot == 0) ? 0 : ((sum % tot) * 1000000) / tot;

I think/fear kbuildbot will still trigger some build issues on 32bit
arches on the above line.

I think div64_u64_rem() should be used in place of '%' for and 
div64_u64() in place of '/' when operating on 64bits integers. 

There are a few more occurences below.

[...]
> ++	/* preempt_disable makes sure samples and sum modify the same slot.
> +	 * this_cpu_add() uses a non-interruptible add to protect against
> +	 * hardware interrupts which may call kstats_record.
> +	 */
> +	preempt_disable();
> +	this_cpu_add(ks->slots[slot].samples, 1);

I think 'this_cpu_inc()' could be used here.

> +	this_cpu_add(ks->slots[slot].sum,
> +		     bucket < SUM_SCALE ? val : (val >> (bucket - SUM_SCALE)));
> +	preempt_enable();
> +}
> +EXPORT_SYMBOL(kstats_record);

Thanks for sharing, this infra will be likely quite useful to me :)

Cheers,

Paolo