Re: [Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Eric DeVolder]

On 04/19/2017 11:21 AM, Jan Beulich wrote:

On 19.04.17 at 17:47,  wrote:

@@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
*new,
 if ( kexec_load_get_bits(type, , ) )
 return -EINVAL;

-spin_lock(_lock);
+ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) );


Did you test this (in a debug build)? I ask because it looks to me that
you've inverted the condition.


You are correct, I inverted the condition. I had found this shortly 
after posting, and have corrected and have tested with Config.mk:debug=y


I will repost patch.

Thanks,
eric



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

Re: [Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Jan Beulich]

>>> On 19.04.17 at 17:47,  wrote:
> @@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
> *new,
>  if ( kexec_load_get_bits(type, , ) )
>  return -EINVAL;
>  
> -spin_lock(_lock);
> +ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) );

Did you test this (in a debug build)? I ask because it looks to me that
you've inverted the condition.

Jan


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

Re: [Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Eric DeVolder]

On 04/19/2017 10:47 AM, Eric DeVolder wrote:

The spinlock in kexec_swap_images() was removed as
this function is only reachable on the kexec hypercall, which is
now protected at the top-level in do_kexec_op_internal(),
thus the local spinlock is no longer necessary.

Per recommendation from Jan Beulich and Andrew Cooper, I left
an ASSERT in place of the spin_lock().

Signed-off-by: Eric DeVolder 
Reviewed-by: Bhavesh Davda 
Reviewed-by: Konrad Rzeszutek Wilk 
---
v3:
 - Incorporated feedback from Jan Beulich and Andrew Cooper
   to leave an ASSERT where spin_lock() once was.

v2: 04/17/2017
 - Patch titled 'kexec: use hypercall_create_continuation to protect KEXEC ops'
 - Separated removal of spinlock in kexec_swap_images() into its own patch.

v1: 04/10/2017
 - Patch titled 'kexec: Add spinlock for the whole hypercall'
 - Removal of spinlock in kexec_swap_images() was part of other patch.
---
 xen/common/kexec.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 253c204..af32e58 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -820,7 +820,6 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
 static int kexec_swap_images(int type, struct kexec_image *new,
  struct kexec_image **old)
 {
-static DEFINE_SPINLOCK(kexec_lock);
 int base, bit, pos;
 int new_slot, old_slot;

@@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
*new,
 if ( kexec_load_get_bits(type, , ) )
 return -EINVAL;

-spin_lock(_lock);
+ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) );


There are two problems here: the spaces need to be eliminated, and more 
importantly, the sense/polarity of the expression is wrong, the '!' 
needs to be removed.




 pos = (test_bit(bit, _flags) != 0);
 old_slot = base + pos;
@@ -846,8 +845,6 @@ static int kexec_swap_images(int type, struct kexec_image 
*new,
 clear_bit(old_slot, _flags);
 *old = kexec_image[old_slot];

-spin_unlock(_lock);
-
 return 0;
 }





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

Re: [Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Daniel Kiper]

On Wed, Apr 19, 2017 at 10:47:16AM -0500, Eric DeVolder wrote:
> The spinlock in kexec_swap_images() was removed as
> this function is only reachable on the kexec hypercall, which is
> now protected at the top-level in do_kexec_op_internal(),
> thus the local spinlock is no longer necessary.
>
> Per recommendation from Jan Beulich and Andrew Cooper, I left
> an ASSERT in place of the spin_lock().
>
> Signed-off-by: Eric DeVolder 
> Reviewed-by: Bhavesh Davda 
> Reviewed-by: Konrad Rzeszutek Wilk 
> ---
> v3:
>  - Incorporated feedback from Jan Beulich and Andrew Cooper
>to leave an ASSERT where spin_lock() once was.
>
> v2: 04/17/2017
>  - Patch titled 'kexec: use hypercall_create_continuation to protect KEXEC 
> ops'
>  - Separated removal of spinlock in kexec_swap_images() into its own patch.
>
> v1: 04/10/2017
>  - Patch titled 'kexec: Add spinlock for the whole hypercall'
>  - Removal of spinlock in kexec_swap_images() was part of other patch.
> ---
>  xen/common/kexec.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/xen/common/kexec.c b/xen/common/kexec.c
> index 253c204..af32e58 100644
> --- a/xen/common/kexec.c
> +++ b/xen/common/kexec.c
> @@ -820,7 +820,6 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
>  static int kexec_swap_images(int type, struct kexec_image *new,
>   struct kexec_image **old)
>  {
> -static DEFINE_SPINLOCK(kexec_lock);
>  int base, bit, pos;
>  int new_slot, old_slot;
>
> @@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
> *new,
>  if ( kexec_load_get_bits(type, , ) )
>  return -EINVAL;
>
> -spin_lock(_lock);
> +ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) );

...and drop ASSERT() here.

Daniel

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

[Xen-devel] [PATCH v3 2/2] kexec: remove spinlock now that all KEXEC hypercall ops are protected at the top-level

[Eric DeVolder]

The spinlock in kexec_swap_images() was removed as
this function is only reachable on the kexec hypercall, which is
now protected at the top-level in do_kexec_op_internal(),
thus the local spinlock is no longer necessary.

Per recommendation from Jan Beulich and Andrew Cooper, I left
an ASSERT in place of the spin_lock().

Signed-off-by: Eric DeVolder 
Reviewed-by: Bhavesh Davda 
Reviewed-by: Konrad Rzeszutek Wilk 
---
v3:
 - Incorporated feedback from Jan Beulich and Andrew Cooper
   to leave an ASSERT where spin_lock() once was.

v2: 04/17/2017
 - Patch titled 'kexec: use hypercall_create_continuation to protect KEXEC ops'
 - Separated removal of spinlock in kexec_swap_images() into its own patch.

v1: 04/10/2017
 - Patch titled 'kexec: Add spinlock for the whole hypercall'
 - Removal of spinlock in kexec_swap_images() was part of other patch.
---
 xen/common/kexec.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/xen/common/kexec.c b/xen/common/kexec.c
index 253c204..af32e58 100644
--- a/xen/common/kexec.c
+++ b/xen/common/kexec.c
@@ -820,7 +820,6 @@ static int kexec_exec(XEN_GUEST_HANDLE_PARAM(void) uarg)
 static int kexec_swap_images(int type, struct kexec_image *new,
  struct kexec_image **old)
 {
-static DEFINE_SPINLOCK(kexec_lock);
 int base, bit, pos;
 int new_slot, old_slot;
 
@@ -832,7 +831,7 @@ static int kexec_swap_images(int type, struct kexec_image 
*new,
 if ( kexec_load_get_bits(type, , ) )
 return -EINVAL;
 
-spin_lock(_lock);
+ASSERT( !test_bit(KEXEC_FLAG_IN_HYPERCALL, _flags) );
 
 pos = (test_bit(bit, _flags) != 0);
 old_slot = base + pos;
@@ -846,8 +845,6 @@ static int kexec_swap_images(int type, struct kexec_image 
*new,
 clear_bit(old_slot, _flags);
 *old = kexec_image[old_slot];
 
-spin_unlock(_lock);
-
 return 0;
 }
 
-- 
2.7.4


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