On Friday 09 June 2017 05:34 PM, Michal Suchánek wrote:
On Thu, 8 Jun 2017 23:30:37 +0530
Hari Bathini <hbath...@linux.vnet.ibm.com> wrote:
Hi Michal,
Sorry for taking this long to respond. I was working on a few other
things.
On Monday 15 May 2017 02:59 PM, Michal Suchánek wrote:
Hello,
On Mon, 15 May 2017 12:59:46 +0530
Hari Bathini <hbath...@linux.vnet.ibm.com> wrote:
On Friday 12 May 2017 09:12 PM, Michal Suchánek wrote:
On Fri, 12 May 2017 15:15:33 +0530
Hari Bathini <hbath...@linux.vnet.ibm.com> wrote:
On Thursday 11 May 2017 06:46 PM, Michal Suchánek wrote:
On Thu, 11 May 2017 02:00:11 +0530
Hari Bathini <hbath...@linux.vnet.ibm.com> wrote:
Hello Michal,
On Wednesday 10 May 2017 09:31 PM, Michal Suchánek wrote:
Hello,
On Wed, 03 May 2017 23:52:52 +0530
Hari Bathini <hbath...@linux.vnet.ibm.com> wrote:
With the introduction of 'fadump_append=' parameter to pass
additional parameters to fadump (capture) kernel, update
documentation about it.
Signed-off-by: Hari Bathini <hbath...@linux.vnet.ibm.com>
---
Changes from v4:
* Based on top of patchset that includes
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=akpm&id=05f383cdfba8793240e73f9a9fbff4e25d66003f
Documentation/powerpc/firmware-assisted-dump.txt | 10
+++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/Documentation/powerpc/firmware-assisted-dump.txt
b/Documentation/powerpc/firmware-assisted-dump.txt index
8394bc8..6327193 100644 ---
a/Documentation/powerpc/firmware-assisted-dump.txt +++
b/Documentation/powerpc/firmware-assisted-dump.txt @@ -162,7
+162,15 @@ How to enable firmware-assisted dump (fadump):
1. Set config option CONFIG_FA_DUMP=y and build kernel.
2. Boot into linux kernel with 'fadump=on' kernel
cmdline option. -3. Optionally, user can also set
'crashkernel=' kernel cmdline +3. A user can pass additional
command line parameters as a comma
+ separated list through 'fadump_append=' parameter, to be
enforced
+ when fadump is active. For example, if parameters like
nr_cpus=1,
+ numa=off & udev.children-max=2 are to be enforced when
fadump is
+ active,
'fadump_append=nr_cpus=1,numa=off,udev.children-max=2'
+ can be passed in command line, which will be replaced
with
+ "nr_cpus=1 numa=off udev.children-max=2" when fadump is
active.
+ This helps in reducing memory consumption during dump
capture. +4. Optionally, user can also set 'crashkernel='
kernel cmdline to specify size of the memory to reserve for
boot memory dump preservation.
Writing your own deficient parser for comma separated
arguments when perfectly fine parser for space separated
quoted arguments exists in the kernel and the bootloader does
not seem like a good idea to me.
Couple of things that prompted me for v5 are:
1. Using parse_early_options() limits the kind of
parameters that can be passed to fadump capture kernel. Passing
parameters like systemd.unit= & udev.childern.max= has no
effect with v4. Updating
boot_command_line parameter, when fadump is active,
seems a better alternative.
2. Passing space-separated quoted arguments is not
working as intended with lilo. Updating bootloader with the
below entry in /etc/lilo.conf file results in a missing append
entry in /etc/yaboot.conf file.
append = "quiet sysrq=1 insmod=sym53c8xx insmod=ipr
crashkernel=512M-:256M fadump_append=\"nr_cpus=1 numa=off
udev.children-max=2\""
Meaning that a script that emulates LILO semantics on top of
yaboot which is completely capable of passing qouted space
separated arguments fails. IMHO it is more reasonable to fix the
script or whatever adaptation layer or use yaboot directly than
working around bug in said script by introducing a new argument
parser in the kernel.
Hmmm.. while trying to implement space-separated parameter list
with quotes as syntax for fadump_append parameter, noticed that
it can make implemenation
more vulnerable. Here are some problems I am facing while
implementing this..
How so?
presumably you can reuse parse_args even if you do not register
with early_param and call it yourself. Then your parsing of
fadump_append is
I wasn't aware of that. Thanks for pointing it out, Michal.
Will try to use parse_args and get back.
I was thinking a bit more about the uses of the commandline and how
fadump_append potentially breaks it.
The first thing that should be addressed and is the special --
argument which denotes the start of init arguments that are not to
be parsed by the kernel. Incidentally the initial implementation
using early_param happened to handles that without issue.
parse_args surely handles that so adding a hook somewhere should
give you location of that argument (if any). And interesting thing
that can happen is passing an -- inside the fadump_append argument.
It should be handled (or not) in some way or other and the handling
documented.
The intention with this patch is to replace
"root=/dev/sda2 ro fadump_append=nr_cpus=1,numa=off
crashkernel=1024M"
with
"root=/dev/sda2 ro nr_cpus=1 numa=off crashkernel=1024M"
when kernel is booting for dump capture.
While parse_args() is making parsing relatively easy, the main idea is
to replace parameters as above when fadump capture kernel is booting.
The code is relatively complicated to replace parameters using
space-separated
quoted string even though parse_args() may parse them easily.
Comma-separated
list was easier to implement and seemed less error prone for
replacing parameters.
You can still do that relatively easily. parse_args() gives you the
content of fadump_capture argument. You should probably add a variant
that gives you also the position of the fadump_capture argument in case
user specified it multiple times - picking the wrong one would cause
errors.
There is no denying that this can be done with the use of parse_args()
function.
But my contention is a custom parser is no more error prone, considering it
only has to search/replace commas in fadump_extra_args= till the first
occurrence of space/nul, without having to deal with the naunces of
parse_args() function. Also, to get the last occurence, could use something
like get_last_crashkernel() instead of adding a variant to parse_args().
Then you can just replace the fadump_append="appended arguments" with
the dequoted "appended arguments" parse_args() gives you. Dequating
should shorten the arguments as would removing the fadump_append=
prefix so it should be quite possible in-place. It is in fact even
easier than the comma separated list since you do not need to do any
parsing yourself - only strlen, memset and memcpy.
That said, if you replace the fadump_append argument the running kernel
will have extra arguments without knowing where they came from making
detection of this happening all the more difficult.
How about replacing "fadump_extra_args=x,y,z" with "fadump_extra_args x
y z"?
Presence of fadump_extra_args hints the user about the extra parameters
passed
to fadump capture kernel and also lets parsing/replacing be simple - replace
the first '=' and subsequent commas with ' ' in this fadump_extra_args=
parameter
and we are good to go. No additional handling. Even if we go with
space-separated
quoted string, simply replacing "fadump_extra_args=" with
"fadump_extra_args "
should suffice, no need to worry about parse_args as well. I prefer
comma-separated
list though, for it leaves no dependency on how bootloader handles
space-separated
quoted strings, considering my encounter with yast bootloader which is
not so
impressive in handling quoted strings. Also, the code change is not so
complicated.
Want to replace fadump_append=, to avoid command line overflow issues
and also to not have to deal with special cases like --. Actually,
fadump_append=
seems to be confusing in that sense. How about
fadump_args=comma-separated-list-
of-params-for-capture-kernel ? Please share your thoughts.
The comma separated list still can contain a -- argument so you still
have to do something about that. Or not and just document that people
should not use it.
The user has the flexibility to use fadump_extra_args= to pass special
parameters
keeping in mind that "fadump_extra_args=x,y,z" will be replaced in-place
with
"fadump_extra_args x y z" and it happens only in fadump capture kernel.
No additional
code changes needed to handle this except for documentation talking
about it?
As for the parameter name fadump_args or fadump_extra_args is more
generic than fadump_append giving you more room for changing the
implementation details without making the argument name non-sensical.
The second thing that breaks is reusing content of /proc/cmdline in
kexec calls for passing arguments to the loaded kernel. It works
flawlessly passing the same arguments the currently running kernel
was started with unless fadump_append argument handler appends
content of the fadump_append argument to actual commandline that
appears there. So this would be an argument against modifying the
commandline. You could argue using fadump and kexec together is an
exotic use case but I would expect that the very machines that
require fadump_append to reduce dump memory requirement benefit
from using kexec for reboots because the BIOS probing the hardware
takes quite a while as well. If rewriting the commandline is
desired then this side effect of recursively adding the content of
fadump_append on kexecs which reuse /proc/cmdline should be
documented.
fadump capture kernel (the kernel where we expand
"fadump_append=x,y,z" to "x y z") only advised to be used for saving
dump (just like kdump kernel).
We are expected to boot into production kernel immediately after
saving dump.
Which you can do for example using kexec.
Only in special cases where a user configures to stay in fadump
capture kernel
will he encounter the above problem. And someone choosing such
scenario is most
likely aware of what to make of /proc/cmdline?
Why? They leave that to the initscripts and since you just changed the
semantics of the kernel parameters significantly those can produce
entertaining results.
Yeah. That is tricky but we are talking about a capture kernel here
which should
typically reboot after dump capture (just like kdump kernel does). So,
the case
you are mentioning about can be considered an exception a script should
be able
to tackle?
Thanks
Hari
