Hi Philipp,
On Tue, Apr 18, 2023 at 11:51 PM Philipp Rudo <prudo(a)redhat.com> wrote:
Hi Tao,
On Tue, 18 Apr 2023 17:18:48 +0800
Tao Liu <ltao(a)redhat.com> wrote:
> Hi Philipp,
>
> On Tue, Apr 18, 2023 at 5:02 PM Philipp Rudo <prudo(a)redhat.com> wrote:
> >
> > Hi Tao,
> >
> > On Mon, 17 Apr 2023 16:52:41 +0800
> > Tao Liu <ltao(a)redhat.com> wrote:
> >
> > > We will output memstrack analysis report in kexec-tools side during
kdump.
> > > Becasue kdump will use 'reboot -f' to reboot 2nd kernel by
default,
> > > which leaves no stable hook target to terminate memstrack and output its
> > > analysis report in dracut side. However it is easy to implement in
> > > kexec-tools side.
> > >
> > > In this patch, we will place the memstrack report before do_kdump_pre()
> > > for now, that is right before kdump starts, so kdump will not share
memory
> > > with memstrack. However if later we want memstrack to record memory
status
> > > during kdump as well, we can easily place it after do_kdump_post().
> >
> > In my expectation doing all the IO to write the dump to disk has the
> > potential to waste lots of memory. That's why I think it's important
> > that we also record the memory status during the dump. Even when it
> > means that memstrack and kdump are competing over the memory.
> >
> Thanks for the comments. I didn't understand why doing IO writing
> would waste memory, do you mean there might be disk data caches which
> will occupy lots of memory? I have no objections to recording memory
> status during kdump. It is easy to get fixed in v2.
yeah, I meant things like caches, bounce buffers, etc.. Their size
changes over time be it intentional or due to bugs and I don't think
they are always fully allocated during init. So there is a chance
that the crashkernel boots but then runs oom during dumping. That's why
I think it makes sense to always record the memory usage when writing
the dump.
Especially, as I don't think that there is a big conflict between
memstrack and the core collector. My assumption is that you don't
enable memstrack permanently but only when you are actively testing
something. It shouldn't be a problem to temporarily increase the
crashkernel value in such situations.
Anyway, that's just my 2 cents.
Thanks a lot for the explanation! I agree with your idea,
Thanks,
Tao Liu
Thanks
Philipp
>
> > > Signed-off-by: Tao Liu <ltao(a)redhat.com>
> > > ---
> > > dracut-kdump.sh | 22 ++++++++++++++++++++++
> > > dracut-module-setup.sh | 2 ++
> > > 2 files changed, 24 insertions(+)
> > >
> > > diff --git a/dracut-kdump.sh b/dracut-kdump.sh
> > > index 4eec70b..8096da4 100755
> > > --- a/dracut-kdump.sh
> > > +++ b/dracut-kdump.sh
> > > @@ -574,6 +574,26 @@ fence_kdump_notify()
> > > fi
> > > }
> > >
> > > +handle_memstrack_report()
> > > +{
> > > + MEMSTRACK_LOG="/.memstrack"
> > > +
> > > + if ! [ "$DEBUG_MEM_LEVEL" -ge 4 ]; then
> >
> > small nit but
> >
> > ! [ "$DEBUG_MEM_LEVEL" -ge 4 ] == [ "$DEBUG_MEM_LEVEL" -lt
4 ]
> >
> > with the latter being easier to read (at least for me)
> >
> Agreed, thanks for pointing this out.
>
> Thanks,
> Tao Liu
>
> > Thanks
> > Philipp
> >
> > > + return
> > > + fi
> > > +
> > > + if pgrep -c '[m]emstrack' > /dev/null; then
> > > + pkill --signal INT '[m]emstrack'
> > > + while pgrep -c '[m]emstrack' > /dev/null; do
> > > + sleep 1
> > > + done
> > > + fi
> > > +
> > > + if [ -e "$MEMSTRACK_LOG" ]; then
> > > + IFS= dinfo < "$MEMSTRACK_LOG"
> > > + fi
> > > +}
> > > +
> > > if [ "$1" = "--error-handler" ]; then
> > > get_kdump_confs
> > > do_failure_action
> > > @@ -599,6 +619,8 @@ if [ -z "$DUMP_INSTRUCTION" ]; then
> > > DUMP_INSTRUCTION="dump_fs $NEWROOT"
> > > fi
> > >
> > > +handle_memstrack_report
> > > +
> > > if ! do_kdump_pre; then
> > > derror "kdump_pre script exited with non-zero status!"
> > > do_final_action
> > > diff --git a/dracut-module-setup.sh b/dracut-module-setup.sh
> > > index a8ac16c..37845e3 100755
> > > --- a/dracut-module-setup.sh
> > > +++ b/dracut-module-setup.sh
> > > @@ -1033,6 +1033,8 @@ install() {
> > > inst "/usr/bin/printf" "/sbin/printf"
> > > inst "/usr/bin/logger" "/sbin/logger"
> > > inst "/usr/bin/chmod" "/sbin/chmod"
> > > + inst "/usr/bin/pkill" "/usr/bin/pkill"
> > > + inst "/usr/bin/pgrep" "/usr/bin/pgrep"
> > > inst "/lib/kdump/kdump-lib-initramfs.sh"
"/lib/kdump-lib-initramfs.sh"
> > > inst "/lib/kdump/kdump-logger.sh"
"/lib/kdump-logger.sh"
> > > inst "$moddir/kdump.sh" "/usr/bin/kdump.sh"
> >
>