On 11/10/16 at 01:56pm, Xunlei Pang wrote:
> On 2016/11/10 at 10:45, Dave Young wrote:
> > On 11/09/16 at 06:34pm, Xunlei Pang wrote:
> >> On 2016/11/09 at 16:33, Dave Young wrote:
> >>> Hi, Xunlei
> >>> On 11/07/16 at 01:34pm, Xunlei Pang wrote:
> >>>> Extend "rd.memdebug" to "4", and "make_trace_mem" to "4+:komem".
> >>>> Add new "cleanup_trace_mem" to cleanup the trace if active.
> >>>>
> >>>> Signed-off-by: Xunlei Pang <xlpang@xxxxxxxxxx>
> >>>> ---
> >>>> modules.d/98dracut-systemd/dracut-cmdline.sh | 2 +-
> >>>> modules.d/98dracut-systemd/dracut-pre-mount.sh | 2 +-
> >>>> modules.d/98dracut-systemd/dracut-pre-pivot.sh | 3 ++-
> >>>> modules.d/98dracut-systemd/dracut-pre-trigger.sh | 2 +-
> >>>> modules.d/99base/dracut-lib.sh | 13 ++++++++++++-
> >>>> modules.d/99base/init.sh | 9 +++++----
> >>>> modules.d/99base/module-setup.sh | 1 +
> >>>> 7 files changed, 23 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/modules.d/98dracut-systemd/dracut-cmdline.sh b/modules.d/98dracut-systemd/dracut-cmdline.sh
> >>>> index 6c6ee02..bff9435 100755
> >>>> --- a/modules.d/98dracut-systemd/dracut-cmdline.sh
> >>>> +++ b/modules.d/98dracut-systemd/dracut-cmdline.sh
> >>>> @@ -42,7 +42,7 @@ export root
> >>>> export rflags
> >>>> export fstype
> >>>>
> >>>> -make_trace_mem "hook cmdline" '1+:mem' '1+:iomem' '3+:slab'
> >>>> +make_trace_mem "hook cmdline" '1+:mem' '1+:iomem' '3+:slab' '4+:komem'
> >>>> # run scriptlets to parse the command line
> >>>> getarg 'rd.break=cmdline' -d 'rdbreak=cmdline' && emergency_shell -n cmdline "Break before cmdline"
> >>>> source_hook cmdline
> >>>> diff --git a/modules.d/98dracut-systemd/dracut-pre-mount.sh b/modules.d/98dracut-systemd/dracut-pre-mount.sh
> >>>> index ae51128..a3b9d29 100755
> >>>> --- a/modules.d/98dracut-systemd/dracut-pre-mount.sh
> >>>> +++ b/modules.d/98dracut-systemd/dracut-pre-mount.sh
> >>>> @@ -8,7 +8,7 @@ type getarg >/dev/null 2>&1 || . /lib/dracut-lib.sh
> >>>>
> >>>> source_conf /etc/conf.d
> >>>>
> >>>> -make_trace_mem "hook pre-mount" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +make_trace_mem "hook pre-mount" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>> # pre pivot scripts are sourced just before we doing cleanup and switch over
> >>>> # to the new root.
> >>>> getarg 'rd.break=pre-mount' 'rdbreak=pre-mount' && emergency_shell -n pre-mount "Break pre-mount"
> >>>> diff --git a/modules.d/98dracut-systemd/dracut-pre-pivot.sh b/modules.d/98dracut-systemd/dracut-pre-pivot.sh
> >>>> index cc70e3c..dc9a250 100755
> >>>> --- a/modules.d/98dracut-systemd/dracut-pre-pivot.sh
> >>>> +++ b/modules.d/98dracut-systemd/dracut-pre-pivot.sh
> >>>> @@ -8,12 +8,13 @@ type getarg >/dev/null 2>&1 || . /lib/dracut-lib.sh
> >>>>
> >>>> source_conf /etc/conf.d
> >>>>
> >>>> -make_trace_mem "hook pre-pivot" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +make_trace_mem "hook pre-pivot" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>> # pre pivot scripts are sourced just before we doing cleanup and switch over
> >>>> # to the new root.
> >>>> getarg 'rd.break=pre-pivot' 'rdbreak=pre-pivot' && emergency_shell -n pre-pivot "Break pre-pivot"
> >>>> source_hook pre-pivot
> >>>>
> >>>> +cleanup_trace_mem
> >>>> # pre pivot cleanup scripts are sourced just before we switch over to the new root.
> >>>> getarg 'rd.break=cleanup' 'rdbreak=cleanup' && emergency_shell -n cleanup "Break cleanup"
> >>>> source_hook cleanup
> >>>> diff --git a/modules.d/98dracut-systemd/dracut-pre-trigger.sh b/modules.d/98dracut-systemd/dracut-pre-trigger.sh
> >>>> index ac1ec36..7cd821e 100755
> >>>> --- a/modules.d/98dracut-systemd/dracut-pre-trigger.sh
> >>>> +++ b/modules.d/98dracut-systemd/dracut-pre-trigger.sh
> >>>> @@ -8,7 +8,7 @@ type getarg >/dev/null 2>&1 || . /lib/dracut-lib.sh
> >>>>
> >>>> source_conf /etc/conf.d
> >>>>
> >>>> -make_trace_mem "hook pre-trigger" "1:shortmem" "2+:mem" "3+:slab"
> >>>> +make_trace_mem "hook pre-trigger" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>>
> >>>> source_hook pre-trigger
> >>>>
> >>>> diff --git a/modules.d/99base/dracut-lib.sh b/modules.d/99base/dracut-lib.sh
> >>>> index 060b3fe..2a29bbc 100755
> >>>> --- a/modules.d/99base/dracut-lib.sh
> >>>> +++ b/modules.d/99base/dracut-lib.sh
> >>>> @@ -1206,12 +1206,20 @@ are_lists_eq() {
> >>>>
> >>>> setmemdebug() {
> >>>> if [ -z "$DEBUG_MEM_LEVEL" ]; then
> >>>> - export DEBUG_MEM_LEVEL=$(getargnum 0 0 3 rd.memdebug)
> >>>> + export DEBUG_MEM_LEVEL=$(getargnum 0 0 4 rd.memdebug)
> >>>> fi
> >>>> }
> >>>>
> >>>> setmemdebug
> >>>>
> >>>> +cleanup_trace_mem()
> >>>> +{
> >>>> + # tracekomem based on kernel trace needs cleanup after use.
> >>>> + if [[ $DEBUG_MEM_LEVEL -eq 4 ]]; then
> >>> It does not work if out of tree modules add it to DEBUG_MEM_LEVEL <
> >>> 4, sounds bad to hard code it. It seems good to do the cleanup without
> >>> the check.
> >> Hi Dave,
> >>
> >> I can't understand why out of tree modules would do this, however we can't
> >> guarantee that even after removing the hard code condition. Let's say, out of
> >> tree modules use it at the very beginning and disable it quite after pre-pivot
> >> with no "rd.memdebug", then the trace will be disabled at pre-pivot which will
> >> cause confusion(or unnoticed data loss) for the out of tree modules.
> > It should be a corner case, but use a fixed DEBUG_MEM_LEVEL sounds
> > still not good, we designed the DEBUG_MEM_LEVEL but the design does not
> > limit each facility to force connect to one of the levels.
>
> Hi Dave,
>
> Maybe I didn't quite catch what you meant.
>
> The current implementation only uses level 4 for komem using '4+:komem', do you mean
> some out of tree module adds something like: make_trace_mem '2+: komem'? If so, it contradicts
> the "rd.memdebug" manpage described in PATCH3/3.
Yes, it is what I thought about.
>
> >
> > But I have no strong opinion, I can live with it..
> >
> >>> There is another concern, if someone manually enables tracing in kernel
> >>> cmdline then we should not cleanup, maybe we can print a warning msg
> >>> and do not do our tracing at all.
> >>>
> >>> For cleanup and prepare ideally we can do something like make_trace_mem,
> >>> like prepare_trace_mem and cleanup_trace_mem, for each facility if there
> >>> is a prepare/cleanup function then call it. But for the time being it
> >>> may not worth to add the complexity. What do you think?
> >> Because the script does cleanup iff is_trace_ready() returns true when
> >> both tracing is on and the events match, I think it is safe enough that
> >> we don't need to worry about the wrong cleanup if we really want to do this:
> >> is_trace_ready() {
> >> local trace_base want_events current_events
> >>
> >> trace_base=$(get_trace_base)
> >> ! [[ -f "$trace_base/tracing/trace" ]] && return 1
> >>
> >> [[ $(cat $trace_base/tracing/tracing_on) = 0 ]] && return 1
> >>
> >> # Also check if trace events were properly setup.
> >> want_events=$(get_want_events)
> >> current_events=$(echo $(cat $trace_base/tracing/set_event))
> >> [[ "$current_events" != "$want_events" ]] && return 1
> >>
> >> return 0
> >> }
> > Dracut is used more in normal boot than kdump, if one enabled trace in
> > grub and the events happen to be same, but we disabled it user will be
> > not happy.
>
> Yes, as long as they don't specify "rd.memdebug=4", the current implementation
> isn't problematic.
>
> If people use "rd.memdebug=4", they should be aware what they are doing to trace,
> also enabling the same events (module_load, module_put and alloc_pages) only is rare.
Yes, they should be aware but still need some documentation..
>
> Regards,
> Xunlei
>
> >
> >> Regards,
> >> Xunlei
> >>
> >>>> + tracekomem --cleanup
> >>>> + fi
> >>>> +}
> >>>> +
> >>>> # parameters: msg [trace_level:trace]...
> >>>> make_trace_mem()
> >>>> {
> >>>> @@ -1296,6 +1304,9 @@ show_memstats()
> >>>> iomem)
> >>>> cat /proc/iomem
> >>>> ;;
> >>>> + komem)
> >>>> + tracekomem
> >>>> + ;;
> >>>> esac
> >>>> }
> >>>>
> >>>> diff --git a/modules.d/99base/init.sh b/modules.d/99base/init.sh
> >>>> index a563393..e4f7cff 100755
> >>>> --- a/modules.d/99base/init.sh
> >>>> +++ b/modules.d/99base/init.sh
> >>>> @@ -131,7 +131,7 @@ if ! getargbool 1 'rd.hostonly'; then
> >>>> fi
> >>>>
> >>>> # run scriptlets to parse the command line
> >>>> -make_trace_mem "hook cmdline" '1+:mem' '1+:iomem' '3+:slab'
> >>>> +make_trace_mem "hook cmdline" '1+:mem' '1+:iomem' '3+:slab' '4+:komem'
> >>>> getarg 'rd.break=cmdline' -d 'rdbreak=cmdline' && emergency_shell -n cmdline "Break before cmdline"
> >>>> source_hook cmdline
> >>>>
> >>>> @@ -160,7 +160,7 @@ fi
> >>>>
> >>>> udevproperty "hookdir=$hookdir"
> >>>>
> >>>> -make_trace_mem "hook pre-trigger" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +make_trace_mem "hook pre-trigger" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>> getarg 'rd.break=pre-trigger' -d 'rdbreak=pre-trigger' && emergency_shell -n pre-trigger "Break before pre-trigger"
> >>>> source_hook pre-trigger
> >>>>
> >>>> @@ -230,7 +230,7 @@ unset RDRETRY
> >>>>
> >>>> # pre-mount happens before we try to mount the root filesystem,
> >>>> # and happens once.
> >>>> -make_trace_mem "hook pre-mount" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +make_trace_mem "hook pre-mount" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>> getarg 'rd.break=pre-mount' -d 'rdbreak=pre-mount' && emergency_shell -n pre-mount "Break pre-mount"
> >>>> source_hook pre-mount
> >>>>
> >>>> @@ -266,11 +266,12 @@ done
> >>>>
> >>>> # pre pivot scripts are sourced just before we doing cleanup and switch over
> >>>> # to the new root.
> >>>> -make_trace_mem "hook pre-pivot" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +make_trace_mem "hook pre-pivot" '1:shortmem' '2+:mem' '3+:slab' '4+:komem'
> >>>> getarg 'rd.break=pre-pivot' -d 'rdbreak=pre-pivot' && emergency_shell -n pre-pivot "Break pre-pivot"
> >>>> source_hook pre-pivot
> >>>>
> >>>> make_trace_mem "hook cleanup" '1:shortmem' '2+:mem' '3+:slab'
> >>>> +cleanup_trace_mem
> >>>> # pre pivot cleanup scripts are sourced just before we switch over to the new root.
> >>>> getarg 'rd.break=cleanup' -d 'rdbreak=cleanup' && emergency_shell -n cleanup "Break cleanup"
> >>>> source_hook cleanup
> >>>> diff --git a/modules.d/99base/module-setup.sh b/modules.d/99base/module-setup.sh
> >>>> index b03772e..a1569b1 100755
> >>>> --- a/modules.d/99base/module-setup.sh
> >>>> +++ b/modules.d/99base/module-setup.sh
> >>>> @@ -35,6 +35,7 @@ install() {
> >>>> inst_script "$moddir/initqueue.sh" "/sbin/initqueue"
> >>>> inst_script "$moddir/loginit.sh" "/sbin/loginit"
> >>>> inst_script "$moddir/rdsosreport.sh" "/sbin/rdsosreport"
> >>>> + inst_script "$moddir/memtrace-ko.sh" "/sbin/tracekomem"
> >>>>
> >>>> [ -e "${initdir}/lib" ] || mkdir -m 0755 -p ${initdir}/lib
> >>>> mkdir -m 0755 -p ${initdir}/lib/dracut
> >>>> --
> >>>> 1.8.3.1
> >>>>
> >>>> --
> >>>> To unsubscribe from this list: send the line "unsubscribe initramfs" in
> >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >>> Thanks
> >>> Dave
> >>> --
> >>> To unsubscribe from this list: send the line "unsubscribe initramfs" in
> >>> the body of a message to majordomo@xxxxxxxxxxxxxxx
> >>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Thanks
> > Dave
> > --
> > To unsubscribe from this list: send the line "unsubscribe initramfs" in
> > the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe initramfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
