Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <6bda45c24afb5bbd069af6f57b07880cd2f5af52.camel@redhat.com>
Subject: Re: [PATCH v3 1/2] kstats: kernel metric collector
From:   Paolo Abeni <pabeni@redhat.com>
To:     Luigi Rizzo <lrizzo@google.com>, linux-kernel@vger.kernel.org,
        mhiramat@kernel.org, akpm@linux-foundation.org,
        gregkh@linuxfoundation.org, naveen.n.rao@linux.ibm.com,
        ardb@kernel.org, rizzo@iet.unipi.it, giuseppe.lettieri@unipi.it,
        toke@redhat.com, hawk@kernel.org, mingo@redhat.com,
        acme@kernel.org, rostedt@goodmis.org, peterz@infradead.org
Date:   Wed, 26 Feb 2020 15:48:49 +0100
In-Reply-To: <20200226135027.34538-2-lrizzo@google.com>
References: <20200226135027.34538-1-lrizzo@google.com>
         <20200226135027.34538-2-lrizzo@google.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.32.5 (3.32.5-1.fc30) 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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