|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH] x86/mm: Improve ring management for memory events. Do not lose guest events
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.
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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |