Hello David,
Thank you for your review.
W dniu 25.09.2020 o 14:31, David Marchand pisze:
> 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"
Changed
>
>> 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?
I reorder patches as you suggested.
I added unit test changes in the same patch that changes distributor
lib. The changes follow your diff.
This also simplified "fix freeing mbufs" patch.
It's applied in v4.
>
> 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);
>
>
--
Lukasz Wojciechowski
Principal Software Engineer
Samsung R&D Institute Poland
Samsung Electronics
Office +48 22 377 88 25
l.wojciec...@partner.samsung.com
