From mboxrd@z Thu Jan  1 00:00:00 1970
From: Keir Fraser <keir.xen@gmail.com>
Subject: Re: [PATCH 2/2] x86: fix RCU locking in
 PHYSDEVOP_get_free_pirq
Date: Wed, 05 Sep 2012 13:33:25 +0100
Message-ID: <CC6D0525.3DCAE%keir.xen@gmail.com>
References: <504760D00200007800098D70@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <504760D00200007800098D70@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>, xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

On 05/09/2012 13:25, "Jan Beulich" <JBeulich@suse.com> wrote:

> Apart from properly pairing locks with unlocks, also reduce the lock
> scope - no need to do the copy_{from,to}_guest()-s inside the protected
> region.
> 
> I actually wonder whether the RCU locks are needed here at all.

If it's a path that only acts on current domain, then no.

 -- Keir

> Reported-by: Tim Deegan <tim@xen.org>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/physdev.c
> +++ b/xen/arch/x86/physdev.c
> @@ -698,13 +698,13 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>          struct physdev_get_free_pirq out;
>          struct domain *d;
>  
> -        d = rcu_lock_current_domain();
> -        
>          ret = -EFAULT;
>          if ( copy_from_guest(&out, arg, 1) != 0 )
>              break;
>  
> +        d = rcu_lock_current_domain();
>          spin_lock(&d->event_lock);
> +
>          ret = get_free_pirq(d, out.type);
>          if ( ret >= 0 )
>          {
> @@ -715,7 +715,9 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>              else
>                  ret = -ENOMEM;
>          }
> +
>          spin_unlock(&d->event_lock);
> +        rcu_unlock_domain(d);
>  
>          if ( ret >= 0 )
>          {
> @@ -723,7 +725,6 @@ ret_t do_physdev_op(int cmd, XEN_GUEST_H
>              ret = copy_to_guest(arg, &out, 1) ? -EFAULT : 0;
>          }
>  
> -        rcu_unlock_domain(d);
>          break;
>      }
>      default:
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel