Re: [PATCH 1/4] Move irq routing data structure to rcu locking

[Gregory Haskins <gregory.haskins@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3029 bytes --]

Gleb Natapov wrote:
> On Mon, Jul 13, 2009 at 09:16:47AM -0400, Gregory Haskins wrote:
>   
>> Gleb Natapov wrote:
>>     
>>> On Mon, Jul 13, 2009 at 09:01:33AM -0400, Gregory Haskins wrote:
>>>   
>>>       
>>>> Gleb Natapov wrote:
>>>>     
>>>>         
>>>>> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>>>>> ---
>>>>>  include/linux/kvm_host.h |    2 +-
>>>>>  virt/kvm/irq_comm.c      |   55 +++++++++++++++++++++-------------------------
>>>>>  virt/kvm/kvm_main.c      |    1 -
>>>>>  3 files changed, 26 insertions(+), 32 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
>>>>> index f54a0d3..6756b3e 100644
>>>>> --- a/include/linux/kvm_host.h
>>>>> +++ b/include/linux/kvm_host.h
>>>>> @@ -161,7 +161,7 @@ struct kvm {
>>>>>  
>>>>>  	struct mutex irq_lock;
>>>>>  #ifdef CONFIG_HAVE_KVM_IRQCHIP
>>>>> -	struct list_head irq_routing; /* of kvm_kernel_irq_routing_entry */
>>>>> +	struct kvm_kernel_irq_routing_entry *irq_routing;
>>>>>  	struct hlist_head mask_notifier_list;
>>>>>  #endif
>>>>>  
>>>>> diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
>>>>> index 7af18b8..b2fa3f6 100644
>>>>> --- a/virt/kvm/irq_comm.c
>>>>> +++ b/virt/kvm/irq_comm.c
>>>>> @@ -148,7 +148,8 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, int irq, int level)
>>>>>  	 * IOAPIC.  So set the bit in both. The guest will ignore
>>>>>  	 * writes to the unused one.
>>>>>  	 */
>>>>> -	list_for_each_entry(e, &kvm->irq_routing, link)
>>>>> +	rcu_read_lock();
>>>>> +	for (e = rcu_dereference(kvm->irq_routing); e && e->set; e++) {
>>>>>   
>>>>>       
>>>>>           
>>>> Hi Gleb,
>>>>   I haven't had a chance to fully digest and review these patches, but
>>>> one thing I did notice is that you seem to be converting from a list to
>>>> an open-coded structure.  I am just curious why you made this design
>>>> decision instead of using the RCU variant of list?
>>>>
>>>>     
>>>>         
>>> It is not scary "open-coded structure" it's just an array :) As I responded
>>> to Michael the idea is to move msis out of irq_routing, make the array
>>> much smaller and either use gsi as an index in the array or use hash table
>>> instead looping over all entries. For now I can justify array as more
>>> cache friendly data structure as we scan it linearly.
>>>   
>>>       
>> Ok, but that might be a good thing to mention in the patch header ;)
>>
>>     
> What exactly?  Besides this is just an RFC. By the time it will be
> applied (if at all) I may do the change already :)
>   

Heh, thats fine.  FWIW, I would suggest this:

"the idea is to move msis out of irq_routing, make the array
much smaller and either use gsi as an index in the array or use hash table
instead looping over all entries. For now I can justify array as more
cache friendly data structure as we scan it linearly"

Otherwise, reviewers might be curious why you are not using list_X_rcu() ;)

Kind Regards,
-Greg




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]