Linux-Fsdevel Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Jonathan Adams <jwadams@google.com>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Cc: netdev@vger.kernel.org, kvm@vger.kernel.org,
	Paolo Bonzini <pbonzini@redhat.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Jim Mattson <jmattson@google.com>,
	David Rientjes <rientjes@google.com>,
	Jonathan Adams <jwadams@google.com>
Subject: Re: [RFC PATCH 6/7] core/metricfs: expose x86-specific irq information through metricfs
Date: Thu, 13 Aug 2020 12:11:35 +0200	[thread overview]
Message-ID: <87mu2yluso.fsf@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20200807212916.2883031-7-jwadams@google.com>

Jonathan Adams <jwadams@google.com> writes:

How is that related to core? The x86 subsys prefix is 'x86' and for this
particular thing it's 'x86/irq:'. That applies to the rest of the series
as well. 

> Add metricfs support for displaying percpu irq counters for x86.
> The top directory is /sys/kernel/debug/metricfs/irq_x86.
> Then there is a subdirectory for each x86-specific irq counter.
> For example:
>
>    cat /sys/kernel/debug/metricfs/irq_x86/TLB/values

What is 'TLB'? I'm not aware of any vector which is named TLB.

The changelog is pretty useless in providing any form of rationale for
this. It tells the WHAT, but not the WHY.

Also what is does this file contain? Aggregates, one line per CPU or the
value of the random CPU of the day? I'm not going to dive into the macro
zoo to figure that out.

> jwadams@google.com: rebased to 5.8-pre6
> 	This is work originally done by another engineer at
> 	google, who would rather not have their name associated with
> 	this patchset. They're okay with me sending it under my name.

I can understand why they wont have their name associated with this.

> +#ifdef CONFIG_METRICFS
> +METRICFS_ITEM(NMI, __nmi_count, "Non-maskable interrupts");
> +#ifdef CONFIG_X86_LOCAL_APIC
> +METRICFS_ITEM(LOC, apic_timer_irqs, "Local timer interrupts");
> +METRICFS_ITEM(SPU, irq_spurious_count, "Spurious interrupts");
> +METRICFS_ITEM(PMI, apic_perf_irqs, "Performance monitoring interrupts");
> +METRICFS_ITEM(IWI, apic_irq_work_irqs, "IRQ work interrupts");
> +METRICFS_ITEM(RTR, icr_read_retry_count, "APIC ICR read retries");
> +#endif
....

So you are adding NR_CPUS * NR_DIRECT_VECTORS debugfs files which show
exactly the same information as /proc/interrupts, right? 

Aside of that _all_ of this information is available via tracepoints as
well.

That's NR_CPUS * 15 and incomplete because x86 has 23 of those directly
handled vectors which do not go through the irq core. So with just 15
and 256 CPUs that's 3840 files.

Impressive number especially without any information why this is useful
and provides value over the existing mechanisms to retrieve exactly the
same information.

The cover letter talks a lot about who Google finds this useful, but
that's not really a convincing argument for this metric failsystem
addon.

Thanks,

        tglx




  reply	other threads:[~2020-08-13 10:11 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-07 21:29 [RFC PATCH 0/7] metricfs metric file system and examples Jonathan Adams
2020-08-07 21:29 ` [RFC PATCH 1/7] core/metricfs: Create metricfs, standardized files under debugfs Jonathan Adams
2020-08-08  5:41   ` Greg KH
2020-08-07 21:29 ` [RFC PATCH 2/7] core/metricfs: add support for percpu metricfs files Jonathan Adams
2020-08-08  5:43   ` Greg KH
2020-08-07 21:29 ` [RFC PATCH 3/7] core/metricfs: metric for kernel warnings Jonathan Adams
2020-08-08  5:45   ` Greg KH
2020-08-07 21:29 ` [RFC PATCH 4/7] core/metricfs: expose softirq information through metricfs Jonathan Adams
2020-08-08  5:46   ` Greg KH
2020-08-07 21:29 ` [RFC PATCH 5/7] core/metricfs: expose scheduler stat " Jonathan Adams
2020-08-07 21:29 ` [RFC PATCH 6/7] core/metricfs: expose x86-specific irq " Jonathan Adams
2020-08-13 10:11   ` Thomas Gleixner [this message]
2020-08-13 11:47     ` Paolo Bonzini
2020-08-13 12:13       ` Thomas Gleixner
2020-08-13 14:10         ` Paolo Bonzini
2020-08-13 14:21           ` Thomas Gleixner
2020-08-07 21:29 ` [RFC PATCH 7/7] net-metricfs: Export /proc/net/dev via metricfs Jonathan Adams
2020-08-08  2:06 ` [RFC PATCH 0/7] metricfs metric file system and examples Andrew Lunn
2020-08-08 15:59   ` David Ahern
2020-08-10 18:20     ` Jakub Kicinski
2020-08-10  9:23 ` Pavel Machek

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87mu2yluso.fsf@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=gregkh@linuxfoundation.org \
    --cc=jmattson@google.com \
    --cc=jwadams@google.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=rientjes@google.com \
    --subject='Re: [RFC PATCH 6/7] core/metricfs: expose x86-specific irq information through metricfs' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).