Re: [PATCH 17/19] [not for merge] print more debug info in mm

From: Daniel Kiper <dkiper@net-space.pl>
To: Glenn Washburn <development@efficientek.com>
Cc: Daniel Axtens <dja@axtens.net>,
	grub-devel@gnu.org, leif@nuviainc.com, stefanb@linux.ibm.com,
	ps@pks.im
Subject: Re: [PATCH 17/19] [not for merge] print more debug info in mm
Date: Wed, 10 Nov 2021 23:17:54 +0100	[thread overview]
Message-ID: <20211110221754.ekcuhsw4dewo3mod@tomti.i.net-space.pl> (raw)
In-Reply-To: <20211110133512.03537798@crass-HP-ZBook-15-G2>

On Wed, Nov 10, 2021 at 01:35:12PM -0600, Glenn Washburn wrote:
> On Wed, 10 Nov 2021 14:47:07 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > CC-ing Glenn...
> >
> > On Tue, Oct 12, 2021 at 06:30:06PM +1100, Daniel Axtens wrote:
> > > This is handy for debugging - I'm including it in case anyone else hacking
> > > on this area finds it helpful.
> > >
> > > Signed-off-by: Daniel Axtens <dja@axtens.net>
> > > ---
> > >  grub-core/kern/mm.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/grub-core/kern/mm.c b/grub-core/kern/mm.c
> > > index 58d5b89e8860..811df1ab5ebb 100644
> > > --- a/grub-core/kern/mm.c
> > > +++ b/grub-core/kern/mm.c
> > > @@ -135,6 +135,9 @@ grub_mm_init_region (void *addr, grub_size_t size)
> > >    for (p = &grub_mm_base, q = *p; q; p = &(q->next), q = *p)
> > >      {
> > >        /* Does this region come _before_ an existing region? */
> > > +      grub_printf ("Extending w/ before %p + %" PRIxGRUB_SIZE " + %" PRIxGRUB_SIZE " = %p ? %s\n",
> > > +		 (grub_uint8_t *)addr, size, q->pre_size, (grub_uint8_t *)q,
> > > +		 (grub_uint8_t *)addr + size + q->pre_size == (grub_uint8_t *) q ? "yes" : "no");
> >
> > I think this kind of messages can be useful. Same applies to patch #18.
> > Though I would use grub_dprintf() instead which should be wrapped with
> > #ifdef MM_DEBUG. However, we have to be very careful with printing any
> > messages from mm and do not exeecec 255 chars message length. If we go
> > above that limit then we will trigger dynamic allocation in grub_dprintf()
> > from mm which may lead to a recursion...
> >
> > Additionally, I think Glenn's patch allowing us to disable logging from
> > certain subsystem would be useful here. Glenn, could you take a look at
> > it once again?
>
> Yes, I'm planning on it. Its been low priority for me and this month is

Cool! Thanks!

> very busy fo me IRL. I don't think my patches should hold this up
> though (or would this cause way to much logs if one didn't want them?)

No rush...

Daniel