Xen devel - Sep 2011 - [PATCH 1/2] enable event channel wake-up for mem_event interfaces

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

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@lists.xensource.com
http://lists.xensource.com/xen-devel

I''ll rework the patches incorporating the feedback and resend the
modified patches.

I''ve got a couple of quick notes, inline below.
>> @@ -135,16 +166,28 @@ void mem_event_put_request(struct domain
>> +    /*
>> +     * 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?
Yes, I believe there are there are cases where a different domain
(i.e. the domain w/ the device model) can map a page generating an
event (mapping a paged-out page, writeable mappings of pages with
non-writable access bits, etc.).  Unfortunately, we can''t pause any
vcpu in those cases.

This is something that I intend to revisit, as guaranteeing that
absolutely no events are lost may be quite complicated (especially
when there are certain events which are not repeatable). I''m
considering the use of the new wait queues or other mechanisms to wait
for the ring to clear up while in the same context.... but that is
another sort of tricky.

I''m quite glad you pointed this out, because I believe the
mem_event_mark_and_pause following the ''possible lost mem_event for
domain'' is incorrect.  If this is a different domain generating the
event, this line is very wrong.
>> +    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?
Unfortunately, yes.  With heavy mem event load (& a dom0 that isn''t
taking them off the ring fast enough).  I think there are two fair
alternatives:
1) Unpausing a vcpu at random.
2) Waiting until the ring reaches a watermark and unpausing all vCPUs.

Any thoughts on these?
>> +    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?
I believe that it is currently always called within a locked
(mem_event_ring_lock) context, but it should be atomic test-and-set
anyways in case it''s not.


Cheers!
-Adin

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

At 13:18 -0400 on 13 Oct (1318511910), Adin Scannell
wrote:
> I''ll rework the patches incorporating the feedback and resend the
> modified patches.
> 
> I''ve got a couple of quick notes, inline below.
> 
> >> @@ -135,16 +166,28 @@ void mem_event_put_request(struct domain
> >> +    /*
> >> +     * 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?
> 
> Yes, I believe there are there are cases where a different domain
> (i.e. the domain w/ the device model) can map a page generating an
> event (mapping a paged-out page, writeable mappings of pages with
> non-writable access bits, etc.).  Unfortunately, we can''t pause
any
> vcpu in those cases.
True.
> This is something that I intend to revisit, as guaranteeing that
> absolutely no events are lost may be quite complicated (especially
> when there are certain events which are not repeatable). I''m
> considering the use of the new wait queues or other mechanisms to wait
> for the ring to clear up while in the same context.... but that is
> another sort of tricky.
Yep.  You could do what the timer code does in this situation and have a
linked list of events that didnt make it on to the queue (to be tidied
up later when there''s space) but I can see that being messy too, and 
in extremis you might not be able to xmalloc another entry...
> > 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?
> 
> Unfortunately, yes.  With heavy mem event load (& a dom0 that
isn''t
> taking them off the ring fast enough).  I think there are two fair
> alternatives:
> 1) Unpausing a vcpu at random.
> 2) Waiting until the ring reaches a watermark and unpausing all vCPUs.
> 
> Any thoughts on these?
3) always start the scan from a different place (either a round-robin or
from the last-serviced-vcpu + 1)?

Cheers,

Tim.

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

On Thu, Oct 06, Tim Deegan wrote:
> 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.
While I havent reviewed the whole patch (sorry for that), one thing that
will break is p2m_mem_paging_populate() called from dom0.

If the ring is full, the gfn state was eventually forwarded from
paging-out state to paging-in state. But since the ring was full, no
request was sent to xenpaging which means the gfn remains in
p2m_ram_paging_in_start until the guest eventually tries to access the
gfn as well. Dom0 will call p2m_mem_paging_populate() again and again (I
think), but there will be no attempt to send a new request once the ring
has free slots again, because the gfn is already in the page-in path and
the vcpu does not belong to the guest.

I have some wild ideas how to handle this situation, but the patch as is
will break page-in attempts from xenpaging itself.

Olaf

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