Bug 193873 - [PATCH] Unify dumpsys() under generic kern_dump.c.
Summary: [PATCH] Unify dumpsys() under generic kern_dump.c.
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Mark Johnston
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-09-23 13:26 UTC by Conrad Meyer
Modified: 2015-01-07 01:58 UTC (History)
6 users (show)

See Also:


Attachments
(Applies with patch -p1 to CURRENT as of r272027) (80.89 KB, patch)
2014-09-23 13:26 UTC, Conrad Meyer
no flags Details | Diff
v2 of the patch w/ feedback incorporated (90.85 KB, patch)
2014-10-01 03:40 UTC, Conrad Meyer
no flags Details | Diff
(Just the changes between v1 and v2.) (36.65 KB, patch)
2014-10-01 03:41 UTC, Conrad Meyer
no flags Details | Diff
v2 of the patch rebased on top of r273035 (past conflicting r272766) (90.92 KB, patch)
2014-10-13 14:12 UTC, Conrad Meyer
no flags Details | Diff
v3 of patch. Addresses Andrew's ARM change and feedback from Mark in Differential. (585.00 KB, patch)
2014-11-12 00:45 UTC, Conrad Meyer
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Conrad Meyer 2014-09-23 13:26:11 UTC
Created attachment 147599 [details]
(Applies with patch -p1 to CURRENT as of r272027)

(To varying degrees by arch.)

x86, ARM, and MIPS are all relatively similar and straightforward. Some
MD-specific methods are left in dump_machdep.c in each arch to provide
mach-dependent implementations. (Map a region temporarily for dumping,
unmap a region, iterate physical memory segments, flush WB caches.)
   
Sparc and PowerPC are weirder. PowerPC had a merged dump/minidump path
that used a different md_pa structure, pmap_md, plumbed through its MMU
interface. So, that was ripped out and replaced with the standard path.

Sparc uses its own non-ELF dumping header and that makes its dumpsys
different enough that unification wasn't an improvement. However, some
logic shared with other archs (blk_dump == cb_dumpdata) was refactored
away.

Patch build-tested against:
  - ARMv6 / CHROMEBOOK
  - AMD64 / GENERIC
  - i386 / GENERIC
  - MIPS / WZR-300HP
  - MIPS64 / SWARM64_SMP
  - PPC / MPC85XX (cpu=booke)
  - PPC / GENERIC (cpu=aim)
  - PPC64 / GENERIC64 (cpu=aim64)
  - Sparc64 / GENERIC

Sponsored by:   EMC / Isilon storage division

Notes:
  * This patch is the first step towards some other dump/minidump improvements. I didn't want to apply them independently to 7-8 different architectures' forks of dumpsys.
  * Patch applies cleanly against a git tree; some hunks include SVN $FreeBSD$ macros which may require some coddling to apply to an SVN tree.
  * powerpc/dump_machdep.c is entirely deleted (something patch(1) doesn't really do well)
  * Net 875 lines removed: 21 files changed, 820 insertions(+), 1697 deletions(-)

 sys/amd64/include/md_var.h          |   5 +
 sys/arm/arm/dump_machdep.c          | 325 +----------------------------
 sys/arm/include/md_var.h            |   7 +
 sys/conf/files                      |   1 +
 sys/conf/files.powerpc              |   1 -
 sys/i386/include/md_var.h           |   5 +
 sys/kern/kern_dump.c                | 402 ++++++++++++++++++++++++++++++++++++
 sys/mips/include/md_var.h           |   8 +
 sys/mips/mips/dump_machdep.c        | 318 +---------------------------
 sys/powerpc/aim/mmu_oea.c           | 167 +++++++--------
 sys/powerpc/aim/mmu_oea64.c         | 164 +++++++--------
 sys/powerpc/booke/pmap.c            | 228 ++++++++++----------
 sys/powerpc/include/md_var.h        |  10 +
 sys/powerpc/include/pmap.h          |  13 +-
 sys/powerpc/powerpc/dump_machdep.c  | 315 ----------------------------
 sys/powerpc/powerpc/mmu_if.m        |  41 ++--
 sys/powerpc/powerpc/pmap_dispatch.c |  28 ++-
 sys/sparc64/include/md_var.h        |   5 +
 sys/sparc64/sparc64/dump_machdep.c  | 115 +++--------
 sys/sys/conf.h                      |  12 ++
 sys/x86/x86/dump_machdep.c          | 347 +------------------------------
Comment 1 Justin Hibbits freebsd_committer 2014-09-29 20:50:26 UTC
Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the md headers, most of which simply call a common routine, with the oddballs doing their own thing?
Comment 2 Conrad Meyer 2014-09-29 21:56:03 UTC
(The other issue brought up on IRC is that there is no reason to initialize both minidumps and full dump headers on PPC (moea{,64}_scan_init), and that allows us to drop the MD static vdump_map.)

I will fix both of these things, the change should be small. I don't think I can get to it today, but sometime this week.
Comment 3 Conrad Meyer 2014-09-30 20:21:23 UTC
(In reply to Justin Hibbits from comment #1)
> Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the
> md headers, most of which simply call a common routine, with the oddballs
> doing their own thing?

Is this something like what you had in mind? http://dpaste.com/0QRFFAS

It's a rough sketch and I haven't tried to compile it. Just want to make sure it seems reasonable to you before going down that path. Thanks!
Comment 4 Justin Hibbits freebsd_committer 2014-09-30 20:44:41 UTC
(In reply to Conrad Meyer from comment #3)
> (In reply to Justin Hibbits from comment #1)
> > Instead of wrapping with DUMPSYS_HAS_*, why not add wrapper functions in the
> > md headers, most of which simply call a common routine, with the oddballs
> > doing their own thing?
> 
> Is this something like what you had in mind? http://dpaste.com/0QRFFAS
> 
> It's a rough sketch and I haven't tried to compile it. Just want to make
> sure it seems reasonable to you before going down that path. Thanks!

Looks reasonable to me.  Thanks for doing this work!
Comment 5 Conrad Meyer 2014-10-01 03:40:31 UTC
Created attachment 147868 [details]
v2 of the patch w/ feedback incorporated

Updated to incorporate feedback. Buildtested against the same architectures. Diff is against r272336.
Comment 6 Conrad Meyer 2014-10-01 03:41:21 UTC
Created attachment 147869 [details]
(Just the changes between v1 and v2.)

This isn't the whole patch, just the changes made after v1 per feedback.
Comment 7 Ed Maste freebsd_committer 2014-10-06 16:45:03 UTC
https://reviews.freebsd.org/D904
Comment 8 Conrad Meyer 2014-10-13 14:12:46 UTC
Created attachment 148257 [details]
v2 of the patch rebased on top of r273035 (past conflicting r272766)
Comment 9 Conrad Meyer 2014-10-27 20:09:18 UTC
Re: Mark's comments from phab:

> Could you not just these define these to do nothing on powerpc and sparc?

Yep. That removes the header #ifdef.

> Is there any reason this stuff couldn't go into sys/kerneldump.h?

I just followed the existing pattern, where doadump() etc already lived in conf.h. I'm not attached to the particular header. kerneldump.h is fine with me.
Comment 10 Andrew Turner freebsd_committer 2014-10-27 20:13:28 UTC
You will need to take into account r273284. On ARM there is now a new section to allow libkvm to translate from va to pa that needs to be added to the dump.
Comment 11 Conrad Meyer 2014-11-11 17:29:16 UTC
(In reply to Andrew Turner from comment #10)
> You will need to take into account r273284. On ARM there is now a new
> section to allow libkvm to translate from va to pa that needs to be added to
> the dump.

Updating for this now. I think the generic facility here is that some architectures may provide a routine to write out auxiliary elf headers.
Comment 12 Conrad Meyer 2014-11-12 00:45:25 UTC
Created attachment 149303 [details]
v3 of patch. Addresses Andrew's ARM change and feedback from Mark in Differential.

Andrew's ARM dump change (r273284) adds an extra ELF phdr; I've added a generic facility for archs to dump a static number of extra headers, with the ARM stuff as an example.

Several clean-ups and changes brought up in CR feedback from Mark. In particular:

* There are several fewer #ifdefs in the generic routines and headers, and a few more stubs in the MD headers.
  * This includes 'do_minidump' sysctl moving back to MD code.
* 'md_pa' has been renamed to 'dump_pa' because it is no longer MD.
* Returns in some MD stubs have been fixed to follow style(9).
* sparc64's dumpsys' kerenldumpheader is back in bss to try and avoid overflowing the kernel stack during dump.
* Several dump-internal routines have been relocated from sys/conf.h to sys/kerneldump.h.
Comment 13 commit-hook freebsd_committer 2015-01-07 01:02:19 UTC
A commit references this bug:

Author: markj
Date: Wed Jan  7 01:01:42 UTC 2015
New revision: 276772
URL: https://svnweb.freebsd.org/changeset/base/276772

Log:
  Factor out duplicated code from dumpsys() on each architecture into generic
  code in sys/kern/kern_dump.c. Most dumpsys() implementations are nearly
  identical and simply redefine a number of constants and helper subroutines;
  a generic implementation will make it easier to implement features around
  kernel core dumps. This change does not alter any minidump code and should
  have no functional impact.

  PR:		193873
  Differential Revision:	https://reviews.freebsd.org/D904
  Submitted by:	Conrad Meyer <conrad.meyer@isilon.com>
  Reviewed by:	jhibbits (earlier version)
  Sponsored by:	EMC / Isilon Storage Division

Changes:
  head/sys/amd64/include/dump.h
  head/sys/arm/arm/dump_machdep.c
  head/sys/arm/include/dump.h
  head/sys/conf/files
  head/sys/i386/include/dump.h
  head/sys/kern/kern_dump.c
  head/sys/kern/kern_shutdown.c
  head/sys/mips/include/dump.h
  head/sys/mips/include/md_var.h
  head/sys/mips/mips/dump_machdep.c
  head/sys/pc98/include/dump.h
  head/sys/powerpc/aim/mmu_oea.c
  head/sys/powerpc/aim/mmu_oea64.c
  head/sys/powerpc/booke/pmap.c
  head/sys/powerpc/include/dump.h
  head/sys/powerpc/include/pmap.h
  head/sys/powerpc/powerpc/dump_machdep.c
  head/sys/powerpc/powerpc/mmu_if.m
  head/sys/powerpc/powerpc/pmap_dispatch.c
  head/sys/sparc64/include/dump.h
  head/sys/sparc64/sparc64/dump_machdep.c
  head/sys/sys/conf.h
  head/sys/sys/kerneldump.h
  head/sys/x86/include/dump.h
  head/sys/x86/x86/dump_machdep.c