Re: [PATCH 08/28] kdb: core for kgdb back end (2 of 2)

[Jason Wessel <jason.wessel@windriver.com>]

Eric W. Biederman wrote:
> Jason Wessel <jason.wessel@windriver.com> writes:
>
>   
>> Eric W. Biederman wrote:
>>     
>>> Jason Wessel <jason.wessel@windriver.com> writes:
>>>
>>>   
>>>       
>>>> This patch contains the hooks and instrumentation into kernel which
>>>> live outside the kernel/debug directory, which the kdb core
>>>> will call to run commands like lsmod, dmesg, bt etc...
>>>>     
>>>>         
>>> You know this dropping the locks from vmalloc_info and swap_info
>>> is down right ugly, and I don't believe it is safe.  That code
>>> was not designed to run while the write_lock is held.
>>>   
>>>       
>> Perhaps we can find some middle ground.  I don't mind simply not
>> allowing the information to be queried from kdb if the locks are not
>> available. 
>>
>> Which is less ugly you, making the swap_lock global or adding a function
>> to query it?
>>     
>
> My recommendation would be to simply drop the swap_info and meminfo
> information for now, and have kdb do what is good at.
>   

I have no objection to dropping this for now.

> Skimming through the history of the discussion it appears Christoph
> Hellwig asked you to do something about these bits on the first
> review.
>
>   
If you are referring to the statement from the RFC patch:

"Patch 1 still containes a lot random junk. E.g. all those meminfo and
whatever hooks aren't required for a very basic kernel debugger"

Most of that was restructured in that code base vs what you have taken a
look at and we are down to a handful of instrumentation points.

> From a maintenance point of view anything where you have to know which
> locks a function will take is fragile.  It is entirely too easy to create
> a change that is fine in it's normal context but breaks kdb.
>
>   

Sure, but that is life for kdb in the current state.  You either make
safe query macros or create something to call helper functions to
inspect code.  Either way it is maintenance.

> Alt-sysrq already allows capturing that kind of information, without any
> thorny maintenance issues.  

Alt-sysrq cannot be called from all the same contexts that you can
invoke kdb.  The maintenance issue you speak of is avoided because of
the points you can actually invoke a sysrq.  Certainly you are not going
to be able to call Alt-sysrq, in an nmi as well as a number of other
contexts.

> By contrast kdb appears to be a much larger
> and inferior tool.
>
>   

I am not certain as to what comparison you are trying to draw here.  If
you are only using kdb to obtain meminfo and swapinfo, sure I buy your
argument.   If you are using kdb to modify, inspect memory and execute
back traces at certain places then I think this is not an apples to
apples comparison.

I'll post a new version of the patch with the hooks you asked about
stripped out.  The commands will also get removed from the kdb command
shell.

Thanks,
Jason.