xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] xen/arm: io: Protect the handlers with a read-write lock
@ 2016-06-28 16:34 Julien Grall
  2016-07-11 17:49 ` Stefano Stabellini
  0 siblings, 1 reply; 3+ messages in thread
From: Julien Grall @ 2016-06-28 16:34 UTC (permalink / raw)
  To: xen-devel; +Cc: Julien Grall, sstabellini, shankerd, wei.chen

Currently, accessing the I/O handlers does not require to take a lock
because new handlers are always added at the end of the array. In a
follow-up patch, this array will be sort to optimize the look up.

Given that most of the time the I/O handlers will not be modify,
using a spinlock will add contention when multiple vCPU are accessing
the emulated MMIOs. So use a read-write lock to protected the handlers.

Finally, take the opportunity to re-indent correctly domain_io_init.

Signed-off-by: Julien Grall <julien.grall@arm.com>
---
 xen/arch/arm/io.c          | 47 +++++++++++++++++++++++++++-------------------
 xen/include/asm-arm/mmio.h |  3 ++-
 2 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
index 0156755..5a96836 100644
--- a/xen/arch/arm/io.c
+++ b/xen/arch/arm/io.c
@@ -70,23 +70,39 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
                                handler->priv);
 }
 
-int handle_mmio(mmio_info_t *info)
+static const struct mmio_handler *find_mmio_handler(struct domain *d,
+                                                    paddr_t gpa)
 {
-    struct vcpu *v = current;
-    int i;
-    const struct mmio_handler *handler = NULL;
-    const struct vmmio *vmmio = &v->domain->arch.vmmio;
+    const struct mmio_handler *handler;
+    unsigned int i;
+    struct vmmio *vmmio = &d->arch.vmmio;
+
+    read_lock(&vmmio->lock);
 
     for ( i = 0; i < vmmio->num_entries; i++ )
     {
         handler = &vmmio->handlers[i];
 
-        if ( (info->gpa >= handler->addr) &&
-             (info->gpa < (handler->addr + handler->size)) )
+        if ( (gpa >= handler->addr) &&
+             (gpa < (handler->addr + handler->size)) )
             break;
     }
 
     if ( i == vmmio->num_entries )
+        handler = NULL;
+
+    read_unlock(&vmmio->lock);
+
+    return handler;
+}
+
+int handle_mmio(mmio_info_t *info)
+{
+    struct vcpu *v = current;
+    const struct mmio_handler *handler = NULL;
+
+    handler = find_mmio_handler(v->domain, info->gpa);
+    if ( !handler )
         return 0;
 
     if ( info->dabt.write )
@@ -104,7 +120,7 @@ void register_mmio_handler(struct domain *d,
 
     BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
 
-    spin_lock(&vmmio->lock);
+    write_lock(&vmmio->lock);
 
     handler = &vmmio->handlers[vmmio->num_entries];
 
@@ -113,24 +129,17 @@ void register_mmio_handler(struct domain *d,
     handler->size = size;
     handler->priv = priv;
 
-    /*
-     * handle_mmio is not using the lock to avoid contention.
-     * Make sure the other processors see the new handler before
-     * updating the number of entries
-     */
-    dsb(ish);
-
     vmmio->num_entries++;
 
-    spin_unlock(&vmmio->lock);
+    write_unlock(&vmmio->lock);
 }
 
 int domain_io_init(struct domain *d)
 {
-   spin_lock_init(&d->arch.vmmio.lock);
-   d->arch.vmmio.num_entries = 0;
+    rwlock_init(&d->arch.vmmio.lock);
+    d->arch.vmmio.num_entries = 0;
 
-   return 0;
+    return 0;
 }
 
 /*
diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
index da1cc2e..32f10f2 100644
--- a/xen/include/asm-arm/mmio.h
+++ b/xen/include/asm-arm/mmio.h
@@ -20,6 +20,7 @@
 #define __ASM_ARM_MMIO_H__
 
 #include <xen/lib.h>
+#include <xen/rwlock.h>
 #include <asm/processor.h>
 #include <asm/regs.h>
 
@@ -51,7 +52,7 @@ struct mmio_handler {
 
 struct vmmio {
     int num_entries;
-    spinlock_t lock;
+    rwlock_t lock;
     struct mmio_handler handlers[MAX_IO_HANDLER];
 };
 
-- 
1.9.1


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

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] xen/arm: io: Protect the handlers with a read-write lock
  2016-06-28 16:34 [PATCH] xen/arm: io: Protect the handlers with a read-write lock Julien Grall
@ 2016-07-11 17:49 ` Stefano Stabellini
  2016-07-11 18:07   ` Julien Grall
  0 siblings, 1 reply; 3+ messages in thread
From: Stefano Stabellini @ 2016-07-11 17:49 UTC (permalink / raw)
  To: Julien Grall; +Cc: sstabellini, wei.chen, shankerd, xen-devel

On Tue, 28 Jun 2016, Julien Grall wrote:
> Currently, accessing the I/O handlers does not require to take a lock
> because new handlers are always added at the end of the array. In a
> follow-up patch, this array will be sort to optimize the look up.
> 
> Given that most of the time the I/O handlers will not be modify,
> using a spinlock will add contention when multiple vCPU are accessing
> the emulated MMIOs. So use a read-write lock to protected the handlers.
> 
> Finally, take the opportunity to re-indent correctly domain_io_init.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>

I would appreciate if you could avoid mixing indentation changes with
other changes in the future.

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

I'll commit.


>  xen/arch/arm/io.c          | 47 +++++++++++++++++++++++++++-------------------
>  xen/include/asm-arm/mmio.h |  3 ++-
>  2 files changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
> index 0156755..5a96836 100644
> --- a/xen/arch/arm/io.c
> +++ b/xen/arch/arm/io.c
> @@ -70,23 +70,39 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
>                                 handler->priv);
>  }
>  
> -int handle_mmio(mmio_info_t *info)
> +static const struct mmio_handler *find_mmio_handler(struct domain *d,
> +                                                    paddr_t gpa)
>  {
> -    struct vcpu *v = current;
> -    int i;
> -    const struct mmio_handler *handler = NULL;
> -    const struct vmmio *vmmio = &v->domain->arch.vmmio;
> +    const struct mmio_handler *handler;
> +    unsigned int i;
> +    struct vmmio *vmmio = &d->arch.vmmio;
> +
> +    read_lock(&vmmio->lock);
>  
>      for ( i = 0; i < vmmio->num_entries; i++ )
>      {
>          handler = &vmmio->handlers[i];
>  
> -        if ( (info->gpa >= handler->addr) &&
> -             (info->gpa < (handler->addr + handler->size)) )
> +        if ( (gpa >= handler->addr) &&
> +             (gpa < (handler->addr + handler->size)) )
>              break;
>      }
>  
>      if ( i == vmmio->num_entries )
> +        handler = NULL;
> +
> +    read_unlock(&vmmio->lock);
> +
> +    return handler;
> +}
> +
> +int handle_mmio(mmio_info_t *info)
> +{
> +    struct vcpu *v = current;
> +    const struct mmio_handler *handler = NULL;
> +
> +    handler = find_mmio_handler(v->domain, info->gpa);
> +    if ( !handler )
>          return 0;
>  
>      if ( info->dabt.write )
> @@ -104,7 +120,7 @@ void register_mmio_handler(struct domain *d,
>  
>      BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
>  
> -    spin_lock(&vmmio->lock);
> +    write_lock(&vmmio->lock);
>  
>      handler = &vmmio->handlers[vmmio->num_entries];
>  
> @@ -113,24 +129,17 @@ void register_mmio_handler(struct domain *d,
>      handler->size = size;
>      handler->priv = priv;
>  
> -    /*
> -     * handle_mmio is not using the lock to avoid contention.
> -     * Make sure the other processors see the new handler before
> -     * updating the number of entries
> -     */
> -    dsb(ish);
> -
>      vmmio->num_entries++;
>  
> -    spin_unlock(&vmmio->lock);
> +    write_unlock(&vmmio->lock);
>  }
>  
>  int domain_io_init(struct domain *d)
>  {
> -   spin_lock_init(&d->arch.vmmio.lock);
> -   d->arch.vmmio.num_entries = 0;
> +    rwlock_init(&d->arch.vmmio.lock);
> +    d->arch.vmmio.num_entries = 0;
>  
> -   return 0;
> +    return 0;
>  }
>  
>  /*
> diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
> index da1cc2e..32f10f2 100644
> --- a/xen/include/asm-arm/mmio.h
> +++ b/xen/include/asm-arm/mmio.h
> @@ -20,6 +20,7 @@
>  #define __ASM_ARM_MMIO_H__
>  
>  #include <xen/lib.h>
> +#include <xen/rwlock.h>
>  #include <asm/processor.h>
>  #include <asm/regs.h>
>  
> @@ -51,7 +52,7 @@ struct mmio_handler {
>  
>  struct vmmio {
>      int num_entries;
> -    spinlock_t lock;
> +    rwlock_t lock;
>      struct mmio_handler handlers[MAX_IO_HANDLER];
>  };
>  
> -- 
> 1.9.1
> 

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] xen/arm: io: Protect the handlers with a read-write lock
  2016-07-11 17:49 ` Stefano Stabellini
@ 2016-07-11 18:07   ` Julien Grall
  0 siblings, 0 replies; 3+ messages in thread
From: Julien Grall @ 2016-07-11 18:07 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: wei.chen, shankerd, xen-devel

Hi Stefano,

On 11/07/16 18:49, Stefano Stabellini wrote:
> On Tue, 28 Jun 2016, Julien Grall wrote:
>> Currently, accessing the I/O handlers does not require to take a lock
>> because new handlers are always added at the end of the array. In a
>> follow-up patch, this array will be sort to optimize the look up.
>>
>> Given that most of the time the I/O handlers will not be modify,
>> using a spinlock will add contention when multiple vCPU are accessing
>> the emulated MMIOs. So use a read-write lock to protected the handlers.
>>
>> Finally, take the opportunity to re-indent correctly domain_io_init.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> I would appreciate if you could avoid mixing indentation changes with
> other changes in the future.

The indentation changes was very small and I did not feel it was 
necessary to have a separate patch for it. I tend to limit the number of 
patches unless it hides important changes.

Anyway, I will try to split coding style changes and functional changes 
in the future.

>
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
>
> I'll commit.

Thank you!

Regards,

>
>>   xen/arch/arm/io.c          | 47 +++++++++++++++++++++++++++-------------------
>>   xen/include/asm-arm/mmio.h |  3 ++-
>>   2 files changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/xen/arch/arm/io.c b/xen/arch/arm/io.c
>> index 0156755..5a96836 100644
>> --- a/xen/arch/arm/io.c
>> +++ b/xen/arch/arm/io.c
>> @@ -70,23 +70,39 @@ static int handle_write(const struct mmio_handler *handler, struct vcpu *v,
>>                                  handler->priv);
>>   }
>>
>> -int handle_mmio(mmio_info_t *info)
>> +static const struct mmio_handler *find_mmio_handler(struct domain *d,
>> +                                                    paddr_t gpa)
>>   {
>> -    struct vcpu *v = current;
>> -    int i;
>> -    const struct mmio_handler *handler = NULL;
>> -    const struct vmmio *vmmio = &v->domain->arch.vmmio;
>> +    const struct mmio_handler *handler;
>> +    unsigned int i;
>> +    struct vmmio *vmmio = &d->arch.vmmio;
>> +
>> +    read_lock(&vmmio->lock);
>>
>>       for ( i = 0; i < vmmio->num_entries; i++ )
>>       {
>>           handler = &vmmio->handlers[i];
>>
>> -        if ( (info->gpa >= handler->addr) &&
>> -             (info->gpa < (handler->addr + handler->size)) )
>> +        if ( (gpa >= handler->addr) &&
>> +             (gpa < (handler->addr + handler->size)) )
>>               break;
>>       }
>>
>>       if ( i == vmmio->num_entries )
>> +        handler = NULL;
>> +
>> +    read_unlock(&vmmio->lock);
>> +
>> +    return handler;
>> +}
>> +
>> +int handle_mmio(mmio_info_t *info)
>> +{
>> +    struct vcpu *v = current;
>> +    const struct mmio_handler *handler = NULL;
>> +
>> +    handler = find_mmio_handler(v->domain, info->gpa);
>> +    if ( !handler )
>>           return 0;
>>
>>       if ( info->dabt.write )
>> @@ -104,7 +120,7 @@ void register_mmio_handler(struct domain *d,
>>
>>       BUG_ON(vmmio->num_entries >= MAX_IO_HANDLER);
>>
>> -    spin_lock(&vmmio->lock);
>> +    write_lock(&vmmio->lock);
>>
>>       handler = &vmmio->handlers[vmmio->num_entries];
>>
>> @@ -113,24 +129,17 @@ void register_mmio_handler(struct domain *d,
>>       handler->size = size;
>>       handler->priv = priv;
>>
>> -    /*
>> -     * handle_mmio is not using the lock to avoid contention.
>> -     * Make sure the other processors see the new handler before
>> -     * updating the number of entries
>> -     */
>> -    dsb(ish);
>> -
>>       vmmio->num_entries++;
>>
>> -    spin_unlock(&vmmio->lock);
>> +    write_unlock(&vmmio->lock);
>>   }
>>
>>   int domain_io_init(struct domain *d)
>>   {
>> -   spin_lock_init(&d->arch.vmmio.lock);
>> -   d->arch.vmmio.num_entries = 0;
>> +    rwlock_init(&d->arch.vmmio.lock);
>> +    d->arch.vmmio.num_entries = 0;
>>
>> -   return 0;
>> +    return 0;
>>   }
>>
>>   /*
>> diff --git a/xen/include/asm-arm/mmio.h b/xen/include/asm-arm/mmio.h
>> index da1cc2e..32f10f2 100644
>> --- a/xen/include/asm-arm/mmio.h
>> +++ b/xen/include/asm-arm/mmio.h
>> @@ -20,6 +20,7 @@
>>   #define __ASM_ARM_MMIO_H__
>>
>>   #include <xen/lib.h>
>> +#include <xen/rwlock.h>
>>   #include <asm/processor.h>
>>   #include <asm/regs.h>
>>
>> @@ -51,7 +52,7 @@ struct mmio_handler {
>>
>>   struct vmmio {
>>       int num_entries;
>> -    spinlock_t lock;
>> +    rwlock_t lock;
>>       struct mmio_handler handlers[MAX_IO_HANDLER];
>>   };
>>
>> --
>> 1.9.1
>>
>

-- 
Julien Grall

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

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-11 18:07 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-28 16:34 [PATCH] xen/arm: io: Protect the handlers with a read-write lock Julien Grall
2016-07-11 17:49 ` Stefano Stabellini
2016-07-11 18:07   ` Julien Grall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).