Re: [Xen-devel] Issue policing writes from Xen to PV domain memory

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.05.14 at 19:48, <aravindp@xxxxxxxxx> wrote:
>>> Looking at what the solution for the ring being full in the PV case
>>> whether we are policing Xen writes or not, calling wait() will not
>>> work due to the scenario I had mentioned a while back and is shown above
>>in the stack trace.
>>> I am repeating that flow here
>>> mem_event_claim_slot() ->
>>>     mem_event_wait_slot() ->
>>>              wait_event(mem_event_wait_try_grab(med, &rc) != -
>>EBUSY)
>>>
>>> wait_event() macro looks like this:
>>> do {
>>>     if ( mem_event_wait_try_grab(med, &rc) != -EBUSY )
>>>         break;
>>>     for ( ; ; ) {
>>>         prepare_to_wait(&med->wq);
>>>         if ( mem_event_wait_try_grab(med, &rc) != -EBUSY )
>>>             break;
>>>         wait();
>>>     }
>>>     finish_wait(&med->wq);
>>> } while (0)
>>>
>>> In the case where the ring is full, wait() gets called and the cpu
>>> gets scheduled away. But since it is in middle of a pagefault, when it
>>> runs again it ends up in handle_exception_saved and the same pagefault is
>>tried again.
>>> But since finish_wait() never ends up being called wqv->esp never
>>> becomes 0 and hence the assert fires on the next go around. So I think
>>> we should be calling
>>> process_pending_softirqs() instead of wait() for PV domains.
>>
>>That would effectively be a spin wait then, which is surely not the right 
>>thing.
>>But I don't follow your explanation above anyway - when coming back from
>>wait(), the state is the same as the original one, so the page fault handling
>>continues, it's not being retried.
> 
> Let me step through this. The pagefault for runstate occurs and wait_event() 
> gets called and the ring is full. 
> 
> wait_event():
> do { 
>     if ( mem_event_wait_try_grab(med, &rc) != -EBUSY )       ==> This will 
> return EBUSY
>         break; 
>     for ( ; ; ) { 
>         prepare_to_wait(&med->wq); 
>         if ( mem_event_wait_try_grab(med, &rc) != -EBUSY )   ==> This will 
> return EBUSY
>             break; 
>         wait();                                                               
>                                     ==> Write to runstate area occurs...
>     } 
>     finish_wait(&med->wq);
> } while (0)
> 
> ...now this write to runstate will again cause a pagefault and we will end 
> up in wait_event() again. The previous attempt would have called 
> prepare_to_wait() but finish_wait() was not called. 
> finish_wait()->__finish_wait() is where wqv->esp gets reset to NULL. So now:
> 
> wait_event():
> do { 
>     if ( mem_event_wait_try_grab(med, &rc) != -EBUSY )       ==> This will 
> return EBUSY
>         break; 
>     for ( ; ; ) { 
>         prepare_to_wait(&med->wq);                                            
>      ==> Assertion 'wqv->esp == 0' fails
>         if ( mem_event_wait_try_grab(med, &rc) != -EBUSY ) 
>             break; 
>         wait();
>     } 
>     finish_wait(&med->wq);
> } while (0)
> 
> Does this make sense?

Of course. But it still doesn't mean you can replace the wait() here
with something else (e.g. the effectively spin wait loop you suggested
earlier). As said earlier on, you need to find a way to avoid the second
invocation of wait_event(), rather than a way to make it "work".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel