DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking
@ 2018-11-13 17:27 Konstantin Ananyev
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 1/2] rwlock: introduce " Konstantin Ananyev
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Konstantin Ananyev @ 2018-11-13 17:27 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

This series targets 19.02 release

Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock()
and new UT test-case for it.

Konstantin Ananyev (2):
  rwlock: introduce 'try' semantics for RD and WR locking
  test: add new test-cases for rwlock autotest

 .../common/include/generic/rte_rwlock.h       |  54 +++
 lib/librte_eal/rte_eal_version.map            |   2 +
 test/test/test_rwlock.c                       | 405 +++++++++++++++++-
 3 files changed, 442 insertions(+), 19 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
@ 2018-11-13 17:27 ` Konstantin Ananyev
  2018-12-19  6:37   ` Honnappa Nagarahalli
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Konstantin Ananyev @ 2018-11-13 17:27 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

This patch targets 19.02 release.

Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 .../common/include/generic/rte_rwlock.h       | 54 +++++++++++++++++++
 lib/librte_eal/rte_eal_version.map            |  2 +
 2 files changed, 56 insertions(+)

diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h b/lib/librte_eal/common/include/generic/rte_rwlock.h
index 5751a0e6d..7e395781e 100644
--- a/lib/librte_eal/common/include/generic/rte_rwlock.h
+++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
@@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
 	}
 }
 
+/**
+ * try to take a read lock.
+ *
+ * @param rwl
+ *   A pointer to a rwlock structure.
+ * @return
+ *   - zero if the lock is successfully taken
+ *   - -EBUSY if lock could not be acquired for reading because a
+ *     writer holds the lock
+ */
+static inline __rte_experimental int
+rte_rwlock_read_trylock(rte_rwlock_t *rwl)
+{
+	int32_t x;
+	int success = 0;
+
+	while (success == 0) {
+		x = rwl->cnt;
+		/* write lock is held */
+		if (x < 0)
+			return -EBUSY;
+		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
+					      (uint32_t)x, (uint32_t)(x + 1));
+	}
+	return 0;
+}
+
 /**
  * Release a read lock.
  *
@@ -87,6 +114,33 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
 	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);
 }
 
+/**
+ * try to take a write lock.
+ *
+ * @param rwl
+ *   A pointer to a rwlock structure.
+ * @return
+ *   - zero if the lock is successfully taken
+ *   - -EBUSY if lock could not be acquired for writing because
+ *     it was already locked for reading or writing
+ */
+static inline __rte_experimental int
+rte_rwlock_write_trylock(rte_rwlock_t *rwl)
+{
+	int32_t x;
+	int success = 0;
+
+	while (success == 0) {
+		x = rwl->cnt;
+		/* a lock is held */
+		if (x != 0)
+			return -EBUSY;
+		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
+					      0, (uint32_t)-1);
+	}
+	return 0;
+}
+
 /**
  * Take a write lock. Loop until the lock is held.
  *
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 3fe78260d..8b1593dd8 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -355,6 +355,8 @@ EXPERIMENTAL {
 	rte_mp_request_async;
 	rte_mp_sendmsg;
 	rte_option_register;
+	rte_rwlock_read_trylock;
+	rte_rwlock_write_trylock;
 	rte_service_lcore_attr_get;
 	rte_service_lcore_attr_reset_all;
 	rte_service_may_be_active;
-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 1/2] rwlock: introduce " Konstantin Ananyev
@ 2018-11-13 17:27 ` Konstantin Ananyev
  2018-12-19  8:28   ` Gavin Hu (Arm Technology China)
  2018-12-19  2:39 ` [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Thomas Monjalon
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Konstantin Ananyev @ 2018-11-13 17:27 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

This patch targets 19.02 release.

Add few functional and perfomance tests
for rte_rwlock_read_trylock() and rte_rwlock_write_trylock().

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
 test/test/test_rwlock.c | 405 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 386 insertions(+), 19 deletions(-)

diff --git a/test/test/test_rwlock.c b/test/test/test_rwlock.c
index 29171c422..47caac9fb 100644
--- a/test/test/test_rwlock.c
+++ b/test/test/test_rwlock.c
@@ -4,8 +4,10 @@
 
 #include <stdio.h>
 #include <stdint.h>
+#include <inttypes.h>
 #include <unistd.h>
 #include <sys/queue.h>
+#include <string.h>
 
 #include <rte_common.h>
 #include <rte_memory.h>
@@ -22,29 +24,41 @@
 /*
  * rwlock test
  * ===========
- *
- * - There is a global rwlock and a table of rwlocks (one per lcore).
- *
- * - The test function takes all of these locks and launches the
- *   ``test_rwlock_per_core()`` function on each core (except the master).
- *
- *   - The function takes the global write lock, display something,
- *     then releases the global lock.
- *   - Then, it takes the per-lcore write lock, display something, and
- *     releases the per-core lock.
- *   - Finally, a read lock is taken during 100 ms, then released.
- *
- * - The main function unlocks the per-lcore locks sequentially and
- *   waits between each lock. This triggers the display of a message
- *   for each core, in the correct order.
- *
- *   Then, it tries to take the global write lock and display the last
- *   message. The autotest script checks that the message order is correct.
  */
 
+#define ITER_NUM	0x80
+
+#define TEST_SEC	5
+
 static rte_rwlock_t sl;
 static rte_rwlock_t sl_tab[RTE_MAX_LCORE];
 
+enum {
+	LC_TYPE_RDLOCK,
+	LC_TYPE_WRLOCK,
+};
+
+static struct {
+	rte_rwlock_t lock;
+	uint64_t tick;
+	volatile union {
+		uint8_t u8[RTE_CACHE_LINE_SIZE];
+		uint64_t u64[RTE_CACHE_LINE_SIZE / sizeof(uint64_t)];
+	} data;
+} __rte_cache_aligned try_rwlock_data;
+
+struct try_rwlock_lcore {
+	int32_t rc;
+	int32_t type;
+	struct {
+		uint64_t tick;
+		uint64_t fail;
+		uint64_t success;
+	} stat;
+} __rte_cache_aligned;
+
+static struct try_rwlock_lcore try_lcore_data[RTE_MAX_LCORE];
+
 static int
 test_rwlock_per_core(__attribute__((unused)) void *arg)
 {
@@ -65,8 +79,27 @@ test_rwlock_per_core(__attribute__((unused)) void *arg)
 	return 0;
 }
 
+/*
+ * - There is a global rwlock and a table of rwlocks (one per lcore).
+ *
+ * - The test function takes all of these locks and launches the
+ *   ``test_rwlock_per_core()`` function on each core (except the master).
+ *
+ *   - The function takes the global write lock, display something,
+ *     then releases the global lock.
+ *   - Then, it takes the per-lcore write lock, display something, and
+ *     releases the per-core lock.
+ *   - Finally, a read lock is taken during 100 ms, then released.
+ *
+ * - The main function unlocks the per-lcore locks sequentially and
+ *   waits between each lock. This triggers the display of a message
+ *   for each core, in the correct order.
+ *
+ *   Then, it tries to take the global write lock and display the last
+ *   message. The autotest script checks that the message order is correct.
+ */
 static int
-test_rwlock(void)
+rwlock_test1(void)
 {
 	int i;
 
@@ -98,4 +131,338 @@ test_rwlock(void)
 	return 0;
 }
 
+static int
+try_read(uint32_t lc)
+{
+	int32_t rc;
+	uint32_t i;
+
+	rc = rte_rwlock_read_trylock(&try_rwlock_data.lock);
+	if (rc != 0)
+		return rc;
+
+	for (i = 0; i != RTE_DIM(try_rwlock_data.data.u64); i++) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u64[i] != 0) {
+			printf("%s(%u) error: unexpected data pattern\n",
+				__func__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+	}
+
+	rte_rwlock_read_unlock(&try_rwlock_data.lock);
+	return rc;
+}
+
+static int
+try_write(uint32_t lc)
+{
+	int32_t rc;
+	uint32_t i, v;
+
+	v = RTE_MAX(lc % UINT8_MAX, 1U);
+
+	rc = rte_rwlock_write_trylock(&try_rwlock_data.lock);
+	if (rc != 0)
+		return rc;
+
+	/* update by bytes in reverese order */
+	for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u8[i] != 0) {
+			printf("%s:%d(%u) error: unexpected data pattern\n",
+				__func__, __LINE__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+
+		try_rwlock_data.data.u8[i] = v;
+	}
+
+	/* restore by bytes in reverese order */
+	for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u8[i] != v) {
+			printf("%s:%d(%u) error: unexpected data pattern\n",
+				__func__, __LINE__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+
+		try_rwlock_data.data.u8[i] = 0;
+	}
+
+	rte_rwlock_write_unlock(&try_rwlock_data.lock);
+	return rc;
+}
+
+static int
+try_read_lcore(__rte_unused void *data)
+{
+	int32_t rc;
+	uint32_t i, lc;
+	uint64_t ftm, stm, tm;
+	struct try_rwlock_lcore *lcd;
+
+	lc = rte_lcore_id();
+	lcd = try_lcore_data + lc;
+	lcd->type = LC_TYPE_RDLOCK;
+
+	ftm = try_rwlock_data.tick;
+	stm = rte_get_timer_cycles();
+
+	do {
+		for (i = 0; i != ITER_NUM; i++) {
+			rc = try_read(lc);
+			if (rc == 0)
+				lcd->stat.success++;
+			else if (rc == -EBUSY)
+				lcd->stat.fail++;
+			else
+				break;
+			rc = 0;
+		}
+		tm = rte_get_timer_cycles() - stm;
+	} while (tm < ftm && rc == 0);
+
+	lcd->rc = rc;
+	lcd->stat.tick = tm;
+	return rc;
+}
+
+static int
+try_write_lcore(__rte_unused void *data)
+{
+	int32_t rc;
+	uint32_t i, lc;
+	uint64_t ftm, stm, tm;
+	struct try_rwlock_lcore *lcd;
+
+	lc = rte_lcore_id();
+	lcd = try_lcore_data + lc;
+	lcd->type = LC_TYPE_WRLOCK;
+
+	ftm = try_rwlock_data.tick;
+	stm = rte_get_timer_cycles();
+
+	do {
+		for (i = 0; i != ITER_NUM; i++) {
+			rc = try_write(lc);
+			if (rc == 0)
+				lcd->stat.success++;
+			else if (rc == -EBUSY)
+				lcd->stat.fail++;
+			else
+				break;
+			rc = 0;
+		}
+		tm = rte_get_timer_cycles() - stm;
+	} while (tm < ftm && rc == 0);
+
+	lcd->rc = rc;
+	lcd->stat.tick = tm;
+	return rc;
+}
+
+static void
+print_try_lcore_stats(const struct try_rwlock_lcore *tlc, uint32_t lc)
+{
+	uint64_t f, s;
+
+	f = RTE_MAX(tlc->stat.fail, 1ULL);
+	s = RTE_MAX(tlc->stat.success, 1ULL);
+
+	printf("try_lcore_data[%u]={\n"
+		"\trc=%d,\n"
+		"\ttype=%s,\n"
+		"\tfail=%" PRIu64 ",\n"
+		"\tsuccess=%" PRIu64 ",\n"
+		"\tcycles=%" PRIu64 ",\n"
+		"\tcycles/op=%#Lf,\n"
+		"\tcycles/success=%#Lf,\n"
+		"\tsuccess/fail=%#Lf,\n"
+		"};\n",
+		lc,
+		tlc->rc,
+		tlc->type == LC_TYPE_RDLOCK ? "RDLOCK" : "WRLOCK",
+		tlc->stat.fail,
+		tlc->stat.success,
+		tlc->stat.tick,
+		(long double)tlc->stat.tick /
+		(tlc->stat.fail + tlc->stat.success),
+		(long double)tlc->stat.tick / s,
+		(long double)tlc->stat.success / f);
+}
+
+static void
+collect_try_lcore_stats(struct try_rwlock_lcore *tlc,
+	const struct try_rwlock_lcore *lc)
+{
+	tlc->stat.tick += lc->stat.tick;
+	tlc->stat.fail += lc->stat.fail;
+	tlc->stat.success += lc->stat.success;
+}
+
+/*
+ * Process collected results:
+ *  - check status
+ *  - collect and print statistics
+ */
+static int
+process_try_lcore_stats(void)
+{
+	int32_t rc;
+	uint32_t lc, rd, wr;
+	struct try_rwlock_lcore rlc, wlc;
+
+	memset(&rlc, 0, sizeof(rlc));
+	memset(&wlc, 0, sizeof(wlc));
+
+	rlc.type = LC_TYPE_RDLOCK;
+	wlc.type = LC_TYPE_WRLOCK;
+	rd = 0;
+	wr = 0;
+
+	rc = 0;
+	RTE_LCORE_FOREACH(lc) {
+		rc |= try_lcore_data[lc].rc;
+		if (try_lcore_data[lc].type == LC_TYPE_RDLOCK) {
+			collect_try_lcore_stats(&rlc, try_lcore_data + lc);
+			rd++;
+		} else {
+			collect_try_lcore_stats(&wlc, try_lcore_data + lc);
+			wr++;
+		}
+	}
+
+	if (rc == 0) {
+		RTE_LCORE_FOREACH(lc)
+			print_try_lcore_stats(try_lcore_data + lc, lc);
+
+		if (rd != 0) {
+			printf("aggregated stats for %u RDLOCK cores:\n", rd);
+			print_try_lcore_stats(&rlc, rd);
+		}
+
+		if (wr != 0) {
+			printf("aggregated stats for %u WRLOCK cores:\n", wr);
+			print_try_lcore_stats(&wlc, wr);
+		}
+	}
+
+	return rc;
+}
+
+static void
+try_test_reset(void)
+{
+	memset(&try_lcore_data, 0, sizeof(try_lcore_data));
+	memset(&try_rwlock_data, 0, sizeof(try_rwlock_data));
+	try_rwlock_data.tick = TEST_SEC * rte_get_tsc_hz();
+}
+
+/* all lcores grab RDLOCK */
+static int
+try_rwlock_test_rda(void)
+{
+	try_test_reset();
+
+	/* start read test on all avaialble lcores */
+	rte_eal_mp_remote_launch(try_read_lcore, NULL, CALL_MASTER);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+/* all slave lcores grab RDLOCK, master one grabs WRLOCK */
+static int
+try_rwlock_test_rds_wrm(void)
+{
+	try_test_reset();
+
+	rte_eal_mp_remote_launch(try_read_lcore, NULL, SKIP_MASTER);
+	try_write_lcore(NULL);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+/* master and even slave lcores grab RDLOCK, odd lcores grab WRLOCK */
+static int
+try_rwlock_test_rde_wro(void)
+{
+	uint32_t lc, mlc;
+
+	try_test_reset();
+
+	mlc = rte_get_master_lcore();
+
+	RTE_LCORE_FOREACH(lc) {
+		if (lc != mlc) {
+			if ((lc & 1) == 0)
+				rte_eal_remote_launch(try_read_lcore,
+						NULL, lc);
+			else
+				rte_eal_remote_launch(try_write_lcore,
+						NULL, lc);
+		}
+	}
+	try_read_lcore(NULL);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+static int
+test_rwlock(void)
+{
+	uint32_t i;
+	int32_t rc, ret;
+
+	static const struct {
+		const char *name;
+		int (*ftst)(void);
+	} test[] = {
+		{
+			.name = "rwlock_test1",
+			.ftst = rwlock_test1,
+		},
+		{
+			.name = "try_rwlock_test_rda",
+			.ftst = try_rwlock_test_rda,
+		},
+		{
+			.name = "try_rwlock_test_rds_wrm",
+			.ftst = try_rwlock_test_rds_wrm,
+		},
+		{
+			.name = "try_rwlock_test_rde_wro",
+			.ftst = try_rwlock_test_rde_wro,
+		},
+	};
+
+	ret = 0;
+	for (i = 0; i != RTE_DIM(test); i++) {
+		printf("starting test %s;\n", test[i].name);
+		rc = test[i].ftst();
+		printf("test %s completed with status %d\n", test[i].name, rc);
+		ret |= rc;
+	}
+
+	return ret;
+}
+
 REGISTER_TEST_COMMAND(rwlock_autotest, test_rwlock);
-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 1/2] rwlock: introduce " Konstantin Ananyev
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
@ 2018-12-19  2:39 ` Thomas Monjalon
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 " Konstantin Ananyev
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2018-12-19  2:39 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev, honnappa.nagarahalli, jerinj, Chao Zhu

13/11/2018 18:27, Konstantin Ananyev:
> This series targets 19.02 release
> 
> Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock()
> and new UT test-case for it.
> 
> Konstantin Ananyev (2):
>   rwlock: introduce 'try' semantics for RD and WR locking
>   test: add new test-cases for rwlock autotest

Any comment?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 1/2] rwlock: introduce " Konstantin Ananyev
@ 2018-12-19  6:37   ` Honnappa Nagarahalli
  2018-12-19  8:30     ` Gavin Hu (Arm Technology China)
                       ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Honnappa Nagarahalli @ 2018-12-19  6:37 UTC (permalink / raw)
  To: Konstantin Ananyev, dev
  Cc: nd, Honnappa Nagarahalli, Gavin Hu (Arm Technology China), nd

> 
> This patch targets 19.02 release.
> 
> Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  .../common/include/generic/rte_rwlock.h       | 54 +++++++++++++++++++
>  lib/librte_eal/rte_eal_version.map            |  2 +
>  2 files changed, 56 insertions(+)
> 
> diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h
> b/lib/librte_eal/common/include/generic/rte_rwlock.h
> index 5751a0e6d..7e395781e 100644
> --- a/lib/librte_eal/common/include/generic/rte_rwlock.h
> +++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
> @@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
>  	}
>  }
> 
> +/**
Please add the experimental API warning

> + * try to take a read lock.
> + *
> + * @param rwl
> + *   A pointer to a rwlock structure.
> + * @return
> + *   - zero if the lock is successfully taken
> + *   - -EBUSY if lock could not be acquired for reading because a
> + *     writer holds the lock
> + */
> +static inline __rte_experimental int
> +rte_rwlock_read_trylock(rte_rwlock_t *rwl) {
> +	int32_t x;
> +	int success = 0;
> +
> +	while (success == 0) {
> +		x = rwl->cnt;
> +		/* write lock is held */
> +		if (x < 0)
> +			return -EBUSY;
> +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> +					      (uint32_t)x, (uint32_t)(x + 1));
> +	}
> +	return 0;
> +}
> +
>  /**
>   * Release a read lock.
>   *
> @@ -87,6 +114,33 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
>  	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);  }
> 
> +/**
Please add the experimental API warning

> + * try to take a write lock.
> + *
> + * @param rwl
> + *   A pointer to a rwlock structure.
> + * @return
> + *   - zero if the lock is successfully taken
> + *   - -EBUSY if lock could not be acquired for writing because
> + *     it was already locked for reading or writing
> + */
> +static inline __rte_experimental int
> +rte_rwlock_write_trylock(rte_rwlock_t *rwl) {
> +	int32_t x;
> +	int success = 0;
> +
> +	while (success == 0) {
> +		x = rwl->cnt;
> +		/* a lock is held */
> +		if (x != 0)
> +			return -EBUSY;
> +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> +					      0, (uint32_t)-1);
> +	}
> +	return 0;
> +}
> +
>  /**
>   * Take a write lock. Loop until the lock is held.
>   *
> diff --git a/lib/librte_eal/rte_eal_version.map
> b/lib/librte_eal/rte_eal_version.map
> index 3fe78260d..8b1593dd8 100644
> --- a/lib/librte_eal/rte_eal_version.map
> +++ b/lib/librte_eal/rte_eal_version.map
> @@ -355,6 +355,8 @@ EXPERIMENTAL {
>  	rte_mp_request_async;
>  	rte_mp_sendmsg;
>  	rte_option_register;
> +	rte_rwlock_read_trylock;
> +	rte_rwlock_write_trylock;
I do not see the other RW lock APIs in this file.

>  	rte_service_lcore_attr_get;
>  	rte_service_lcore_attr_reset_all;
>  	rte_service_may_be_active;
> --
> 2.17.1

Other than the minor comments,
Reviewed-by: Honnappa Nagarahalli <Honnappa.nagarahalli@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest
  2018-11-13 17:27 ` [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
@ 2018-12-19  8:28   ` Gavin Hu (Arm Technology China)
  0 siblings, 0 replies; 17+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2018-12-19  8:28 UTC (permalink / raw)
  To: Konstantin Ananyev, dev



> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Konstantin Ananyev
> Sent: Wednesday, November 14, 2018 1:28 AM
> To: dev@dpdk.org
> Cc: Konstantin Ananyev <konstantin.ananyev@intel.com>
> Subject: [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest
>
> This patch targets 19.02 release.
>
> Add few functional and perfomance tests
> for rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
>
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
>  test/test/test_rwlock.c | 405
> ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 386 insertions(+), 19 deletions(-)
>
> diff --git a/test/test/test_rwlock.c b/test/test/test_rwlock.c index
> 29171c422..47caac9fb 100644
> --- a/test/test/test_rwlock.c
> +++ b/test/test/test_rwlock.c
> @@ -4,8 +4,10 @@
>
>  #include <stdio.h>
>  #include <stdint.h>
> +#include <inttypes.h>
>  #include <unistd.h>
>  #include <sys/queue.h>
> +#include <string.h>
>
>  #include <rte_common.h>
>  #include <rte_memory.h>
> @@ -22,29 +24,41 @@
>  /*
>   * rwlock test
>   * ===========
> - *
> - * - There is a global rwlock and a table of rwlocks (one per lcore).
> - *
> - * - The test function takes all of these locks and launches the
> - *   ``test_rwlock_per_core()`` function on each core (except the master).
> - *
> - *   - The function takes the global write lock, display something,
> - *     then releases the global lock.
> - *   - Then, it takes the per-lcore write lock, display something, and
> - *     releases the per-core lock.
> - *   - Finally, a read lock is taken during 100 ms, then released.
> - *
> - * - The main function unlocks the per-lcore locks sequentially and
> - *   waits between each lock. This triggers the display of a message
> - *   for each core, in the correct order.
> - *
> - *   Then, it tries to take the global write lock and display the last
> - *   message. The autotest script checks that the message order is correct.
>   */
As the description was moved to rwlock_test1, a new general description of
the tests is still required.
>
> +#define ITER_NUM0x80
> +
> +#define TEST_SEC5
> +
>  static rte_rwlock_t sl;
>  static rte_rwlock_t sl_tab[RTE_MAX_LCORE];
>
> +enum {
> +LC_TYPE_RDLOCK,
> +LC_TYPE_WRLOCK,
> +};
> +
> +static struct {
> +rte_rwlock_t lock;
> +uint64_t tick;
> +volatile union {
> +uint8_t u8[RTE_CACHE_LINE_SIZE];
> +uint64_t u64[RTE_CACHE_LINE_SIZE / sizeof(uint64_t)];
> +} data;
> +} __rte_cache_aligned try_rwlock_data;
> +
> +struct try_rwlock_lcore {
> +int32_t rc;
> +int32_t type;
> +struct {
> +uint64_t tick;
> +uint64_t fail;
> +uint64_t success;
> +} stat;
> +} __rte_cache_aligned;
> +
> +static struct try_rwlock_lcore try_lcore_data[RTE_MAX_LCORE];
> +
>  static int
>  test_rwlock_per_core(__attribute__((unused)) void *arg)  { @@ -65,8
> +79,27 @@ test_rwlock_per_core(__attribute__((unused)) void *arg)
>  return 0;
>  }
>
> +/*
> + * - There is a global rwlock and a table of rwlocks (one per lcore).
> + *
> + * - The test function takes all of these locks and launches the
> + *   ``test_rwlock_per_core()`` function on each core (except the master).
> + *
> + *   - The function takes the global write lock, display something,
> + *     then releases the global lock.
> + *   - Then, it takes the per-lcore write lock, display something, and
> + *     releases the per-core lock.
> + *   - Finally, a read lock is taken during 100 ms, then released.
> + *
> + * - The main function unlocks the per-lcore locks sequentially and
> + *   waits between each lock. This triggers the display of a message
> + *   for each core, in the correct order.
> + *
> + *   Then, it tries to take the global write lock and display the last
> + *   message. The autotest script checks that the message order is correct.
> + */
>  static int
> -test_rwlock(void)
> +rwlock_test1(void)
>  {
>  int i;
>
> @@ -98,4 +131,338 @@ test_rwlock(void)
>  return 0;
>  }
>
> +static int
> +try_read(uint32_t lc)
> +{
> +int32_t rc;
> +uint32_t i;
> +
> +rc = rte_rwlock_read_trylock(&try_rwlock_data.lock);
> +if (rc != 0)
> +return rc;
> +
> +for (i = 0; i != RTE_DIM(try_rwlock_data.data.u64); i++) {
> +
> +/* race condition occurred, lock doesn't work properly */
> +if (try_rwlock_data.data.u64[i] != 0) {
> +printf("%s(%u) error: unexpected data pattern\n",
> +__func__, lc);
> +rte_memdump(stdout, NULL,
> +(void *)(uintptr_t)&try_rwlock_data.data,
> +sizeof(try_rwlock_data.data));
> +rc = -EFAULT;
> +break;
> +}
> +}
> +
> +rte_rwlock_read_unlock(&try_rwlock_data.lock);
> +return rc;
> +}
> +
> +static int
> +try_write(uint32_t lc)
> +{
> +int32_t rc;
> +uint32_t i, v;
> +
> +v = RTE_MAX(lc % UINT8_MAX, 1U);
> +
> +rc = rte_rwlock_write_trylock(&try_rwlock_data.lock);
> +if (rc != 0)
> +return rc;
> +
> +/* update by bytes in reverese order */
> +for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
> +
> +/* race condition occurred, lock doesn't work properly */
> +if (try_rwlock_data.data.u8[i] != 0) {
> +printf("%s:%d(%u) error: unexpected data pattern\n",
> +__func__, __LINE__, lc);
> +rte_memdump(stdout, NULL,
> +(void *)(uintptr_t)&try_rwlock_data.data,
> +sizeof(try_rwlock_data.data));
> +rc = -EFAULT;
> +break;
> +}
> +
> +try_rwlock_data.data.u8[i] = v;
> +}
> +
> +/* restore by bytes in reverese order */
> +for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
> +
> +/* race condition occurred, lock doesn't work properly */
> +if (try_rwlock_data.data.u8[i] != v) {
> +printf("%s:%d(%u) error: unexpected data pattern\n",
> +__func__, __LINE__, lc);
> +rte_memdump(stdout, NULL,
> +(void *)(uintptr_t)&try_rwlock_data.data,
> +sizeof(try_rwlock_data.data));
> +rc = -EFAULT;
> +break;
> +}
> +
> +try_rwlock_data.data.u8[i] = 0;
> +}
> +
> +rte_rwlock_write_unlock(&try_rwlock_data.lock);
> +return rc;
> +}
> +
> +static int
> +try_read_lcore(__rte_unused void *data) {
> +int32_t rc;
> +uint32_t i, lc;
> +uint64_t ftm, stm, tm;
> +struct try_rwlock_lcore *lcd;
> +
> +lc = rte_lcore_id();
> +lcd = try_lcore_data + lc;
> +lcd->type = LC_TYPE_RDLOCK;
> +
> +ftm = try_rwlock_data.tick;
> +stm = rte_get_timer_cycles();
> +
> +do {
> +for (i = 0; i != ITER_NUM; i++) {
> +rc = try_read(lc);
> +if (rc == 0)
> +lcd->stat.success++;
> +else if (rc == -EBUSY)
> +lcd->stat.fail++;
> +else
> +break;
> +rc = 0;
> +}
> +tm = rte_get_timer_cycles() - stm;
> +} while (tm < ftm && rc == 0);
> +
> +lcd->rc = rc;
> +lcd->stat.tick = tm;
> +return rc;
> +}
> +
> +static int
> +try_write_lcore(__rte_unused void *data) {
> +int32_t rc;
> +uint32_t i, lc;
> +uint64_t ftm, stm, tm;
> +struct try_rwlock_lcore *lcd;
> +
> +lc = rte_lcore_id();
> +lcd = try_lcore_data + lc;
> +lcd->type = LC_TYPE_WRLOCK;
> +
> +ftm = try_rwlock_data.tick;
> +stm = rte_get_timer_cycles();
> +
> +do {
> +for (i = 0; i != ITER_NUM; i++) {
> +rc = try_write(lc);
> +if (rc == 0)
> +lcd->stat.success++;
> +else if (rc == -EBUSY)
> +lcd->stat.fail++;
> +else
> +break;
> +rc = 0;
> +}
> +tm = rte_get_timer_cycles() - stm;
> +} while (tm < ftm && rc == 0);
> +
> +lcd->rc = rc;
> +lcd->stat.tick = tm;
> +return rc;
> +}
> +
> +static void
> +print_try_lcore_stats(const struct try_rwlock_lcore *tlc, uint32_t lc)
> +{
> +uint64_t f, s;
> +
> +f = RTE_MAX(tlc->stat.fail, 1ULL);
> +s = RTE_MAX(tlc->stat.success, 1ULL);
> +
> +printf("try_lcore_data[%u]={\n"
> +"\trc=%d,\n"
> +"\ttype=%s,\n"
> +"\tfail=%" PRIu64 ",\n"
> +"\tsuccess=%" PRIu64 ",\n"
> +"\tcycles=%" PRIu64 ",\n"
> +"\tcycles/op=%#Lf,\n"
> +"\tcycles/success=%#Lf,\n"
> +"\tsuccess/fail=%#Lf,\n"
> +"};\n",
> +lc,
> +tlc->rc,
> +tlc->type == LC_TYPE_RDLOCK ? "RDLOCK" : "WRLOCK",
> +tlc->stat.fail,
> +tlc->stat.success,
> +tlc->stat.tick,
> +(long double)tlc->stat.tick /
> +(tlc->stat.fail + tlc->stat.success),
> +(long double)tlc->stat.tick / s,
> +(long double)tlc->stat.success / f);
> +}
> +
> +static void
> +collect_try_lcore_stats(struct try_rwlock_lcore *tlc,
> +const struct try_rwlock_lcore *lc)
> +{
> +tlc->stat.tick += lc->stat.tick;
> +tlc->stat.fail += lc->stat.fail;
> +tlc->stat.success += lc->stat.success; }
> +
> +/*
> + * Process collected results:
> + *  - check status
> + *  - collect and print statistics
> + */
> +static int
> +process_try_lcore_stats(void)
> +{
> +int32_t rc;
> +uint32_t lc, rd, wr;
> +struct try_rwlock_lcore rlc, wlc;
> +
> +memset(&rlc, 0, sizeof(rlc));
> +memset(&wlc, 0, sizeof(wlc));
> +
> +rlc.type = LC_TYPE_RDLOCK;
> +wlc.type = LC_TYPE_WRLOCK;
> +rd = 0;
> +wr = 0;
> +
> +rc = 0;
> +RTE_LCORE_FOREACH(lc) {
> +rc |= try_lcore_data[lc].rc;
> +if (try_lcore_data[lc].type == LC_TYPE_RDLOCK) {
> +collect_try_lcore_stats(&rlc, try_lcore_data + lc);
> +rd++;
> +} else {
> +collect_try_lcore_stats(&wlc, try_lcore_data + lc);
> +wr++;
> +}
> +}
> +
> +if (rc == 0) {
> +RTE_LCORE_FOREACH(lc)
> +print_try_lcore_stats(try_lcore_data + lc, lc);
> +
> +if (rd != 0) {
> +printf("aggregated stats for %u RDLOCK cores:\n",
> rd);
> +print_try_lcore_stats(&rlc, rd);
> +}
> +
> +if (wr != 0) {
> +printf("aggregated stats for %u WRLOCK cores:\n",
> wr);
> +print_try_lcore_stats(&wlc, wr);
> +}
> +}
> +
> +return rc;
> +}
> +
> +static void
> +try_test_reset(void)
> +{
> +memset(&try_lcore_data, 0, sizeof(try_lcore_data));
> +memset(&try_rwlock_data, 0, sizeof(try_rwlock_data));
> +try_rwlock_data.tick = TEST_SEC * rte_get_tsc_hz(); }
> +
> +/* all lcores grab RDLOCK */
> +static int
> +try_rwlock_test_rda(void)
> +{
> +try_test_reset();
> +
> +/* start read test on all avaialble lcores */
> +rte_eal_mp_remote_launch(try_read_lcore, NULL, CALL_MASTER);
> +rte_eal_mp_wait_lcore();
> +
> +return process_try_lcore_stats();
> +}
> +
> +/* all slave lcores grab RDLOCK, master one grabs WRLOCK */ static int
> +try_rwlock_test_rds_wrm(void)
> +{
> +try_test_reset();
> +
> +rte_eal_mp_remote_launch(try_read_lcore, NULL, SKIP_MASTER);
> +try_write_lcore(NULL);
> +rte_eal_mp_wait_lcore();
> +
> +return process_try_lcore_stats();
> +}
> +
> +/* master and even slave lcores grab RDLOCK, odd lcores grab WRLOCK */
> +static int
> +try_rwlock_test_rde_wro(void)
> +{
> +uint32_t lc, mlc;
> +
> +try_test_reset();
> +
> +mlc = rte_get_master_lcore();
> +
> +RTE_LCORE_FOREACH(lc) {
> +if (lc != mlc) {
> +if ((lc & 1) == 0)
> +rte_eal_remote_launch(try_read_lcore,
> +NULL, lc);
> +else
> +rte_eal_remote_launch(try_write_lcore,
> +NULL, lc);
> +}
> +}
> +try_read_lcore(NULL);
> +rte_eal_mp_wait_lcore();
> +
> +return process_try_lcore_stats();
> +}
> +
> +static int
> +test_rwlock(void)
> +{
> +uint32_t i;
> +int32_t rc, ret;
> +
> +static const struct {
> +const char *name;
> +int (*ftst)(void);
> +} test[] = {
> +{
> +.name = "rwlock_test1",
> +.ftst = rwlock_test1,
> +},
> +{
> +.name = "try_rwlock_test_rda",
> +.ftst = try_rwlock_test_rda,
> +},
> +{
> +.name = "try_rwlock_test_rds_wrm",
> +.ftst = try_rwlock_test_rds_wrm,
> +},
> +{
> +.name = "try_rwlock_test_rde_wro",
> +.ftst = try_rwlock_test_rde_wro,
> +},
> +};
> +
> +ret = 0;
> +for (i = 0; i != RTE_DIM(test); i++) {
> +printf("starting test %s;\n", test[i].name);
> +rc = test[i].ftst();
> +printf("test %s completed with status %d\n", test[i].name,
> rc);
> +ret |= rc;
> +}
> +
> +return ret;
> +}
> +
>  REGISTER_TEST_COMMAND(rwlock_autotest, test_rwlock);
> --
> 2.17.1
Other than the minor comment,
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19  6:37   ` Honnappa Nagarahalli
@ 2018-12-19  8:30     ` Gavin Hu (Arm Technology China)
  2018-12-19 10:28     ` Mattias Rönnblom
  2018-12-19 15:11     ` Honnappa Nagarahalli
  2 siblings, 0 replies; 17+ messages in thread
From: Gavin Hu (Arm Technology China) @ 2018-12-19  8:30 UTC (permalink / raw)
  To: Honnappa Nagarahalli, Konstantin Ananyev, dev; +Cc: nd



> -----Original Message-----
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> Sent: Wednesday, December 19, 2018 2:38 PM
> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD
> and WR locking
> 
> >
> > This patch targets 19.02 release.
> >
> > Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  .../common/include/generic/rte_rwlock.h       | 54 +++++++++++++++++++
> >  lib/librte_eal/rte_eal_version.map            |  2 +
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > index 5751a0e6d..7e395781e 100644
> > --- a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > +++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > @@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
> >  	}
> >  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a read lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for reading because a
> > + *     writer holds the lock
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_read_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
> > +		x = rwl->cnt;
> > +		/* write lock is held */
> > +		if (x < 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl-
> >cnt,
> > +					      (uint32_t)x, (uint32_t)(x + 1));
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Release a read lock.
> >   *
> > @@ -87,6 +114,33 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
> >  	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a write lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for writing because
> > + *     it was already locked for reading or writing
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_write_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
> > +		x = rwl->cnt;
> > +		/* a lock is held */
> > +		if (x != 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl-
> >cnt,
> > +					      0, (uint32_t)-1);
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Take a write lock. Loop until the lock is held.
> >   *
> > diff --git a/lib/librte_eal/rte_eal_version.map
> > b/lib/librte_eal/rte_eal_version.map
> > index 3fe78260d..8b1593dd8 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -355,6 +355,8 @@ EXPERIMENTAL {
> >  	rte_mp_request_async;
> >  	rte_mp_sendmsg;
> >  	rte_option_register;
> > +	rte_rwlock_read_trylock;
> > +	rte_rwlock_write_trylock;
> I do not see the other RW lock APIs in this file.
> 
> >  	rte_service_lcore_attr_get;
> >  	rte_service_lcore_attr_reset_all;
> >  	rte_service_may_be_active;
> > --
> > 2.17.1
> 
> Other than the minor comments,
> Reviewed-by: Honnappa Nagarahalli <Honnappa.nagarahalli@arm.com>
Same comments as Honnappa, experimental warning is required in new code.
Reviewed-by: Gavin Hu <gavin.hu@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19  6:37   ` Honnappa Nagarahalli
  2018-12-19  8:30     ` Gavin Hu (Arm Technology China)
@ 2018-12-19 10:28     ` Mattias Rönnblom
  2018-12-19 10:56       ` Ananyev, Konstantin
  2018-12-19 15:11     ` Honnappa Nagarahalli
  2 siblings, 1 reply; 17+ messages in thread
From: Mattias Rönnblom @ 2018-12-19 10:28 UTC (permalink / raw)
  To: Honnappa Nagarahalli, Konstantin Ananyev, dev
  Cc: nd, Gavin Hu (Arm Technology China)

On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
>> diff --git a/lib/librte_eal/rte_eal_version.map
>> b/lib/librte_eal/rte_eal_version.map
>> index 3fe78260d..8b1593dd8 100644
>> --- a/lib/librte_eal/rte_eal_version.map
>> +++ b/lib/librte_eal/rte_eal_version.map
>> @@ -355,6 +355,8 @@ EXPERIMENTAL {
>>   	rte_mp_request_async;
>>   	rte_mp_sendmsg;
>>   	rte_option_register;
>> +	rte_rwlock_read_trylock;
>> +	rte_rwlock_write_trylock;
> I do not see the other RW lock APIs in this file.
> 

They, just like those added, are static inline functions, will not 
result in any symbols in any shared objects and thus shouldn't be in any 
version.map file.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19 10:28     ` Mattias Rönnblom
@ 2018-12-19 10:56       ` Ananyev, Konstantin
  2018-12-19 11:00         ` Bruce Richardson
  0 siblings, 1 reply; 17+ messages in thread
From: Ananyev, Konstantin @ 2018-12-19 10:56 UTC (permalink / raw)
  To: Mattias Rönnblom, Honnappa Nagarahalli, dev
  Cc: nd, Gavin Hu (Arm Technology China)



> -----Original Message-----
> From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> Sent: Wednesday, December 19, 2018 10:28 AM
> To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> 
> On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> >> diff --git a/lib/librte_eal/rte_eal_version.map
> >> b/lib/librte_eal/rte_eal_version.map
> >> index 3fe78260d..8b1593dd8 100644
> >> --- a/lib/librte_eal/rte_eal_version.map
> >> +++ b/lib/librte_eal/rte_eal_version.map
> >> @@ -355,6 +355,8 @@ EXPERIMENTAL {
> >>   	rte_mp_request_async;
> >>   	rte_mp_sendmsg;
> >>   	rte_option_register;
> >> +	rte_rwlock_read_trylock;
> >> +	rte_rwlock_write_trylock;
> > I do not see the other RW lock APIs in this file.
> >
> 
> They, just like those added, are static inline functions, will not
> result in any symbols in any shared objects and thus shouldn't be in any
> version.map file.

So, are you guys trying to say that there is no need to put these 'trylock'
functions into 'experimental' section?
Konstantin
 


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19 10:56       ` Ananyev, Konstantin
@ 2018-12-19 11:00         ` Bruce Richardson
  2018-12-19 12:41           ` Ananyev, Konstantin
  0 siblings, 1 reply; 17+ messages in thread
From: Bruce Richardson @ 2018-12-19 11:00 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Mattias Rönnblom, Honnappa Nagarahalli, dev, nd,
	Gavin Hu (Arm Technology China)

On Wed, Dec 19, 2018 at 10:56:09AM +0000, Ananyev, Konstantin wrote:
> 
> 
> > -----Original Message-----
> > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > Sent: Wednesday, December 19, 2018 10:28 AM
> > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> > 
> > On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> > >> diff --git a/lib/librte_eal/rte_eal_version.map
> > >> b/lib/librte_eal/rte_eal_version.map
> > >> index 3fe78260d..8b1593dd8 100644
> > >> --- a/lib/librte_eal/rte_eal_version.map
> > >> +++ b/lib/librte_eal/rte_eal_version.map
> > >> @@ -355,6 +355,8 @@ EXPERIMENTAL {
> > >>   	rte_mp_request_async;
> > >>   	rte_mp_sendmsg;
> > >>   	rte_option_register;
> > >> +	rte_rwlock_read_trylock;
> > >> +	rte_rwlock_write_trylock;
> > > I do not see the other RW lock APIs in this file.
> > >
> > 
> > They, just like those added, are static inline functions, will not
> > result in any symbols in any shared objects and thus shouldn't be in any
> > version.map file.
> 
> So, are you guys trying to say that there is no need to put these 'trylock'
> functions into 'experimental' section?
> Konstantin
> 
Anything static inline doesn't have a mapfile entry, so there is no issue
there. However, the APIs themselves, since they are new should probably
have the __rte_experimental attribute marked on them in the code.

/Bruce

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19 11:00         ` Bruce Richardson
@ 2018-12-19 12:41           ` Ananyev, Konstantin
  0 siblings, 0 replies; 17+ messages in thread
From: Ananyev, Konstantin @ 2018-12-19 12:41 UTC (permalink / raw)
  To: Richardson, Bruce
  Cc: Mattias Rönnblom, Honnappa Nagarahalli, dev, nd,
	Gavin Hu (Arm Technology China)



> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, December 19, 2018 11:00 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Mattias Rönnblom <hofors@lysator.liu.se>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; dev@dpdk.org; nd
> <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> 
> On Wed, Dec 19, 2018 at 10:56:09AM +0000, Ananyev, Konstantin wrote:
> >
> >
> > > -----Original Message-----
> > > From: Mattias Rönnblom [mailto:hofors@lysator.liu.se]
> > > Sent: Wednesday, December 19, 2018 10:28 AM
> > > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > > Subject: Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
> > >
> > > On 2018-12-19 07:37, Honnappa Nagarahalli wrote:
> > > >> diff --git a/lib/librte_eal/rte_eal_version.map
> > > >> b/lib/librte_eal/rte_eal_version.map
> > > >> index 3fe78260d..8b1593dd8 100644
> > > >> --- a/lib/librte_eal/rte_eal_version.map
> > > >> +++ b/lib/librte_eal/rte_eal_version.map
> > > >> @@ -355,6 +355,8 @@ EXPERIMENTAL {
> > > >>   	rte_mp_request_async;
> > > >>   	rte_mp_sendmsg;
> > > >>   	rte_option_register;
> > > >> +	rte_rwlock_read_trylock;
> > > >> +	rte_rwlock_write_trylock;
> > > > I do not see the other RW lock APIs in this file.
> > > >
> > >
> > > They, just like those added, are static inline functions, will not
> > > result in any symbols in any shared objects and thus shouldn't be in any
> > > version.map file.
> >
> > So, are you guys trying to say that there is no need to put these 'trylock'
> > functions into 'experimental' section?
> > Konstantin
> >
> Anything static inline doesn't have a mapfile entry, so there is no issue
> there.

Ok, if these map entries are not needed, I'll remove them in v2. 

> However, the APIs themselves, since they are new should probably
> have the __rte_experimental attribute marked on them in the code.

They do have __rte_experimental attribute in the .h file.
Konstantin

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19  6:37   ` Honnappa Nagarahalli
  2018-12-19  8:30     ` Gavin Hu (Arm Technology China)
  2018-12-19 10:28     ` Mattias Rönnblom
@ 2018-12-19 15:11     ` Honnappa Nagarahalli
  2018-12-19 16:27       ` Ananyev, Konstantin
  2 siblings, 1 reply; 17+ messages in thread
From: Honnappa Nagarahalli @ 2018-12-19 15:11 UTC (permalink / raw)
  To: Konstantin Ananyev, dev; +Cc: nd, Gavin Hu (Arm Technology China), nd

> 
> >
> > This patch targets 19.02 release.
> >
> > Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().
> > Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> >  .../common/include/generic/rte_rwlock.h       | 54 +++++++++++++++++++
> >  lib/librte_eal/rte_eal_version.map            |  2 +
> >  2 files changed, 56 insertions(+)
> >
> > diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > index 5751a0e6d..7e395781e 100644
> > --- a/lib/librte_eal/common/include/generic/rte_rwlock.h
> > +++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
> > @@ -75,6 +75,33 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
> >  	}
> >  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a read lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for reading because a
> > + *     writer holds the lock
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_read_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
> > +		x = rwl->cnt;
> > +		/* write lock is held */
> > +		if (x < 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> > +					      (uint32_t)x, (uint32_t)(x + 1));
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Release a read lock.
> >   *
> > @@ -87,6 +114,33 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
> >  	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);  }
> >
> > +/**
> Please add the experimental API warning
> 
> > + * try to take a write lock.
> > + *
> > + * @param rwl
> > + *   A pointer to a rwlock structure.
> > + * @return
> > + *   - zero if the lock is successfully taken
> > + *   - -EBUSY if lock could not be acquired for writing because
> > + *     it was already locked for reading or writing
> > + */
> > +static inline __rte_experimental int
> > +rte_rwlock_write_trylock(rte_rwlock_t *rwl) {
> > +	int32_t x;
> > +	int success = 0;
> > +
> > +	while (success == 0) {
(Apologies for not thinking through all my comments earlier)
I am wondering if the 'while' loop is required for the write lock.

> > +		x = rwl->cnt;
> > +		/* a lock is held */
> > +		if (x != 0)
> > +			return -EBUSY;
> > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> > +					      0, (uint32_t)-1);
This might fail as the lock was taken (either reader or writer). We should return from here as it already indicates that the lock is not available for the writer to take. Otherwise, there is a possibility that the while loop will run for multiple iterations. I would think, from the user's expectation, it might not be correct.

In the read_trylock, the while loop should be fine because the write lock itself is not held. The failure could be because some other reader incremented the counter before we did. i.e. the reader lock itself may be available to take in the next iteration.

> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Take a write lock. Loop until the lock is held.
> >   *
> > diff --git a/lib/librte_eal/rte_eal_version.map
> > b/lib/librte_eal/rte_eal_version.map
> > index 3fe78260d..8b1593dd8 100644
> > --- a/lib/librte_eal/rte_eal_version.map
> > +++ b/lib/librte_eal/rte_eal_version.map
> > @@ -355,6 +355,8 @@ EXPERIMENTAL {
> >  	rte_mp_request_async;
> >  	rte_mp_sendmsg;
> >  	rte_option_register;
> > +	rte_rwlock_read_trylock;
> > +	rte_rwlock_write_trylock;
> I do not see the other RW lock APIs in this file.
> 
> >  	rte_service_lcore_attr_get;
> >  	rte_service_lcore_attr_reset_all;
> >  	rte_service_may_be_active;
> > --
> > 2.17.1
> 
> Other than the minor comments,
> Reviewed-by: Honnappa Nagarahalli <Honnappa.nagarahalli@arm.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-12-19 15:11     ` Honnappa Nagarahalli
@ 2018-12-19 16:27       ` Ananyev, Konstantin
  0 siblings, 0 replies; 17+ messages in thread
From: Ananyev, Konstantin @ 2018-12-19 16:27 UTC (permalink / raw)
  To: Honnappa Nagarahalli, dev; +Cc: nd, Gavin Hu (Arm Technology China), nd



> > > + * try to take a write lock.
> > > + *
> > > + * @param rwl
> > > + *   A pointer to a rwlock structure.
> > > + * @return
> > > + *   - zero if the lock is successfully taken
> > > + *   - -EBUSY if lock could not be acquired for writing because
> > > + *     it was already locked for reading or writing
> > > + */
> > > +static inline __rte_experimental int
> > > +rte_rwlock_write_trylock(rte_rwlock_t *rwl) {
> > > +	int32_t x;
> > > +	int success = 0;
> > > +
> > > +	while (success == 0) {
> (Apologies for not thinking through all my comments earlier)
> I am wondering if the 'while' loop is required for the write lock.
> 
> > > +		x = rwl->cnt;
> > > +		/* a lock is held */
> > > +		if (x != 0)
> > > +			return -EBUSY;
> > > +		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
> > > +					      0, (uint32_t)-1);
> This might fail as the lock was taken (either reader or writer). We should return from here as it already indicates that the lock is not
> available for the writer to take. Otherwise, there is a possibility that the while loop will run for multiple iterations. I would think, from the
> user's expectation, it might not be correct.

Good point  - it will also save us extra read in case of failure.
Will change in v2.
Konstantin

> 
> In the read_trylock, the while loop should be fine because the write lock itself is not held. The failure could be because some other reader
> incremented the counter before we did. i.e. the reader lock itself may be available to take in the next iteration.
> 
> > > +	}
> > > +	return 0;
> > > +}
> > > +
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v2 0/2] Add 'try' semantics for RD and WR locking
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
                   ` (2 preceding siblings ...)
  2018-12-19  2:39 ` [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Thomas Monjalon
@ 2018-12-19 18:07 ` Konstantin Ananyev
  2018-12-19 19:53   ` Thomas Monjalon
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 1/2] rwlock: introduce " Konstantin Ananyev
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
  5 siblings, 1 reply; 17+ messages in thread
From: Konstantin Ananyev @ 2018-12-19 18:07 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock()
and new UT test-case for it.

v1 -> v2
Changes per Honnappa and Gavin comments:
 - remove cycle in  rte_rwlock_write_trylock()
 - remove static inline functions from .map file
 - update comments

Konstantin Ananyev (2):
  rwlock: introduce 'try' semantics for RD and WR locking
  test: add new test-cases for rwlock autotest

 .../common/include/generic/rte_rwlock.h       |  56 +++
 test/test/test_rwlock.c                       | 409 +++++++++++++++++-
 2 files changed, 446 insertions(+), 19 deletions(-)

-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v2 1/2] rwlock: introduce 'try' semantics for RD and WR locking
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
                   ` (3 preceding siblings ...)
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 " Konstantin Ananyev
@ 2018-12-19 18:07 ` Konstantin Ananyev
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
  5 siblings, 0 replies; 17+ messages in thread
From: Konstantin Ananyev @ 2018-12-19 18:07 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock().

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Reviewed-by: Honnappa Nagarahalli <Honnappa.nagarahalli@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 .../common/include/generic/rte_rwlock.h       | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/lib/librte_eal/common/include/generic/rte_rwlock.h b/lib/librte_eal/common/include/generic/rte_rwlock.h
index 5751a0e6d..b05d85aee 100644
--- a/lib/librte_eal/common/include/generic/rte_rwlock.h
+++ b/lib/librte_eal/common/include/generic/rte_rwlock.h
@@ -75,6 +75,36 @@ rte_rwlock_read_lock(rte_rwlock_t *rwl)
 	}
 }
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * try to take a read lock.
+ *
+ * @param rwl
+ *   A pointer to a rwlock structure.
+ * @return
+ *   - zero if the lock is successfully taken
+ *   - -EBUSY if lock could not be acquired for reading because a
+ *     writer holds the lock
+ */
+static inline __rte_experimental int
+rte_rwlock_read_trylock(rte_rwlock_t *rwl)
+{
+	int32_t x;
+	int success = 0;
+
+	while (success == 0) {
+		x = rwl->cnt;
+		/* write lock is held */
+		if (x < 0)
+			return -EBUSY;
+		success = rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
+					      (uint32_t)x, (uint32_t)(x + 1));
+	}
+	return 0;
+}
+
 /**
  * Release a read lock.
  *
@@ -87,6 +117,32 @@ rte_rwlock_read_unlock(rte_rwlock_t *rwl)
 	rte_atomic32_dec((rte_atomic32_t *)(intptr_t)&rwl->cnt);
 }
 
+/**
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice.
+ *
+ * try to take a write lock.
+ *
+ * @param rwl
+ *   A pointer to a rwlock structure.
+ * @return
+ *   - zero if the lock is successfully taken
+ *   - -EBUSY if lock could not be acquired for writing because
+ *     it was already locked for reading or writing
+ */
+static inline __rte_experimental int
+rte_rwlock_write_trylock(rte_rwlock_t *rwl)
+{
+	int32_t x;
+
+	x = rwl->cnt;
+	if (x != 0 || rte_atomic32_cmpset((volatile uint32_t *)&rwl->cnt,
+				0, (uint32_t)-1) == 0)
+		return -EBUSY;
+
+	return 0;
+}
+
 /**
  * Take a write lock. Loop until the lock is held.
  *
-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [dpdk-dev] [PATCH v2 2/2] test: add new test-cases for rwlock autotest
  2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
                   ` (4 preceding siblings ...)
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 1/2] rwlock: introduce " Konstantin Ananyev
@ 2018-12-19 18:07 ` Konstantin Ananyev
  5 siblings, 0 replies; 17+ messages in thread
From: Konstantin Ananyev @ 2018-12-19 18:07 UTC (permalink / raw)
  To: dev; +Cc: Konstantin Ananyev

Add few functional and perfomance tests
for rte_rwlock_read_trylock() and rte_rwlock_write_trylock().

Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
---
 test/test/test_rwlock.c | 409 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 390 insertions(+), 19 deletions(-)

diff --git a/test/test/test_rwlock.c b/test/test/test_rwlock.c
index 29171c422..224f0dea8 100644
--- a/test/test/test_rwlock.c
+++ b/test/test/test_rwlock.c
@@ -4,8 +4,10 @@
 
 #include <stdio.h>
 #include <stdint.h>
+#include <inttypes.h>
 #include <unistd.h>
 #include <sys/queue.h>
+#include <string.h>
 
 #include <rte_common.h>
 #include <rte_memory.h>
@@ -22,29 +24,45 @@
 /*
  * rwlock test
  * ===========
- *
- * - There is a global rwlock and a table of rwlocks (one per lcore).
- *
- * - The test function takes all of these locks and launches the
- *   ``test_rwlock_per_core()`` function on each core (except the master).
- *
- *   - The function takes the global write lock, display something,
- *     then releases the global lock.
- *   - Then, it takes the per-lcore write lock, display something, and
- *     releases the per-core lock.
- *   - Finally, a read lock is taken during 100 ms, then released.
- *
- * - The main function unlocks the per-lcore locks sequentially and
- *   waits between each lock. This triggers the display of a message
- *   for each core, in the correct order.
- *
- *   Then, it tries to take the global write lock and display the last
- *   message. The autotest script checks that the message order is correct.
+ * Provides UT for rte_rwlock API.
+ * Main concern is on functional testing, but also provides some
+ * performance measurements.
+ * Obviously for proper testing need to be executed with more than one lcore.
  */
 
+#define ITER_NUM	0x80
+
+#define TEST_SEC	5
+
 static rte_rwlock_t sl;
 static rte_rwlock_t sl_tab[RTE_MAX_LCORE];
 
+enum {
+	LC_TYPE_RDLOCK,
+	LC_TYPE_WRLOCK,
+};
+
+static struct {
+	rte_rwlock_t lock;
+	uint64_t tick;
+	volatile union {
+		uint8_t u8[RTE_CACHE_LINE_SIZE];
+		uint64_t u64[RTE_CACHE_LINE_SIZE / sizeof(uint64_t)];
+	} data;
+} __rte_cache_aligned try_rwlock_data;
+
+struct try_rwlock_lcore {
+	int32_t rc;
+	int32_t type;
+	struct {
+		uint64_t tick;
+		uint64_t fail;
+		uint64_t success;
+	} stat;
+} __rte_cache_aligned;
+
+static struct try_rwlock_lcore try_lcore_data[RTE_MAX_LCORE];
+
 static int
 test_rwlock_per_core(__attribute__((unused)) void *arg)
 {
@@ -65,8 +83,27 @@ test_rwlock_per_core(__attribute__((unused)) void *arg)
 	return 0;
 }
 
+/*
+ * - There is a global rwlock and a table of rwlocks (one per lcore).
+ *
+ * - The test function takes all of these locks and launches the
+ *   ``test_rwlock_per_core()`` function on each core (except the master).
+ *
+ *   - The function takes the global write lock, display something,
+ *     then releases the global lock.
+ *   - Then, it takes the per-lcore write lock, display something, and
+ *     releases the per-core lock.
+ *   - Finally, a read lock is taken during 100 ms, then released.
+ *
+ * - The main function unlocks the per-lcore locks sequentially and
+ *   waits between each lock. This triggers the display of a message
+ *   for each core, in the correct order.
+ *
+ *   Then, it tries to take the global write lock and display the last
+ *   message. The autotest script checks that the message order is correct.
+ */
 static int
-test_rwlock(void)
+rwlock_test1(void)
 {
 	int i;
 
@@ -98,4 +135,338 @@ test_rwlock(void)
 	return 0;
 }
 
+static int
+try_read(uint32_t lc)
+{
+	int32_t rc;
+	uint32_t i;
+
+	rc = rte_rwlock_read_trylock(&try_rwlock_data.lock);
+	if (rc != 0)
+		return rc;
+
+	for (i = 0; i != RTE_DIM(try_rwlock_data.data.u64); i++) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u64[i] != 0) {
+			printf("%s(%u) error: unexpected data pattern\n",
+				__func__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+	}
+
+	rte_rwlock_read_unlock(&try_rwlock_data.lock);
+	return rc;
+}
+
+static int
+try_write(uint32_t lc)
+{
+	int32_t rc;
+	uint32_t i, v;
+
+	v = RTE_MAX(lc % UINT8_MAX, 1U);
+
+	rc = rte_rwlock_write_trylock(&try_rwlock_data.lock);
+	if (rc != 0)
+		return rc;
+
+	/* update by bytes in reverese order */
+	for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u8[i] != 0) {
+			printf("%s:%d(%u) error: unexpected data pattern\n",
+				__func__, __LINE__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+
+		try_rwlock_data.data.u8[i] = v;
+	}
+
+	/* restore by bytes in reverese order */
+	for (i = RTE_DIM(try_rwlock_data.data.u8); i-- != 0; ) {
+
+		/* race condition occurred, lock doesn't work properly */
+		if (try_rwlock_data.data.u8[i] != v) {
+			printf("%s:%d(%u) error: unexpected data pattern\n",
+				__func__, __LINE__, lc);
+			rte_memdump(stdout, NULL,
+				(void *)(uintptr_t)&try_rwlock_data.data,
+				sizeof(try_rwlock_data.data));
+			rc = -EFAULT;
+			break;
+		}
+
+		try_rwlock_data.data.u8[i] = 0;
+	}
+
+	rte_rwlock_write_unlock(&try_rwlock_data.lock);
+	return rc;
+}
+
+static int
+try_read_lcore(__rte_unused void *data)
+{
+	int32_t rc;
+	uint32_t i, lc;
+	uint64_t ftm, stm, tm;
+	struct try_rwlock_lcore *lcd;
+
+	lc = rte_lcore_id();
+	lcd = try_lcore_data + lc;
+	lcd->type = LC_TYPE_RDLOCK;
+
+	ftm = try_rwlock_data.tick;
+	stm = rte_get_timer_cycles();
+
+	do {
+		for (i = 0; i != ITER_NUM; i++) {
+			rc = try_read(lc);
+			if (rc == 0)
+				lcd->stat.success++;
+			else if (rc == -EBUSY)
+				lcd->stat.fail++;
+			else
+				break;
+			rc = 0;
+		}
+		tm = rte_get_timer_cycles() - stm;
+	} while (tm < ftm && rc == 0);
+
+	lcd->rc = rc;
+	lcd->stat.tick = tm;
+	return rc;
+}
+
+static int
+try_write_lcore(__rte_unused void *data)
+{
+	int32_t rc;
+	uint32_t i, lc;
+	uint64_t ftm, stm, tm;
+	struct try_rwlock_lcore *lcd;
+
+	lc = rte_lcore_id();
+	lcd = try_lcore_data + lc;
+	lcd->type = LC_TYPE_WRLOCK;
+
+	ftm = try_rwlock_data.tick;
+	stm = rte_get_timer_cycles();
+
+	do {
+		for (i = 0; i != ITER_NUM; i++) {
+			rc = try_write(lc);
+			if (rc == 0)
+				lcd->stat.success++;
+			else if (rc == -EBUSY)
+				lcd->stat.fail++;
+			else
+				break;
+			rc = 0;
+		}
+		tm = rte_get_timer_cycles() - stm;
+	} while (tm < ftm && rc == 0);
+
+	lcd->rc = rc;
+	lcd->stat.tick = tm;
+	return rc;
+}
+
+static void
+print_try_lcore_stats(const struct try_rwlock_lcore *tlc, uint32_t lc)
+{
+	uint64_t f, s;
+
+	f = RTE_MAX(tlc->stat.fail, 1ULL);
+	s = RTE_MAX(tlc->stat.success, 1ULL);
+
+	printf("try_lcore_data[%u]={\n"
+		"\trc=%d,\n"
+		"\ttype=%s,\n"
+		"\tfail=%" PRIu64 ",\n"
+		"\tsuccess=%" PRIu64 ",\n"
+		"\tcycles=%" PRIu64 ",\n"
+		"\tcycles/op=%#Lf,\n"
+		"\tcycles/success=%#Lf,\n"
+		"\tsuccess/fail=%#Lf,\n"
+		"};\n",
+		lc,
+		tlc->rc,
+		tlc->type == LC_TYPE_RDLOCK ? "RDLOCK" : "WRLOCK",
+		tlc->stat.fail,
+		tlc->stat.success,
+		tlc->stat.tick,
+		(long double)tlc->stat.tick /
+		(tlc->stat.fail + tlc->stat.success),
+		(long double)tlc->stat.tick / s,
+		(long double)tlc->stat.success / f);
+}
+
+static void
+collect_try_lcore_stats(struct try_rwlock_lcore *tlc,
+	const struct try_rwlock_lcore *lc)
+{
+	tlc->stat.tick += lc->stat.tick;
+	tlc->stat.fail += lc->stat.fail;
+	tlc->stat.success += lc->stat.success;
+}
+
+/*
+ * Process collected results:
+ *  - check status
+ *  - collect and print statistics
+ */
+static int
+process_try_lcore_stats(void)
+{
+	int32_t rc;
+	uint32_t lc, rd, wr;
+	struct try_rwlock_lcore rlc, wlc;
+
+	memset(&rlc, 0, sizeof(rlc));
+	memset(&wlc, 0, sizeof(wlc));
+
+	rlc.type = LC_TYPE_RDLOCK;
+	wlc.type = LC_TYPE_WRLOCK;
+	rd = 0;
+	wr = 0;
+
+	rc = 0;
+	RTE_LCORE_FOREACH(lc) {
+		rc |= try_lcore_data[lc].rc;
+		if (try_lcore_data[lc].type == LC_TYPE_RDLOCK) {
+			collect_try_lcore_stats(&rlc, try_lcore_data + lc);
+			rd++;
+		} else {
+			collect_try_lcore_stats(&wlc, try_lcore_data + lc);
+			wr++;
+		}
+	}
+
+	if (rc == 0) {
+		RTE_LCORE_FOREACH(lc)
+			print_try_lcore_stats(try_lcore_data + lc, lc);
+
+		if (rd != 0) {
+			printf("aggregated stats for %u RDLOCK cores:\n", rd);
+			print_try_lcore_stats(&rlc, rd);
+		}
+
+		if (wr != 0) {
+			printf("aggregated stats for %u WRLOCK cores:\n", wr);
+			print_try_lcore_stats(&wlc, wr);
+		}
+	}
+
+	return rc;
+}
+
+static void
+try_test_reset(void)
+{
+	memset(&try_lcore_data, 0, sizeof(try_lcore_data));
+	memset(&try_rwlock_data, 0, sizeof(try_rwlock_data));
+	try_rwlock_data.tick = TEST_SEC * rte_get_tsc_hz();
+}
+
+/* all lcores grab RDLOCK */
+static int
+try_rwlock_test_rda(void)
+{
+	try_test_reset();
+
+	/* start read test on all avaialble lcores */
+	rte_eal_mp_remote_launch(try_read_lcore, NULL, CALL_MASTER);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+/* all slave lcores grab RDLOCK, master one grabs WRLOCK */
+static int
+try_rwlock_test_rds_wrm(void)
+{
+	try_test_reset();
+
+	rte_eal_mp_remote_launch(try_read_lcore, NULL, SKIP_MASTER);
+	try_write_lcore(NULL);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+/* master and even slave lcores grab RDLOCK, odd lcores grab WRLOCK */
+static int
+try_rwlock_test_rde_wro(void)
+{
+	uint32_t lc, mlc;
+
+	try_test_reset();
+
+	mlc = rte_get_master_lcore();
+
+	RTE_LCORE_FOREACH(lc) {
+		if (lc != mlc) {
+			if ((lc & 1) == 0)
+				rte_eal_remote_launch(try_read_lcore,
+						NULL, lc);
+			else
+				rte_eal_remote_launch(try_write_lcore,
+						NULL, lc);
+		}
+	}
+	try_read_lcore(NULL);
+	rte_eal_mp_wait_lcore();
+
+	return process_try_lcore_stats();
+}
+
+static int
+test_rwlock(void)
+{
+	uint32_t i;
+	int32_t rc, ret;
+
+	static const struct {
+		const char *name;
+		int (*ftst)(void);
+	} test[] = {
+		{
+			.name = "rwlock_test1",
+			.ftst = rwlock_test1,
+		},
+		{
+			.name = "try_rwlock_test_rda",
+			.ftst = try_rwlock_test_rda,
+		},
+		{
+			.name = "try_rwlock_test_rds_wrm",
+			.ftst = try_rwlock_test_rds_wrm,
+		},
+		{
+			.name = "try_rwlock_test_rde_wro",
+			.ftst = try_rwlock_test_rde_wro,
+		},
+	};
+
+	ret = 0;
+	for (i = 0; i != RTE_DIM(test); i++) {
+		printf("starting test %s;\n", test[i].name);
+		rc = test[i].ftst();
+		printf("test %s completed with status %d\n", test[i].name, rc);
+		ret |= rc;
+	}
+
+	return ret;
+}
+
 REGISTER_TEST_COMMAND(rwlock_autotest, test_rwlock);
-- 
2.17.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [dpdk-dev] [PATCH v2 0/2] Add 'try' semantics for RD and WR locking
  2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 " Konstantin Ananyev
@ 2018-12-19 19:53   ` Thomas Monjalon
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Monjalon @ 2018-12-19 19:53 UTC (permalink / raw)
  To: Konstantin Ananyev; +Cc: dev, honnappa.nagarahalli, gavin.hu

19/12/2018 19:07, Konstantin Ananyev:
> Introduce rte_rwlock_read_trylock() and rte_rwlock_write_trylock()
> and new UT test-case for it.
> 
> v1 -> v2
> Changes per Honnappa and Gavin comments:
>  - remove cycle in  rte_rwlock_write_trylock()
>  - remove static inline functions from .map file
>  - update comments
> 
> Konstantin Ananyev (2):
>   rwlock: introduce 'try' semantics for RD and WR locking
>   test: add new test-cases for rwlock autotest
> 
>  .../common/include/generic/rte_rwlock.h       |  56 +++
>  test/test/test_rwlock.c                       | 409 +++++++++++++++++-
>  2 files changed, 446 insertions(+), 19 deletions(-)

Applied, thanks

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2018-12-19 19:53 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-13 17:27 [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Konstantin Ananyev
2018-11-13 17:27 ` [dpdk-dev] [PATCH 1/2] rwlock: introduce " Konstantin Ananyev
2018-12-19  6:37   ` Honnappa Nagarahalli
2018-12-19  8:30     ` Gavin Hu (Arm Technology China)
2018-12-19 10:28     ` Mattias Rönnblom
2018-12-19 10:56       ` Ananyev, Konstantin
2018-12-19 11:00         ` Bruce Richardson
2018-12-19 12:41           ` Ananyev, Konstantin
2018-12-19 15:11     ` Honnappa Nagarahalli
2018-12-19 16:27       ` Ananyev, Konstantin
2018-11-13 17:27 ` [dpdk-dev] [PATCH 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev
2018-12-19  8:28   ` Gavin Hu (Arm Technology China)
2018-12-19  2:39 ` [dpdk-dev] [PATCH 0/2] Add 'try' semantics for RD and WR locking Thomas Monjalon
2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 " Konstantin Ananyev
2018-12-19 19:53   ` Thomas Monjalon
2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 1/2] rwlock: introduce " Konstantin Ananyev
2018-12-19 18:07 ` [dpdk-dev] [PATCH v2 2/2] test: add new test-cases for rwlock autotest Konstantin Ananyev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).