Hi,
The general approach seems good. A few comments on the detail, below:
At 17:24 -0400 on 28 Sep (1317230698), Adin Scannell wrote:
> -void mem_event_put_request(struct domain *d, struct mem_event_domain *med,
> mem_event_request_t *req)
> +static inline int mem_event_ring_free(struct domain *d, struct
> mem_event_domain *med)
> +{
> + int free_requests;
> +
> + free_requests = RING_FREE_REQUESTS(&med->front_ring);
> + if ( unlikely(free_requests < d->max_vcpus) )
> + {
> + /* This may happen. */
> + gdprintk(XENLOG_INFO, "mem_event request slots for domain %d: %d\n",
> + d->domain_id, free_requests);
> + WARN_ON(1);
If this is something that might happen on production systems (and is
basically benign except for the performance), we shouldn't print a full
WARN(). The printk is more than enough.
> + }
> +
> + return free_requests;
> +}
> +
> +int mem_event_put_request(struct domain *d, struct mem_event_domain *med,
> mem_event_request_t *req)
> {
> mem_event_front_ring_t *front_ring;
> RING_IDX req_prod;
>
> + if( mem_event_check_ring(d, med) < 0 )
Xen coding style has another space between the 'if' and the '(' (here
and elsewhere).
> @@ -135,16 +166,28 @@ void mem_event_put_request(struct domain
> req_prod++;
>
> /* Update ring */
> - med->req_producers--;
> front_ring->req_prod_pvt = req_prod;
> RING_PUSH_REQUESTS(front_ring);
>
> + /*
> + * We ensure that each vcpu can put at least *one* event -- because some
> + * events are not repeatable, such as dropping a page. This will ensure
> no
> + * vCPU is left with an event that they must place on the ring, but
> cannot.
> + * They will be paused after the event is placed.
> + * See large comment below in mem_event_unpause_vcpus().
> + */
> + if( current->domain->domain_id == d->domain_id &&
> + mem_event_ring_free(d, med) < d->max_vcpus )
> + mem_event_mark_and_pause(current, med);
> +
This idiom of comparing domain-ids cropped up in the earlier mem-event
patches and seems to be spreading, but the right check is just
(current->domain == d).
Also: are there cases where current->domain != d? If so, can't those cases
cause the ring to fill up?
> -void mem_event_unpause_vcpus(struct domain *d)
> +void mem_event_unpause_vcpus(struct domain *d, struct mem_event_domain *med)
> {
> struct vcpu *v;
> + int free;
> + int online = d->max_vcpus;
>
5A> + if( !med->blocked )
> + return;
> +
> + mem_event_ring_lock(med);
> + free = mem_event_ring_free(d, med);
> +
> + /*
> + * 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 large comment above in mem_event_put_request().
> + */
> for_each_vcpu ( d, v )
> + if ( test_bit(_VPF_mem_event, &v->pause_flags) )
> + online--;
> +
> + for_each_vcpu ( d, v )
> + {
> + if ( !(med->blocked) || online >= mem_event_ring_free(d, med) )
> + break;
Is there a risk that under heavy mem-event loads vcpu 0 might starve
other vcpus entirely because they're never allowed to unpause here?
> if ( test_and_clear_bit(_VPF_mem_event, &v->pause_flags) )
> + {
> vcpu_wake(v);
> + online++;
> + med->blocked--;
> + }
> + }
> +
> + mem_event_ring_unlock(med);
> }
>
> -void mem_event_mark_and_pause(struct vcpu *v)
> +void mem_event_mark_and_pause(struct vcpu *v, struct mem_event_domain *med)
> {
> - set_bit(_VPF_mem_event, &v->pause_flags);
> - vcpu_sleep_nosync(v);
> -}
> -
> -void mem_event_put_req_producers(struct mem_event_domain *med)
> -{
> - mem_event_ring_lock(med);
> - med->req_producers--;
> - mem_event_ring_unlock(med);
> + if ( !test_bit(_VPF_mem_event, &v->pause_flags) )
> + {
> + set_bit(_VPF_mem_event, &v->pause_flags);
Does this need to be an atomic test-and-set?
Cheers,
Tim.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home