Re: [Xen-devel] [PATCH] x86/mm: Improve ring management for memory events. Do not lose guest events

["Andres Lagar-Cavilla" <andres@xxxxxxxxxxxxxxxx>]

> Hi,
>
> This looks fine to me; this time I'd like Olaf to review and test it
> before I commit it. :)
>
> A few comments inline below -- mostly nits about style.

Great, will address them -- once Olaf gives us a green light.
Andres

>
> At 13:28 -0500 on 11 Jan (1326288504), Andres Lagar-Cavilla wrote:
>> Signed-off-by: Adin Scannell <adin@xxxxxxxxxxx>
>> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx>
>
> Housekeeping: please keep Olaf's Signed-off-by: line to cover the parts
> of this patch that he's certifying.
>
>> diff -r d3342b370b6f -r a85e7d46401b xen/arch/x86/hvm/hvm.c
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4188,25 +4188,31 @@ static int hvm_memory_event_traps(long p
>>                                    unsigned long value, unsigned long
>> old,
>>                                    bool_t gla_valid, unsigned long gla)
>>  {
>> +    int rc;
>>      struct vcpu* v = current;
>>      struct domain *d = v->domain;
>>      mem_event_request_t req;
>> -    int rc;
>>
>
> Please try to avoid this kind of unrelated shuffling (and I saw some
> whitespace changes later on as well).  It's not a big deal, but it makes
> reviewing a bit easier and avoids unnecessarily clashing with other
> patches.
>
>> diff -r d3342b370b6f -r a85e7d46401b xen/arch/x86/mm/mem_event.c
>> --- a/xen/arch/x86/mm/mem_event.c
>> +++ b/xen/arch/x86/mm/mem_event.c
>
>> +
>> +/*
>> + * mem_event_wake_requesters() will wakeup vcpus waiting for room in
>> the
>
> DYM mem_event_wake_blocked() ?
>
>> + * ring. These vCPUs were paused on their way out after placing an
>> event,
>> + * but need to be resumed where the ring is capable of processing at
>> least
>> + * one event from them.
>> + */
>> +static void mem_event_wake_blocked(struct domain *d, struct
>> mem_event_domain *med)
>> +{
>> +    struct vcpu *v;
>> +    int online = d->max_vcpus;
>> +    int avail_req = mem_event_ring_available(med);
>> +
>> +    if( avail_req <= 0 || med->blocked == 0 )
>
> s/if(/if (/
>
>> +        return;
>> +
>> +    /*
>> +     * We ensure that we only have vCPUs online if there are enough
>> free slots
>> +     * for their memory events to be processed.  This will ensure that
>> no
>> +     * memory events are lost (due to the fact that certain types of
>> events
>> +     * cannot be replayed, we need to ensure that there is space in the
>> ring
>> +     * for when they are hit).
>> +     * See comment below in mem_event_put_request().
>> +     */
>> +    for_each_vcpu ( d, v )
>> +        if ( test_bit(med->pause_flag, &v->pause_flags) )
>> +            online--;
>> +
>> +    ASSERT(online == (d->max_vcpus - med->blocked));
>
> Maybe better to do the cheap calculation as the default and the more
> expensive one for the ASSERT?
>
>> +    /* We remember which vcpu last woke up to avoid scanning always
>> linearly
>> +     * from zero and starving higher-numbered vcpus under high load */
>> +    if ( d->vcpu )
>> +    {
>> +        int i, j, k;
>> +
>> +        for (i = med->last_vcpu_wake_up + 1, j = 0; j < d->max_vcpus;
>> i++, j++)
>> +        {
>> +            k = i % d->max_vcpus;
>> +            v = d->vcpu[k];
>> +            if ( !v )
>> +                continue;
>> +
>> +            if ( !(med->blocked) || online >= avail_req )
>> +               break;
>> +
>> +            if ( test_and_clear_bit(med->pause_flag, &v->pause_flags) )
>> +            {
>> +                vcpu_unpause(v);
>> +                online++;
>> +                med->blocked--;
>> +                med->last_vcpu_wake_up = k;
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +/*
>> + * In the event that a vCPU attempted to place an event in the ring and
>> + * was unable to do so, it is queued on a wait queue.  These are woken
>> as
>> + * needed, and take precedence over the blocked vCPUs.
>> + */
>> +static void mem_event_wake_queued(struct domain *d, struct
>> mem_event_domain *med)
>> +{
>> +    int avail_req = mem_event_ring_available(med);
>> +
>> +    if ( avail_req > 0 )
>> +        wake_up_nr(&med->wq, avail_req);
>> +}
>> +
>> +/*
>> + * mem_event_wake_waiters() will wakeup all vcpus waiting for the ring
>> to
>
> DYM mem_event_wake() ?
>
>> + * become available.  If we have queued vCPUs, they get top priority.
>> We
>> + * are guaranteed that they will go through code paths that will
>> eventually
>> + * call mem_event_wake() again, ensuring that any blocked vCPUs will
>> get
>> + * unpaused once all the queued vCPUs have made it through.
>> + */
>> +void mem_event_wake(struct domain *d, struct mem_event_domain *med)
>> +{
>> +    if (!list_empty(&med->wq.list))
>> +        mem_event_wake_queued(d, med);
>> +    else
>> +        mem_event_wake_blocked(d, med);
>> +}
>> +
>> +static int mem_event_disable(struct domain *d, struct mem_event_domain
>> *med)
>> +{
>> +    if( med->ring_page )
>
> s/if(/if (/g  :)
>
>> +    {
>> +        struct vcpu *v;
>> +
>> +        mem_event_ring_lock(med);
>> +
>> +        if (!list_empty(&med->wq.list))
>
> if ( ... )
>
>> +        {
>> +            mem_event_ring_unlock(med);
>> +            return -EBUSY;
>> +        }
>> +
>> +        unmap_domain_page(med->ring_page);
>> +        med->ring_page = NULL;
>> +
>> +        unmap_domain_page(med->shared_page);
>> +        med->shared_page = NULL;
>> +
>> +        /* Wake up everybody */
>> +        wake_up_all(&med->wq);
>> +
>> +        /* Unblock all vCPUs */
>> +        for_each_vcpu ( d, v )
>> +        {
>> +            if ( test_and_clear_bit(med->pause_flag, &v->pause_flags) )
>> +            {
>> +                vcpu_unpause(v);
>> +                med->blocked--;
>> +            }
>> +        }
>> +
>> +        mem_event_ring_unlock(med);
>> +    }
>>
>>      return 0;
>>  }
>>
>> -void mem_event_put_request(struct domain *d, struct mem_event_domain
>> *med, mem_event_request_t *req)
>> +static inline void mem_event_release_slot(struct domain *d,
>> +                                          struct mem_event_domain *med)
>> +{
>> +    /* Update the accounting */
>> +    if ( current->domain == d )
>> +        med->target_producers--;
>> +    else
>> +        med->foreign_producers--;
>> +
>> +    /* Kick any waiters */
>> +    mem_event_wake(d, med);
>> +}
>> +
>> +/*
>> + * mem_event_mark_and_pause() tags vcpu and put it to sleep.
>> + * The vcpu will resume execution in mem_event_wake_waiters().
>> + */
>> +void mem_event_mark_and_pause(struct vcpu *v, struct mem_event_domain
>> *med)
>> +{
>> +    if ( !test_and_set_bit(med->pause_flag, &v->pause_flags) )
>> +    {
>> +        vcpu_pause_nosync(v);
>> +        med->blocked++;
>> +    }
>> +}
>> +
>> +/*
>> + * This must be preceeded by a call to claim_slot(), and is guaranteed
>> to
>
> preceded
>
> Cheers,
>
> Tim.
>



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