Re: [Xen-devel] [PATCH 1 of 2] Allow decrease_reservation to be preempted if remove_page returns negative

["Jan Beulich" <JBeulich@xxxxxxxx>]

 >>> On 05.12.11 at 16:24, Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx> wrote:
> This will allow for handling of full paging rings in a more graceful manner.
> ATM, remove_page does not return negative values, so this code is not yet
> exercised.

Which makes the patch on its own be of questionable value...

> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
> 
> diff -r 790cd814bee8 -r 4c4a9ed0728c xen/common/memory.c
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -226,6 +226,8 @@ static void decrease_reservation(struct 
>  
>      for ( i = a->nr_done; i < a->nr_extents; i++ )
>      {
> +        int one_remove_succeeded = 0;
> +
>          if ( hypercall_preempt_check() )
>          {
>              a->preempted = 1;
> @@ -255,8 +257,33 @@ static void decrease_reservation(struct 
>              continue;
>  
>          for ( j = 0; j < (1 << a->extent_order); j++ )
> -            if ( !guest_remove_page(a->domain, gmfn + j) )
> +        {
> +            int rv = guest_remove_page(a->domain, gmfn + j);
> +            /* negative rv is a result of a mem event ring full
> +             * in the presence of paging. We preempt the hypercall */
> +            if ( rv < 0 )
> +            {
> +                /* Indicate we're done with this extent */
> +                if ((j+1) == (1 << a->extent_order))
> +                    i++;

So this implies that the page was actually acted upon? How that, if
the event ring was full? (That's part of the reason why doing the
change here without the change to guest_remove_page() is
bogus - one can't really review it in one step.)

> +                a->preempted = 1;
> +                raise_softirq(SCHEDULE_SOFTIRQ);

Do you really need to open code this *here* (rather than, if at all,
in guest_remove_page() or wherever else the event ring full
condition is actually being determined)?

>                  goto out;
> +            }
> +            /* rv of zero means we tried to remove a gfn with no backing
> +             * mfn. It can be a bad argument, or, a continuation in the midst
> +             * of an extent. Heuristically determine second case. */

Heuristically? What if this goes wrong (I can't really judge this from the
code below)?

Jan

> +            if ( !rv )
> +            {
> +                /* Has to be first extent */
> +                if ( i != a->nr_done )
> +                    goto out;
> +                /* No previous remove in this extent must have succeeded */
> +                if ( one_remove_succeeded )
> +                    goto out;
> +            } else
> +                one_remove_succeeded = 1;
> +        }
>      }
>  
>   out:
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx 
> http://lists.xensource.com/xen-devel 




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel