Re: [PATCH v10 2/9] xen: do not free reserved memory into heap

[Juergen Gross <jgross@xxxxxxxx>]

On 24.08.22 11:31, Julien Grall wrote:
> Hi Juergen,
>
> On 24/08/2022 10:27, Juergen Gross wrote:
> > On 24.08.22 11:03, Julien Grall wrote:
> > > Hi,
> > >
> > > On 16/08/2022 07:40, Jan Beulich wrote:
> > > > On 16.08.2022 04:36, Penny Zheng wrote:
> > > > > +void free_domstatic_page(struct page_info *page)
> > > > > +{
> > > > > +    struct domain *d = page_get_owner(page);
> > > > > +    bool drop_dom_ref;
> > > > > +
> > > > > +    if ( unlikely(!d) )
> > > > > +    {
> > > > > +        ASSERT_UNREACHABLE();
> > > > > +        printk("The about-to-free static page %"PRI_mfn" must be owned by 
> > > > > a domain\n",
> > > > > +               mfn_x(page_to_mfn(page)));
> > > > > +        return;
> > > > > +    }
> > > > For the message to be useful as a hint if the assertion triggers, it
> > > > wants printing ahead of the assertion. I also think it wants to be a
> > > > XENLOG_G_* kind of log level, so it would be rate limited by default
> > > > in release builds. Just to be on the safe side.
> > > +1
> > >
> > > > (I'm not in favor of
> > > > the log message in the first place, but I do know that Julien had
> > > > asked for one.)
> > > TBH, I think all ASSERT_UNREACHABLE() paths should be accompanied with a 
> > > printk(). This would also allow us to catch issue in production rather than 
> > > in only in debug.
> > What about something like the following then?
> That could be a first step. I still think a message like Penny has added in the 
> patch is useful.
As the WARN() would spit out file and line, a comment might be enough.

In the end that is something the maintainer of the related code should decide.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature