Re: [Xen-devel] [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted interrupts

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: Thursday, September 10, 2015 6:01 PM
> To: Wu, Feng
> Cc: Andrew Cooper; Dario Faggioli; George Dunlap; Tian, Kevin;
> xen-devel@xxxxxxxxxxxxx; Keir Fraser
> Subject: RE: [PATCH v6 16/18] vmx: Add some scheduler hooks for VT-d posted
> interrupts
> 
> >>> On 10.09.15 at 11:41, <feng.wu@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Thursday, September 10, 2015 5:26 PM
> >> >>> On 10.09.15 at 10:59, <feng.wu@xxxxxxxxx> wrote:
> >> > First, how to check it while waiting to acquire the lock .pi_block_cpu
> >> > didn't change?
> >>
> >> Note the difference between "check while waiting" and "check that
> >> while waiting": The former is indeed hard to implement, while the
> >> latter is pretty straightforward (and we do so elsewhere).
> >>
> >> > Secondly, even if we can check it, what should we do if .pi_block_cpu
> >> > is changed after acquiring the lock as I mentioned above?
> >>
> >> Drop the lock and start over. I.e. (taking your pseudo code)
> >>
> >> restart:
> >>     local_pi_block_cpu = ...;
> >>     bail-if-invalid (e.g. -1 in current model)
> >>     spin_lock_irqsave(&per_cpu(, local_pi_block_cpu), flags);
> >>     if(local_pi_block_cpu != actual_pi_block_cpu) {
> >>         spin_unlock_irqrestore(&per_cpu(,local_pi_block_cpu), flags);
> >>         goto restart;
> >>     }
> >
> > Thanks a lot for showing me this pseudo code! My concern is if
> > .pi_block_vcpu is changed to -1 at this point, it doesn't work.
> > .pi_block_vcpu being -1 here means the vCPU is remove from
> > the blocking list by others, then we cannot delete it again via
> > list_del() here.
> 
> Did you miss the "bail-if-invalid" above?

I am sorry, do I miss something here? If .pi_block_cpu becomes
-1 here (after the above 'if' statement is finished with
local_pi_block_cpu == actual_pi_block_cpu ), how can "bail-if-invalid"
above help?

Thanks,
Feng

> 
> > BTW, I cannot see performance overhead for list_del_init()
> > compared to list_del().
> >
> > list_del():
> > static inline void list_del(struct list_head *entry)
> > {
> >     ASSERT(entry->next->prev == entry);
> >     ASSERT(entry->prev->next == entry);
> >     __list_del(entry->prev, entry->next);
> >     entry->next = LIST_POISON1;
> >     entry->prev = LIST_POISON2;
> > }
> >
> > list_del_init():
> > static inline void list_del_init(struct list_head *entry)
> > {
> >     __list_del(entry->prev, entry->next);
> >     INIT_LIST_HEAD(entry);
> > }
> 
> Well, yes, both do two stores (I forgot about the poisoning), but
> arguably the poisoning could become a debug-build-only thing. I.e.
> it is an implementation detail that the number of stores currently
> is the same. From an abstract perspective one should still prefer
> list_del() when the re-init isn't really needed. And in the specific
> case here asking you to use list_del() makes sure the code ends
> up not even trying the deletion when not needed.
> 
> Jan


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