Re: [Xen-devel] [PATCH v6 6/8] mm: Keep heap accessible to others while scrubbing

["Jan Beulich" <jbeulich@xxxxxxxx>]

>>> Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 08/04/17 7:03 PM >>>
>+static void check_and_stop_scrub(struct page_info *head)
>+{
>+    if ( head->u.free.scrub_state == BUDDY_SCRUBBING )
>+    {
>+        struct page_info pg;

Do you really need a full struct page_info here? I.e. can't this be
typeof(*head->u.free)? (I'm sorry for thinking of this only now.)

>@@ -1144,6 +1221,23 @@ bool scrub_free_pages(void)
>}
>}
 >
>+                st.pg = pg;
>+                /*
>+                 * get_free_buddy() grabs a buddy with first_dirty set to
>+                 * INVALID_DIRTY_IDX so we can't set pg's first_dirty here.
>+                 * It will be set either below or in the lock callback (in
>+                 * scrub_continue()).
>+                 */
>+                st.first_dirty = (i >= (1UL << order) - 1) ?

Everywhere else it is 1U - why 1UL here?

>--- a/xen/include/asm-arm/mm.h
>+++ b/xen/include/asm-arm/mm.h
>@@ -42,18 +42,26 @@ struct page_info
>unsigned long type_info;
>} inuse;
>/* Page is on a free list: ((count_info & PGC_count_mask) == 0). */
>-        struct {
>-            /* Do TLBs need flushing for safety before next page use? */
>-            unsigned long need_tlbflush:1;
>-
>-            /*
>-             * Index of the first *possibly* unscrubbed page in the buddy.
>-             * One more than maximum possible order to accommodate
>-             * INVALID_DIRTY_IDX.
>-             */
>+        union {
>+            struct {

Indentation.

>+                /* Do TLBs need flushing for safety before next page use? */
>+                unsigned long need_tlbflush:1;
>+
>+                /*
>+                 * Index of the first *possibly* unscrubbed page in the buddy.
>+                 * One more than maximum possible order to accommodate
>+                 * INVALID_DIRTY_IDX.
>+                 */
>#define INVALID_DIRTY_IDX ((1UL << (MAX_ORDER + 1)) - 1)
>unsigned long first_dirty:MAX_ORDER + 1;

Why is this not being re-indented as well?
 
>+#define BUDDY_NOT_SCRUBBING    0
>+#define BUDDY_SCRUBBING        1
>+#define BUDDY_SCRUB_ABORT      2
>+              unsigned long scrub_state:2;

Indentation. Looks to be even worse in the x86 header (including an
earlier patch adding too much indentation). (Of course all of this with
the caveat that my mail web frontend may have garbled things.)

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel