From: "Enrico Weigelt, metux IT consult" <lkml@metux.net>
To: Thomas Gleixner <tglx@linutronix.de>,
Aubrey Li <aubrey.li@linux.intel.com>
Cc: mingo@redhat.com, peterz@infradead.org, hpa@zytor.com,
ak@linux.intel.com, tim.c.chen@linux.intel.com,
dave.hansen@intel.com, arjan@linux.intel.com,
adobriyan@gmail.com, akpm@linux-foundation.org,
aubrey.li@intel.com, linux-api@vger.kernel.org,
linux-kernel@vger.kernel.org, Andy Lutomirski <luto@kernel.org>
Subject: Re: [PATCH v17 1/3] proc: add /proc/<pid>/arch_status
Date: Thu, 25 Apr 2019 12:40:54 +0200 [thread overview]
Message-ID: <d8395c0e-7486-7c84-2c1c-de0929ec27b7@metux.net> (raw)
In-Reply-To: <alpine.DEB.2.21.1904242310040.1762@nanos.tec.linutronix.de>
On 24.04.19 23:18, Thomas Gleixner wrote:
Hi,
>> +config PROC_PID_ARCH_STATUS
>> + bool "Enable /proc/<pid>/arch_status file"
>
> Why is this switchable? x86 selects it if PROC_FS is enabled and all other
> architectures are absolutely not interested in this.
IMHO, it's good to have a switch, but that way doesn't make much sense.
Instead, I'd do it the other way round: make that switch depending on
those archs that actually support it. Something like this:
config PROC_PID_ARCH_STATUS
bool "Enable /proc/<pid>/arch_status file"
depends on PROC_FS
depends on BROKEN
When x86 comes in, it would change to:
config PROC_PID_ARCH_STATUS
bool "Enable /proc/<pid>/arch_status file"
depends on PROC_FS
depends on X86
And later arm coming in:
config PROC_PID_ARCH_STATUS
bool "Enable /proc/<pid>/arch_status file"
depends on PROC_FS
depends on X86 || ARM
>> + default n
>> + help
>> + Provides a way to examine process architecture specific information.
>> + See <file:Documentation/filesystems/proc.txt> for more information.
>
> Which contains zero information about this file when only this patch is
> applied.
hmm, the patch alone doesn't do anything useful anyway. it only becomes
useful with subsequent patches that add some arch. I wonder if there's
anything more useful to document at that point.
>> +/*
>> + * Add support for task architecture specific output in /proc/pid/arch_status.
>> + * task_arch_status() must be defined in asm/processor.h
>> + */
>> +#ifdef CONFIG_PROC_PID_ARCH_STATUS
>> +# ifndef task_arch_status
>> +# define task_arch_status(m, task)
>> +# endif
>
> What exactly is the point of this macro mess? If an architecture selects
> CONFIG_PROC_PID_ARCH_STATUS then it has to provide proc_task_arch_status()
> and the prototype should be in include/linux/proc_fs.h.
ACK.
>> +static int proc_pid_arch_status(struct seq_file *m, struct pid_namespace *ns,
>> + struct pid *pid, struct task_struct *task)
>> +{
>> + task_arch_status(m, task);
>> + return 0;
>> +}
Is that wrapper really neeeded ?
--mtx
--
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287
prev parent reply other threads:[~2019-04-25 10:41 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-21 18:35 [PATCH v17 1/3] proc: add /proc/<pid>/arch_status Aubrey Li
2019-04-21 18:35 ` [PATCH v17 2/3] /proc/pid/arch_status: Add AVX-512 usage elapsed time Aubrey Li
2019-04-24 21:19 ` Thomas Gleixner
2019-04-21 18:35 ` [PATCH v17 3/3] Documentation/filesystems/proc.txt: add arch_status file Aubrey Li
2019-04-24 21:27 ` Thomas Gleixner
2019-04-24 21:18 ` [PATCH v17 1/3] proc: add /proc/<pid>/arch_status Thomas Gleixner
2019-04-25 1:50 ` Li, Aubrey
2019-04-25 7:20 ` Thomas Gleixner
2019-04-25 8:12 ` Li, Aubrey
2019-04-25 8:20 ` Thomas Gleixner
2019-04-25 8:24 ` Li, Aubrey
2019-04-25 10:11 ` Enrico Weigelt, metux IT consult
2019-04-25 10:42 ` Li, Aubrey
2019-04-25 10:46 ` Enrico Weigelt, metux IT consult
2019-04-25 10:50 ` Thomas Gleixner
2019-04-26 11:20 ` Enrico Weigelt, metux IT consult
2019-04-26 13:38 ` Thomas Gleixner
2019-04-25 10:40 ` Enrico Weigelt, metux IT consult [this message]
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=d8395c0e-7486-7c84-2c1c-de0929ec27b7@metux.net \
--to=lkml@metux.net \
--cc=adobriyan@gmail.com \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=arjan@linux.intel.com \
--cc=aubrey.li@intel.com \
--cc=aubrey.li@linux.intel.com \
--cc=dave.hansen@intel.com \
--cc=hpa@zytor.com \
--cc=linux-api@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luto@kernel.org \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=tim.c.chen@linux.intel.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).