Hello Lukasz,
On Wed, Sep 23, 2020 at 3:25 PM Lukasz Wojciechowski
<l.wojciec...@partner.samsung.com> wrote:
>
> During review and verification of the patch created by Sarosh Arif:
> "test_distributor: prevent memory leakages from the pool" I found out
> that running distributor unit tests multiple times in a row causes fails.
> So I investigated all the issues I found.
>
> There are few synchronization issues that might cause deadlocks
> or corrupted data. They are fixed with this set of patches for both tests
> and librte_distributor library.
>
> ---
> v3:
> * add missing acked and tested by statements from v1
>
> v2:
> * assign NULL to freed mbufs in distributor test
> * fix handshake check on legacy single distributor
> rte_distributor_return_pkt_single()
> * add patch 7 passing NULL to legacy API calls if no bufs are returned
> * add patch 8 fixing API documentation
>
> Lukasz Wojciechowski (8):
> app/test: fix deadlock in distributor test
> app/test: synchronize statistics between lcores
> app/test: fix freeing mbufs in distributor tests
> app/test: collect return mbufs in distributor test
For these patches, we can use the "test/distributor: " prefix, and we
then avoid repeating "in distributor test"
> distributor: fix missing handshake synchronization
> distributor: fix handshake deadlock
> distributor: do not use oldpkt when not needed
> distributor: align API documentation with code
Thanks for working on those fixes !
Here is a suggestion:
- we can move this new patch 7 before patch 3 in the series, and
update the unit test:
* by passing NULL to the first call to rte_distributor_get_pkt(),
there is no need for buf[] array init in handle_work(),
handle_work_with_free_mbufs() and handle_work_for_shutdown_test(),
* at all points of those functions the buf[] array then contains only
[0, num[ valid entries,
* bonus point, this makes the UT check passing NULL oldpkt,
- the former patch 3 is then easier to do since there is no need for
buf[] array clearing,
This gives the following diff, wdyt?
diff --git a/app/test/test_distributor.c b/app/test/test_distributor.c
index f31b54edf3..b7ab93ecbe 100644
--- a/app/test/test_distributor.c
+++ b/app/test/test_distributor.c
@@ -65,13 +65,10 @@ handle_work(void *arg)
struct rte_mbuf *buf[8] __rte_cache_aligned;
struct worker_params *wp = arg;
struct rte_distributor *db = wp->dist;
- unsigned int count = 0, num = 0;
+ unsigned int count = 0, num;
unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
- int i;
- for (i = 0; i < 8; i++)
- buf[i] = NULL;
- num = rte_distributor_get_pkt(db, id, buf, buf, num);
+ num = rte_distributor_get_pkt(db, id, buf, NULL, 0);
while (!quit) {
__atomic_fetch_add(&worker_stats[id].handled_packets, num,
__ATOMIC_ACQ_REL);
@@ -277,22 +274,17 @@ handle_work_with_free_mbufs(void *arg)
struct rte_mbuf *buf[8] __rte_cache_aligned;
struct worker_params *wp = arg;
struct rte_distributor *d = wp->dist;
+ unsigned int num;
unsigned int i;
- unsigned int num = 0;
unsigned int id = __atomic_fetch_add(&worker_idx, 1, __ATOMIC_RELAXED);
- for (i = 0; i < 8; i++)
- buf[i] = NULL;
- num = rte_distributor_get_pkt(d, id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
while (!quit) {
__atomic_fetch_add(&worker_stats[id].handled_packets, num,
__ATOMIC_ACQ_REL);
- for (i = 0; i < num; i++) {
+ for (i = 0; i < num; i++)
rte_pktmbuf_free(buf[i]);
- buf[i] = NULL;
- }
- num = rte_distributor_get_pkt(d,
- id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
}
__atomic_fetch_add(&worker_stats[id].handled_packets, num,
__ATOMIC_ACQ_REL);
@@ -347,15 +339,13 @@ handle_work_for_shutdown_test(void *arg)
struct rte_mbuf *buf[8] __rte_cache_aligned;
struct worker_params *wp = arg;
struct rte_distributor *d = wp->dist;
- unsigned int num = 0;
+ unsigned int num;
unsigned int i;
unsigned int zero_id = 0;
const unsigned int id = __atomic_fetch_add(&worker_idx, 1,
__ATOMIC_RELAXED);
- for (i = 0; i < 8; i++)
- buf[i] = NULL;
- num = rte_distributor_get_pkt(d, id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
if (id == zero_id && num > 0) {
@@ -369,12 +359,9 @@ handle_work_for_shutdown_test(void *arg)
while (!quit && !(id == zero_id && zero_quit)) {
__atomic_fetch_add(&worker_stats[id].handled_packets, num,
__ATOMIC_ACQ_REL);
- for (i = 0; i < num; i++) {
+ for (i = 0; i < num; i++)
rte_pktmbuf_free(buf[i]);
- buf[i] = NULL;
- }
- num = rte_distributor_get_pkt(d,
- id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
zero_id = __atomic_load_n(&zero_idx, __ATOMIC_ACQUIRE);
if (id == zero_id && num > 0) {
@@ -392,21 +379,16 @@ handle_work_for_shutdown_test(void *arg)
while (zero_quit)
usleep(100);
- for (i = 0; i < num; i++) {
+ for (i = 0; i < num; i++)
rte_pktmbuf_free(buf[i]);
- buf[i] = NULL;
- }
- num = rte_distributor_get_pkt(d,
- id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
while (!quit) {
__atomic_fetch_add(&worker_stats[id].handled_packets,
num, __ATOMIC_ACQ_REL);
- for (i = 0; i < num; i++) {
+ for (i = 0; i < num; i++)
rte_pktmbuf_free(buf[i]);
- buf[i] = NULL;
- }
- num = rte_distributor_get_pkt(d, id, buf, buf, 0);
+ num = rte_distributor_get_pkt(d, id, buf, NULL, 0);
}
}
rte_distributor_return_pkt(d, id, buf, num);
--
David Marchand
