Re: [Xen-devel] [PATCH v8 15/17] vmx: VT-d posted-interrupt core logic handling

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

Hey,

Here I am reviewing this patch, sorry for the delay.

Ok, we have discussed a lot about all this, and in fact I had to go
back in my mail archive and re-read the rather long sub-thread for this
patch in v7. :-)

Also, in that thread, I found (as I was recalling there being) a couple of open 
questions, one even pointing to the possibility of adopting a different design 
for this part of the code, which I am not sure could have been considered a 
closed matter.

In any case, it would have been nice, given the situation, if you'd have put a 
few words about, e.g., which solution you ended up implementing and why, either 
in the cover or here (e.g., in the '---' area).

From the design point of view, I said during v7 that I don't dislike having 
some of the things that this feature requires dpme in (VMX specific part of) 
the context switch path, and that is still valid.

What I really don't like much is this blocking cancellation hook you have 
introduced.

I mean...

On Mon, 2015-10-12 at 16:55 +0800, Feng Wu wrote:
> - Add the following hooks, this part was suggested
>   by George Dunlap <george.dunlap@xxxxxxxxxxxxx> and
>   Dario Faggioli <dario.faggioli@xxxxxxxxxx>.
>     * arch_vcpu_block()
>       Called alled before vcpu is blocking and update the PID
>       (posted-interrupt descriptor).
> 
>     * arch_vcpu_block_cancel()
>       Called when interrupts come in during blocking.
> 
... This one.

Reason is, hooks are not, IMO, among the nicest things. You have to
remember to call them, you have to put the call to them in the proper
place, etc., when writing the code. OTOH, when reading the code, they
break the flow and force one to go and figure out what happens in
potentially not so related areas. In summary, they're hard to get
right. :-/

That being said, I can live with this, but I wonder whether we really
can't do without. For instance, Jan said in the v7 thread:

  "Couldn't this be taken care of by, if necessary, adjusting PI state
   in vmx_do_resume()?"

This is actually what started the sub-sub-thread about the alternative
design of doing everything during VMENTERs/VMEXITs. If you are
unconvinced about going that path all the way, would at least do the
fixup in there (i.e., taking care of the case where we called
arch_vcpu_block() but then we did not block) work and make sense?

Actually, I think even another possible implementation variant that was
suggested at some point (by George, in this case, for other reasons and
purposes) could make this adding this hook unnecessary, i.e.:

  "vcpu_block()
    set(_VPF_blocked)
    local_events_need_delivery()
      hvm_vcpu_has_pending_irq()
    ...
  context_switch
     v->arch.block()
      - Add v to pcpu.pi_blocked_vcpu
      - NV => pi_wakeup_vector

  If we do it [this] way, and an interrupt comes in before the context
  switch is finished, it will call posted_intr_vector.  We can, at that
  point, check to see if the current vcpu is marked as blocked.  If it 
  is, we can call vcpu_unblock() without having to modify NV or worry 
  about adding / removing the vcpu from the pi_blocked_vcpu list."

At the time, I "voted against" this design, because it seemed we could
manage to handle interrupt ('regular' and posted) happening during
blocking in one and unified way, and with _only_ arch_vcpu_block(). If
that is no longer the case (and it's not, as we're adding more hooks,
and the need to call the second is a special case being introduced by
PI), it may be worth reconsidering things...

So, all in all, I don't know. As said, I don't like this cancellation
hook because it's one more hook and because --while I see why it's
useful in this specific case-- I don't like having it in generic code
(in schedule.c), and even less having it called in two places
(vcpu_block() and do_pool()). However, if others (Jan and George, I
guess) are not equally concerned about it, I can live with it.

Thoughts?

>     * vmx_pi_switch_from()
>       Called before context switch, we update the PID when the
>       vCPU is preempted or going to sleep.
> 
>     * vmx_pi_switch_to()
>       Called after context switch, we update the PID when the vCPU
>       is going to run.
> 
>     * arch_vcpu_wake_prepare()
>       It will be called when waking up the vCPU, we update
>       the posted interrupt descriptor when the vCPU is
>       unblocked.
> 
The rest of the patch seems fine to me (at least the scheduling related
implications).

Just a few (pretty minor) comments.

> --- a/xen/arch/x86/domain.c
> +++ b/xen/arch/x86/domain.c
> @@ -1608,6 +1608,18 @@ void context_switch(struct vcpu *prev, struct
> vcpu *next)
>      if ( (per_cpu(curr_vcpu, cpu) == next) ||
>           (is_idle_domain(nextd) && cpu_online(cpu)) )
>      {
> +        /*
> +         * When we handle the lazy context switch for the following
> +         * two scenarios:
> +         * - Preempted by a tasklet, which uses in an idle context
> +         * - the prev vcpu is in offline and no new available vcpus
> in run queue
> +         * We don't change the 'SN' bit in posted-interrupt
> descriptor, this
> +         * may incur spurious PI notification events, but since PI
> notification
> +         * event is only sent when 'ON' is clear, and once the PI
> notificatoin
> +         * is sent, ON is set by hardware, so not so many spurious
> events and
> +         * it is not a big deal.
> +         */
> +
>          local_irq_enable();
>      }
>
This comment: can't it leave somewhere else, more VMX and/or PI
related?

I know this is arch code already but, still, if I'm here, I am reading
and trying to understand how context switch works, potentially, not
being interested in PI at all... And yet I find this doc comment,
talking about some SN and ON bits, without even defining what they are
and what they mean. :-/

Really, I'm not saying we shouldn't have it. On the contrary, it has
some valuable content in it. Can we just find another place where to
put it?

Also, about the content. The last part, when it talks about spurious
interrupts, it says they're not a problem because we won't get that
many. I think that someone not very familiar with this things could use
being also told that it is ok/safe to get them (i.e., they don't get
lost, etc.). There's an email from George that explain this quite well.
I'd also be ok with this particular thing going in the patch changelog,
rather than in a comment, as far as it is somewhere.

> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> index 3eefed7..383fd62 100644
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c

> @@ -800,10 +802,13 @@ void vcpu_block(void)
>  
>      set_bit(_VPF_blocked, &v->pause_flags);
>  
> +    arch_vcpu_block(v);
> +
>
This is maybe not so big of a deal but, since we call this pretty early
in the blocking path, and _especially_ if we are to keep the
cancellation hook, we may want to consider arch_vcpu_block_prepare()
(as we did for wake).

Regards,
Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

Attachment: signature.asc
Description: This is a digitally signed message part

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