|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v3 2/5] lib/ukring: Fix formatting issues {ring.c, ring.h}
Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>
On 7/23/20 3:49 PM, Alexander Jung wrote:
> From: Alexander Jung <alexander.jung@xxxxxxxxx>
>
> This commit addresses issues found using checkpatch:
>
> * return is not a function, parentheses are not required;
> * trailing whitespace;
> * space required before the open parenthesis '(';
> * return of an errno should typically be negative; and,
> * missing a blank line after declarations.
>
> Signed-off-by: Alexander Jung <alexander.jung@xxxxxxxxx>
> ---
> lib/ukring/include/uk/ring.h | 140
> ++++++++++++++++++++++++-------------------
> lib/ukring/ring.c | 17 +++---
> 2 files changed, 87 insertions(+), 70 deletions(-)
>
> diff --git a/lib/ukring/include/uk/ring.h b/lib/ukring/include/uk/ring.h
> index 899563c..abc95fe 100644
> --- a/lib/ukring/include/uk/ring.h
> +++ b/lib/ukring/include/uk/ring.h
> @@ -29,8 +29,8 @@
> *
> */
>
> -#ifndef _SYS_BUF_RING_H_
> -#define _SYS_BUF_RING_H_
> +#ifndef _SYS_BUF_RING_H_
> +#define _SYS_BUF_RING_H_
>
> #include <machine/cpu.h>
>
> @@ -39,22 +39,24 @@
> #include <sys/mutex.h>
> #endif
>
> +
> struct buf_ring {
> - volatile uint32_t br_prod_head;
> - volatile uint32_t br_prod_tail;
> - int br_prod_size;
> - int br_prod_mask;
> - uint64_t br_drops;
> - volatile uint32_t br_cons_head __aligned(CACHE_LINE_SIZE);
> - volatile uint32_t br_cons_tail;
> - int br_cons_size;
> - int br_cons_mask;
> + volatile uint32_t br_prod_head;
> + volatile uint32_t br_prod_tail;
> + int br_prod_size;
> + int br_prod_mask;
> + uint64_t br_drops;
> + volatile uint32_t br_cons_head __aligned(CACHE_LINE_SIZE);
> + volatile uint32_t br_cons_tail;
> + int br_cons_size;
> + int br_cons_mask;
> #ifdef DEBUG_BUFRING
> - struct mtx *br_lock;
> -#endif
> - void *br_ring[0] __aligned(CACHE_LINE_SIZE);
> + struct mtx *br_lock;
> +#endif
> + void *br_ring[0] __aligned(CACHE_LINE_SIZE);
> };
>
> +
> /*
> * multi-producer safe lock-free ring buffer enqueue
> *
> @@ -63,6 +65,7 @@ static __inline int
> buf_ring_enqueue(struct buf_ring *br, void *buf)
> {
> uint32_t prod_head, prod_next, cons_tail;
> +
> #ifdef DEBUG_BUFRING
> int i;
>
> @@ -72,12 +75,14 @@ buf_ring_enqueue(struct buf_ring *br, void *buf)
> * trigger a spurious panic.
> */
> for (i = br->br_cons_head; i != br->br_prod_head;
> - i = ((i + 1) & br->br_cons_mask))
> - if(br->br_ring[i] == buf)
> + i = ((i + 1) & br->br_cons_mask))
> + if (br->br_ring[i] == buf)
> panic("buf=%p already enqueue at %d prod=%d cons=%d",
> - buf, i, br->br_prod_tail, br->br_cons_tail);
> -#endif
> + buf, i, br->br_prod_tail,
> br->br_cons_tail);
> +#endif
> +
> critical_enter();
> +
> do {
> prod_head = br->br_prod_head;
> prod_next = (prod_head + 1) & br->br_prod_mask;
> @@ -85,31 +90,34 @@ buf_ring_enqueue(struct buf_ring *br, void *buf)
>
> if (prod_next == cons_tail) {
> rmb();
> - if (prod_head == br->br_prod_head &&
> - cons_tail == br->br_cons_tail) {
> + if (prod_head == br->br_prod_head && cons_tail ==
> br->br_cons_tail) {
> br->br_drops++;
> critical_exit();
> - return (ENOBUFS);
> + return -ENOBUFS;
> }
> continue;
> }
> } while (!atomic_cmpset_acq_int(&br->br_prod_head, prod_head,
> prod_next));
> +
> #ifdef DEBUG_BUFRING
> if (br->br_ring[prod_head] != NULL)
> panic("dangling value in enqueue");
> -#endif
> +#endif
> +
> br->br_ring[prod_head] = buf;
>
> /*
> * If there are other enqueues in progress
> * that preceded us, we need to wait for them
> * to complete
> - */
> + */
> while (br->br_prod_tail != prod_head)
> cpu_spinwait();
> +
> atomic_store_rel_int(&br->br_prod_tail, prod_next);
> critical_exit();
> - return (0);
> +
> + return 0;
> }
>
> /*
> @@ -123,32 +131,35 @@ buf_ring_dequeue_mc(struct buf_ring *br)
> void *buf;
>
> critical_enter();
> +
> do {
> cons_head = br->br_cons_head;
> cons_next = (cons_head + 1) & br->br_cons_mask;
>
> if (cons_head == br->br_prod_tail) {
> critical_exit();
> - return (NULL);
> + return NULL;
> }
> } while (!atomic_cmpset_acq_int(&br->br_cons_head, cons_head,
> cons_next));
>
> buf = br->br_ring[cons_head];
> +
> #ifdef DEBUG_BUFRING
> br->br_ring[cons_head] = NULL;
> #endif
> +
> /*
> * If there are other dequeues in progress
> * that preceded us, we need to wait for them
> - * to complete
> - */
> + * to complete
> + */
> while (br->br_cons_tail != cons_head)
> cpu_spinwait();
>
> atomic_store_rel_int(&br->br_cons_tail, cons_next);
> critical_exit();
>
> - return (buf);
> + return buf;
> }
>
> /*
> @@ -186,45 +197,49 @@ buf_ring_dequeue_sc(struct buf_ring *br)
> *
> prod_tail = br->br_prod_tail;
> *
> if (cons_head == prod_tail)
> *
> return (NULL);
> - *
> <condition is false and code uses invalid(old) buf>`
> + *
> <condition is false and code uses invalid(old) buf>`
> *
> * <1> Load (on core 1) from br->br_ring[cons_head] can be reordered
> (speculative readed) by CPU.
> - */
> + */
> #if defined(__arm__) || defined(__aarch64__)
> cons_head = atomic_load_acq_32(&br->br_cons_head);
> #else
> cons_head = br->br_cons_head;
> #endif
> prod_tail = atomic_load_acq_32(&br->br_prod_tail);
> -
> +
> cons_next = (cons_head + 1) & br->br_cons_mask;
> #ifdef PREFETCH_DEFINED
> cons_next_next = (cons_head + 2) & br->br_cons_mask;
> #endif
> -
> - if (cons_head == prod_tail)
> - return (NULL);
>
> -#ifdef PREFETCH_DEFINED
> - if (cons_next != prod_tail) {
> + if (cons_head == prod_tail)
> + return NULL;
> +
> +#ifdef PREFETCH_DEFINED
> + if (cons_next != prod_tail) {
> prefetch(br->br_ring[cons_next]);
> - if (cons_next_next != prod_tail)
> + if (cons_next_next != prod_tail)
> prefetch(br->br_ring[cons_next_next]);
> }
> #endif
> +
> br->br_cons_head = cons_next;
> buf = br->br_ring[cons_head];
>
> #ifdef DEBUG_BUFRING
> br->br_ring[cons_head] = NULL;
> +
> if (!mtx_owned(br->br_lock))
> panic("lock not held on single consumer dequeue");
> +
> if (br->br_cons_tail != cons_head)
> panic("inconsistent list cons_tail=%d cons_head=%d",
> - br->br_cons_tail, cons_head);
> + br->br_cons_tail, cons_head);
> #endif
> +
> br->br_cons_tail = cons_next;
> - return (buf);
> + return buf;
> }
>
> /*
> @@ -237,17 +252,20 @@ buf_ring_advance_sc(struct buf_ring *br)
> {
> uint32_t cons_head, cons_next;
> uint32_t prod_tail;
> -
> +
> cons_head = br->br_cons_head;
> prod_tail = br->br_prod_tail;
> -
> cons_next = (cons_head + 1) & br->br_cons_mask;
> - if (cons_head == prod_tail)
> +
> + if (cons_head == prod_tail)
> return;
> +
> br->br_cons_head = cons_next;
> +
> #ifdef DEBUG_BUFRING
> br->br_ring[cons_head] = NULL;
> #endif
> +
> br->br_cons_tail = cons_next;
> }
>
> @@ -270,8 +288,8 @@ buf_ring_advance_sc(struct buf_ring *br)
> static __inline void
> buf_ring_putback_sc(struct buf_ring *br, void *new)
> {
> - KASSERT(br->br_cons_head != br->br_prod_tail,
> - ("Buf-Ring has none in putback")) ;
> + KASSERT(br->br_cons_head != br->br_prod_tail,
> + ("Buf-Ring has none in putback"));
> br->br_ring[br->br_cons_head] = new;
> }
>
> @@ -283,11 +301,11 @@ buf_ring_putback_sc(struct buf_ring *br, void *new)
> static __inline void *
> buf_ring_peek(struct buf_ring *br)
> {
> -
> #ifdef DEBUG_BUFRING
> if ((br->br_lock != NULL) && !mtx_owned(br->br_lock))
> panic("lock not held on single consumer dequeue");
> -#endif
> +#endif
> +
> /*
> * I believe it is safe to not have a memory barrier
> * here because we control cons and tail is worst case
> @@ -295,9 +313,9 @@ buf_ring_peek(struct buf_ring *br)
> * return NULL immediately after a buffer has been enqueued
> */
> if (br->br_cons_head == br->br_prod_tail)
> - return (NULL);
> -
> - return (br->br_ring[br->br_cons_head]);
> + return NULL;
> +
> + return br->br_ring[br->br_cons_head];
> }
>
> static __inline void *
> @@ -308,10 +326,10 @@ buf_ring_peek_clear_sc(struct buf_ring *br)
>
> if (!mtx_owned(br->br_lock))
> panic("lock not held on single consumer dequeue");
> -#endif
> +#endif
>
> if (br->br_cons_head == br->br_prod_tail)
> - return (NULL);
> + return NULL;
>
> #if defined(__arm__) || defined(__aarch64__)
> /*
> @@ -334,38 +352,34 @@ buf_ring_peek_clear_sc(struct buf_ring *br)
> */
> ret = br->br_ring[br->br_cons_head];
> br->br_ring[br->br_cons_head] = NULL;
> - return (ret);
> +
> + return ret;
> #else
> - return (br->br_ring[br->br_cons_head]);
> + return br->br_ring[br->br_cons_head];
> #endif
> }
>
> static __inline int
> buf_ring_full(struct buf_ring *br)
> {
> -
> - return (((br->br_prod_head + 1) & br->br_prod_mask) ==
> br->br_cons_tail);
> + return ((br->br_prod_head + 1) & br->br_prod_mask) == br->br_cons_tail;
> }
>
> static __inline int
> buf_ring_empty(struct buf_ring *br)
> {
> -
> - return (br->br_cons_head == br->br_prod_tail);
> + return br->br_cons_head == br->br_prod_tail;
> }
>
> static __inline int
> buf_ring_count(struct buf_ring *br)
> {
> -
> - return ((br->br_prod_size + br->br_prod_tail - br->br_cons_tail)
> - & br->br_prod_mask);
> + return (br->br_prod_size + br->br_prod_tail - br->br_cons_tail)
> + & br->br_prod_mask;
> }
>
> struct buf_ring *buf_ring_alloc(int count, struct malloc_type *type, int
> flags,
> - struct mtx *);
> + struct mtx *);
> void buf_ring_free(struct buf_ring *br, struct malloc_type *type);
>
> -
> -
> #endif
> \ No newline at end of file
> diff --git a/lib/ukring/ring.c b/lib/ukring/ring.c
> index 644c3be..e327bd7 100644
> --- a/lib/ukring/ring.c
> +++ b/lib/ukring/ring.c
> @@ -42,20 +42,23 @@ buf_ring_alloc(int count, struct malloc_type *type, int
> flags, struct mtx *lock)
> struct buf_ring *br;
>
> KASSERT(powerof2(count), ("buf ring must be size power of 2"));
> -
> +
> br = malloc(sizeof(struct buf_ring) + count*sizeof(caddr_t),
> - type, flags|M_ZERO);
> + type, flags|M_ZERO);
> +
> if (br == NULL)
> - return (NULL);
> + return NULL;
> +
> #ifdef DEBUG_BUFRING
> br->br_lock = lock;
> -#endif
> +#endif
> +
> br->br_prod_size = br->br_cons_size = count;
> - br->br_prod_mask = br->br_cons_mask = count-1;
> + br->br_prod_mask = br->br_cons_mask = count - 1;
> br->br_prod_head = br->br_cons_head = 0;
> br->br_prod_tail = br->br_cons_tail = 0;
> -
> - return (br);
> +
> + return br;
> }
>
> void
>
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |