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

[Juergen Gross <jgross@xxxxxxxx>]

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?

--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -40,6 +40,16 @@
     unlikely(ret_warn_on_);             \
 })

+#define WARN_ONCE() do {                \
+    static bool warned = false;         \
+                                        \
+    if ( !warned )                      \
+    {                                   \
+        warned = true;                  \
+        WARN();                         \
+    }                                   \
+} while (0)
+
 /* All clang versions supported by Xen have _Static_assert. */
 #if defined(__clang__) || \
     (__GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6))
@@ -63,7 +73,7 @@
 #define ASSERT_UNREACHABLE() assert_failed("unreachable")
 #else
 #define ASSERT(p) do { if ( 0 && (p) ) {} } while (0)
-#define ASSERT_UNREACHABLE() do { } while (0)
+#define ASSERT_UNREACHABLE() WARN_ONCE()
 #endif

 #define ABS(_x) ({                              \


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature