Re: [Xen-devel] [PATCH RFC 7/7] tools/tests: Remove superfluous and incomplete spinlock from xen-access

2014-11-28 Thread Razvan Cojocaru 
On 11/12/2014 05:31 PM, Tamas K Lengyel wrote:
 The spin-lock implementation in the xen-access test program is implemented
 in a fashion that is actually incomplete. The x86 assembly that guarantees 
 that
 the lock is held by only one thread lacks the lock; instruction.
 
 However, the spin-lock is not actually necessary in xen-access as it is not
 multithreaded. The presence of the faulty implementation of the lock in a non-
 mulithreaded environment is unnecessarily complicated for developers who are
 trying to follow this code as a guide in implementing their own applications.
 Thus, removing it from the code improves the clarity on the behavior of the
 system.
 
 Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com
 ---
  tools/tests/xen-access/xen-access.c | 68 
 -
  1 file changed, 6 insertions(+), 62 deletions(-)
 
 diff --git a/tools/tests/xen-access/xen-access.c 
 b/tools/tests/xen-access/xen-access.c
 index 3538323..428c459 100644
 --- a/tools/tests/xen-access/xen-access.c
 +++ b/tools/tests/xen-access/xen-access.c
 @@ -45,56 +45,6 @@
  #define ERROR(a, b...) fprintf(stderr, a \n, ## b)
  #define PERROR(a, b...) fprintf(stderr, a : %s\n, ## b, strerror(errno))
  
 -/* Spinlock and mem event definitions */
 -
 -#define SPIN_LOCK_UNLOCKED 0
 -
 -#define ADDR (*(volatile long *) addr)
 -/**
 - * test_and_set_bit - Set a bit and return its old value
 - * @nr: Bit to set
 - * @addr: Address to count from
 - *
 - * This operation is atomic and cannot be reordered.
 - * It also implies a memory barrier.
 - */
 -static inline int test_and_set_bit(int nr, volatile void *addr)
 -{
 -int oldbit;
 -
 -asm volatile (
 -btsl %2,%1\n\tsbbl %0,%0
 -: =r (oldbit), =m (ADDR)
 -: Ir (nr), m (ADDR) : memory);
 -return oldbit;
 -}
 -
 -typedef int spinlock_t;
 -
 -static inline void spin_lock(spinlock_t *lock)
 -{
 -while ( test_and_set_bit(1, lock) );
 -}
 -
 -static inline void spin_lock_init(spinlock_t *lock)
 -{
 -*lock = SPIN_LOCK_UNLOCKED;
 -}
 -
 -static inline void spin_unlock(spinlock_t *lock)
 -{
 -*lock = SPIN_LOCK_UNLOCKED;
 -}
 -
 -static inline int spin_trylock(spinlock_t *lock)
 -{
 -return !test_and_set_bit(1, lock);
 -}
 -
 -#define vm_event_ring_lock_init(_m)  spin_lock_init((_m)-ring_lock)
 -#define vm_event_ring_lock(_m)   spin_lock((_m)-ring_lock)
 -#define vm_event_ring_unlock(_m) spin_unlock((_m)-ring_lock)
 -
  typedef struct vm_event {
  domid_t domain_id;
  xc_evtchn *xce_handle;
 @@ -102,7 +52,6 @@ typedef struct vm_event {
  vm_event_back_ring_t back_ring;
  uint32_t evtchn_port;
  void *ring_page;
 -spinlock_t ring_lock;
  } vm_event_t;
  
  typedef struct xenaccess {
 @@ -241,9 +190,6 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t 
 domain_id)
  /* Set domain id */
  xenaccess-vm_event.domain_id = domain_id;
  
 -/* Initialise lock */
 -vm_event_ring_lock_init(xenaccess-vm_event);
 -
  /* Enable mem_access */
  xenaccess-vm_event.ring_page =
  xc_mem_access_enable(xenaccess-xc_handle,
 @@ -320,13 +266,14 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, 
 domid_t domain_id)
  return NULL;
  }
  
 +/*
 + * Note that this function is not thread safe.
 + */
  int get_request(vm_event_t *vm_event, vm_event_request_t *req)
  {
  vm_event_back_ring_t *back_ring;
  RING_IDX req_cons;
  
 -vm_event_ring_lock(vm_event);
 -
  back_ring = vm_event-back_ring;
  req_cons = back_ring-req_cons;
  
 @@ -338,18 +285,17 @@ int get_request(vm_event_t *vm_event, 
 vm_event_request_t *req)
  back_ring-req_cons = req_cons;
  back_ring-sring-req_event = req_cons + 1;
  
 -vm_event_ring_unlock(vm_event);
 -
  return 0;
  }
  
 +/*
 + * Note that this function is not thread safe.
 + */
  static int put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
  {
  vm_event_back_ring_t *back_ring;
  RING_IDX rsp_prod;
  
 -vm_event_ring_lock(vm_event);
 -
  back_ring = vm_event-back_ring;
  rsp_prod = back_ring-rsp_prod_pvt;
  
 @@ -361,8 +307,6 @@ static int put_response(vm_event_t *vm_event, 
 vm_event_response_t *rsp)
  back_ring-rsp_prod_pvt = rsp_prod;
  RING_PUSH_RESPONSES(back_ring);
  
 -vm_event_ring_unlock(vm_event);
 -
  return 0;
  }

I've just now noticed that get_request() and put_response() only ever
return 0, so it makes no sense to check the return values later on, and
they should basically either be modified to be void functions or some
error checking should be added (not sure what that could be).


Regards,
Razvan

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

[Xen-devel] [PATCH RFC 7/7] tools/tests: Remove superfluous and incomplete spinlock from xen-access

2014-11-12 Thread Tamas K Lengyel 
The spin-lock implementation in the xen-access test program is implemented
in a fashion that is actually incomplete. The x86 assembly that guarantees that
the lock is held by only one thread lacks the lock; instruction.

However, the spin-lock is not actually necessary in xen-access as it is not
multithreaded. The presence of the faulty implementation of the lock in a non-
mulithreaded environment is unnecessarily complicated for developers who are
trying to follow this code as a guide in implementing their own applications.
Thus, removing it from the code improves the clarity on the behavior of the
system.

Signed-off-by: Tamas K Lengyel tamas.leng...@zentific.com
---
 tools/tests/xen-access/xen-access.c | 68 -
 1 file changed, 6 insertions(+), 62 deletions(-)

diff --git a/tools/tests/xen-access/xen-access.c 
b/tools/tests/xen-access/xen-access.c
index 3538323..428c459 100644
--- a/tools/tests/xen-access/xen-access.c
+++ b/tools/tests/xen-access/xen-access.c
@@ -45,56 +45,6 @@
 #define ERROR(a, b...) fprintf(stderr, a \n, ## b)
 #define PERROR(a, b...) fprintf(stderr, a : %s\n, ## b, strerror(errno))
 
-/* Spinlock and mem event definitions */
-
-#define SPIN_LOCK_UNLOCKED 0
-
-#define ADDR (*(volatile long *) addr)
-/**
- * test_and_set_bit - Set a bit and return its old value
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This operation is atomic and cannot be reordered.
- * It also implies a memory barrier.
- */
-static inline int test_and_set_bit(int nr, volatile void *addr)
-{
-int oldbit;
-
-asm volatile (
-btsl %2,%1\n\tsbbl %0,%0
-: =r (oldbit), =m (ADDR)
-: Ir (nr), m (ADDR) : memory);
-return oldbit;
-}
-
-typedef int spinlock_t;
-
-static inline void spin_lock(spinlock_t *lock)
-{
-while ( test_and_set_bit(1, lock) );
-}
-
-static inline void spin_lock_init(spinlock_t *lock)
-{
-*lock = SPIN_LOCK_UNLOCKED;
-}
-
-static inline void spin_unlock(spinlock_t *lock)
-{
-*lock = SPIN_LOCK_UNLOCKED;
-}
-
-static inline int spin_trylock(spinlock_t *lock)
-{
-return !test_and_set_bit(1, lock);
-}
-
-#define vm_event_ring_lock_init(_m)  spin_lock_init((_m)-ring_lock)
-#define vm_event_ring_lock(_m)   spin_lock((_m)-ring_lock)
-#define vm_event_ring_unlock(_m) spin_unlock((_m)-ring_lock)
-
 typedef struct vm_event {
 domid_t domain_id;
 xc_evtchn *xce_handle;
@@ -102,7 +52,6 @@ typedef struct vm_event {
 vm_event_back_ring_t back_ring;
 uint32_t evtchn_port;
 void *ring_page;
-spinlock_t ring_lock;
 } vm_event_t;
 
 typedef struct xenaccess {
@@ -241,9 +190,6 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t 
domain_id)
 /* Set domain id */
 xenaccess-vm_event.domain_id = domain_id;
 
-/* Initialise lock */
-vm_event_ring_lock_init(xenaccess-vm_event);
-
 /* Enable mem_access */
 xenaccess-vm_event.ring_page =
 xc_mem_access_enable(xenaccess-xc_handle,
@@ -320,13 +266,14 @@ xenaccess_t *xenaccess_init(xc_interface **xch_r, domid_t 
domain_id)
 return NULL;
 }
 
+/*
+ * Note that this function is not thread safe.
+ */
 int get_request(vm_event_t *vm_event, vm_event_request_t *req)
 {
 vm_event_back_ring_t *back_ring;
 RING_IDX req_cons;
 
-vm_event_ring_lock(vm_event);
-
 back_ring = vm_event-back_ring;
 req_cons = back_ring-req_cons;
 
@@ -338,18 +285,17 @@ int get_request(vm_event_t *vm_event, vm_event_request_t 
*req)
 back_ring-req_cons = req_cons;
 back_ring-sring-req_event = req_cons + 1;
 
-vm_event_ring_unlock(vm_event);
-
 return 0;
 }
 
+/*
+ * Note that this function is not thread safe.
+ */
 static int put_response(vm_event_t *vm_event, vm_event_response_t *rsp)
 {
 vm_event_back_ring_t *back_ring;
 RING_IDX rsp_prod;
 
-vm_event_ring_lock(vm_event);
-
 back_ring = vm_event-back_ring;
 rsp_prod = back_ring-rsp_prod_pvt;
 
@@ -361,8 +307,6 @@ static int put_response(vm_event_t *vm_event, 
vm_event_response_t *rsp)
 back_ring-rsp_prod_pvt = rsp_prod;
 RING_PUSH_RESPONSES(back_ring);
 
-vm_event_ring_unlock(vm_event);
-
 return 0;
 }
 
-- 
2.1.1


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