[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





On 06/27/2017 03:28 PM, Jan Beulich wrote:
Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> 06/22/17 8:56 PM >>>
Changes in v5:
* Fixed off-by-one error in setting first_dirty
* Changed struct page_info.u.free to a union to permit use of ACCESS_ONCE in
   check_and_stop_scrub()

I don't see the need for this:

+static void check_and_stop_scrub(struct page_info *head)
+{
+    if ( head->u.free.scrub_state == BUDDY_SCRUBBING )
+    {
+        struct page_info pg;
+
+        head->u.free.scrub_state = BUDDY_SCRUB_ABORT;
+        spin_lock_kick();
+        for ( ; ; )
+        {
+            /* Can't ACCESS_ONCE() a bitfield. */
+            pg.u.free.val = ACCESS_ONCE(head->u.free.val);

Something like ACCESS_ONCE(head->u.free).val ought to work (or read_atomic(),
due to the questionable scalar type check in ACCESS_ONCE()).

Hmm... I couldn't get this to work with either suggestion.

page_alloc.c:751:13: error: conversion to non-scalar type requested
             pg.u.free = read_atomic(&head->u.free);

page_alloc.c:753:6: error: conversion to non-scalar type requested
      if ( ACCESS_ONCE(head->u.free).scrub_state != BUDDY_SCRUB_ABORT )

@@ -1106,25 +1155,53 @@ bool scrub_free_pages(void)
          do {
              while ( !page_list_empty(&heap(node, zone, order)) )
              {
-                unsigned int i;
+                unsigned int i, dirty_cnt;
+                struct scrub_wait_state st;
/* Unscrubbed pages are always at the end of the list. */
                  pg = page_list_last(&heap(node, zone, order));
                  if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX )
                      break;
+ ASSERT(!pg->u.free.scrub_state);

Please use BUDDY_NOT_SCRUBBING here.

@@ -1138,6 +1215,17 @@ bool scrub_free_pages(void)
                      }
                  }
+ st.pg = pg;
+                st.first_dirty = (i >= (1UL << order) - 1) ?
+                    INVALID_DIRTY_IDX : i + 1;

Would you mind explaining to me (again?) why you can't set pg's first_dirty
directly here? In case I'm not mistaken and this has been asked before, maybe
this is a hint that a comment might be warranted.


In get_free_buddy() (formerly part of alloc_heap_pages()) I have

           /* Find smallest order which can satisfy the request. */
            for ( j = order; j <= MAX_ORDER; j++ )
            {
                if ( (pg = page_list_remove_head(&heap(node, zone, j))) )
                {
                    if ( pg->u.free.first_dirty == INVALID_DIRTY_IDX )
                        return pg;
                    /*
                     * We grab single pages (order=0) even if they are
* unscrubbed. Given that scrubbing one page is fairly quick
                     * it is not worth breaking higher orders.
                     */
                    if ( (order == 0) || use_unscrubbed )
                    {
                        check_and_stop_scrub(pg);
                        return pg;
                    }


If first_dirty gets assigned INVALID_DIRTY_IDX then get_free_buddy() will return pg right away without telling the scrubber that the buddy has been taken for use. The scrubber will then put the buddy back on the heap.


-boris

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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.