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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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?

> 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