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

[Razvan Cojocaru <rcojocaru@bitdefender.com>]

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.lengyel@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