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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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