1:19 a.m.
Hello Dave,
This patchset is used to fix some bug and modify display of some commands.
When some cpus are removed, the related data is not reasonable to be displayed,
so hide them.
Please check the patches to see detailed information.
--
Regards
Qiao Nuohan
2:06 p.m.
----- Original Message -----
Hello Qiao,
I am going to NAK this patch-set in its current format for several reasons.
It is much too large and affects way too many different/disparate
commands, and in many cases I do not believe that the display
of the offlined cpu's data is unreasonable.
I also don't like the idea of making so many changes to the behavior
of so many architecture-neutral commands -- and then restricting it
to x86_64 only.
Many of the changes reflect the contents of per-cpu data structures
of offlined cpus, but even though the cpu is currently offline, the
data structures still exist. Why prevent the user from viewing their
contents?
A few examples of cases that I don't agree should be modified:
(1) "ptov offset:cpuspec": I see no reason to disallow/hide the
translation of such an argument. Why is a problem?
(2) "kmem -o": Again, I *want* to see these values, and see
no reason to hide them.
(3) "struct" with an "offset:cpuspec" address. Perhaps the user
actually wants to see what the structure contained when the
cpu was offlined. What possible harm would there be in displaying
the structure?
(4) "p" with an "symbol:cpuspec" address. Same as
"struct".
(5) "help -m": this is debug output of machine-dependent data, and
should absolutely not be hidden/restricted.
(6) I don't like the idea of *not* storing the task information of
the idle tasks in the "refresh_task_table_xxx()" functions.
and so on...
The crash utility is a debugger, and I don't feel that we should
be removing stuff that could possibly be something that a user
actually wants to see or could conceivably be useful. Only if the
offline data actually causes the command to fail or display bogus
data, then yes, then it should be addressed.
About the only one of your patches that does seem to make sense
would be "help -r"; AFAICT, all of the others are open to discussion
and would at least need further justification.
Also, when your patch "hides" the data, it quietly skips the output
and goes on to the next cpu. Wouldn't it at least make more sense
to display "CPU x: [OFFLINE]" or something to that effect (depending
upon the the command's output format)?
So let me suggest that you repost the patch -- in a format that
contains just the following:
(1) the new interface
(2) any commands or command options that actually fail or show
incorrect/invalid data.
With respect to (2), please show examples of before-and-after,
showing the bogus output in the "before", and the proposed display
in the "after". And please indicate something in the "after" display
that shows the cpu number and the fact that it is offline.
Then -- after the new interface is accepted -- feel free to then post
individual command/command-option patches if you really believe the
offline data should not be displayed, and we can argue the relevancy or
whether it is reasonable to show/hide the data. But I can't just
make such wholesale changes in one fell swoop as this patchset
attempts to do.
Thanks,
Dave
3:25 a.m.
Hello Dave,
On 09/09/2014 03:06 AM, Dave Anderson wrote:
I can only take x86_64 into consideration, for I am not clear about
other architecture's cpu hot remove. I put the architecture check in
the new API, check_offline_cpu, we can involve other architecture that
supports cpu hot remove.
I think just showing online cpu's data is reasonable. What about adding
a internal crash variables(used by command set) to hide/show offline
cpu's data?
For the patchset involves too many fixes, I will firstly send one/two
fix to see whether the change is good.
--
Regards
Qiao Nuohan
9:04 a.m.
----- Original Message -----
From the perspective of the crash utility, a cpu is either online or
offline,
right? What would be the architecture-specific issue?
Why? Give me an example as to when it is/was a problem?
What about adding a internal crash variables (used by command set) to
hide/show offline cpu's data?
I suppose that could be done, but again, in my opinion there is no compelling
reason to do so. I could be wrong, but aside from maybe "help -r", it seems
that you are trying to answer a question that nobody's asking.
Dave
7:49 a.m.
Hello Dave，
On 09/09/2014 10:04 PM, Dave Anderson wrote:
I know it is important to show data of offline cpu, like debugging hot remove.
But for those who don't care about the removed cpu, hiding offline cpu will be
more clear. Then let me talk about the reason why I think hiding will be more
clear.
I first got a vmcore with 90 cpus at first and 30 of them were physically
removed. After 30 cpus physically removed, the machine works with 60 cpus.
To those who don't care about data of the removed cpu, the following data
is confusing:
1. The machine only got 60 cpus, but crash shows 90 cpus.
2. when I execute command timer, crash show 90 TVEC_BASES, some of them(maybe
exceed 30) are empty. But I have to check which cpu is offline, and then I
can know whether the empty is because of offline cpu or just no timer was set
on that cpu.
3. comes to idle tasks, offline cpu is halt and related idle tasks will not
work, but crash shows they are running right now.
4. ...
After I check kernel, I found when cpu is set to offline, things, processes,
timers, interrupts etc., are migrated to a new cpu. So I tried to hide when
cpu is set offline(logically removed) instead of physically removed.
The attachment is what I am trying to implement. If you don't like it, we can
go on discussing it.
--
Regards
Qiao Nuohan
2:57 p.m.
----- Original Message -----
The attachment is what I am trying to implement. If you don't like it, we can
go on discussing it.
Hello Qiao,
This patch-set could use some clarification. In fact, I was at
first confused as to the meaning of flag, and thought it was
"backwards". Then I realized that you are hiding the offline
cpus data by default, and the user would have to turn it back
on to show the data.
First, as I indicated in our original discussion, I do not want
to make such a change in traditional crash behavior by default.
The decision to hide an offline cpu's data should be up to the user.
Second, back to semantics, the environment variable name and its
arguments should be clarified to make it more obvious as to what
they actually mean. The "[on|off]" is confusing when combined
with "offline_cpu". Let me suggest something like:
$ crash --offline [show|hide] (where "show" is the default)
or during runtime:
crash> set offline [show|hide]
And then with respect to setting its internal state:
(pc->flags2 & OFFLINE_HIDE): means that "hide" mode is turned on.
Third, your sample construct is kind of hard to understand, in fact
I thought the logic was backwards at first:
next_cpu:
+ if (!(pc->flags2 & OFFLINE_CPU) && check_offline_cpu(cpu)) {
+ if (++cpu < kt->cpus)
+ goto next_cpu;
+ }
+
Since the check_offline_cpu() is only going to be used to determine
whether something is going to be shown or hidden, perhaps you could
create a more meaningful function name that could be used like this:
next_cpu:
if (hide_offline_cpu(cpu)) {
if (++cpu < kt->cpus)
goto next_cpu;
}
And lastly, when the offline cpu data is to be hidden, I still feel
that there should be *some* kind of indication that the cpu is offline.
In your example, instead of just skipping the TVEC_BASES[cpu] line,
why not indicate why it's missing? So in your example, show something
like this:
TVEC_BASES[3]: [OFFLINE]
I think these are all reasonable compromises.
And also, don't forget crash.8, which should basically show the
same output as "crash -h".
Thanks,
Dave
3875
days inactive
3893
days old
devel@lists.crash-utility.osci.io
8 comments
2 participants
participants (2)
-
Dave Anderson -
qiaonuohan