From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6DEC8A04C0; Fri, 25 Sep 2020 14:32:18 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 856231E965; Fri, 25 Sep 2020 14:31:46 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by dpdk.org (Postfix) with ESMTP id E88071E965 for ; Fri, 25 Sep 2020 14:31:44 +0200 (CEST) Dkim-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601037104; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=zuI2FmGYTAKhdZWgLdsTNcIbTMW5mKPRdDc7PaA8fNo=; b=BJgVZ8B/g50wwS+cbty9+ugE7eUz/On1O8VayR6M3SHfiS9NbYKBwCaZYIT8hNQx/uslDu x1EWSz6eVrTKL2o8AYtsGj8edkbLSZDN+NiQgmz07pMCvPTCCivV3o4hxUhd2T9aQrLI3+ 8tu4POz2lH+M/CG7CZzvGXA+XC1fAMo= Received: from mail-ua1-f72.google.com (mail-ua1-f72.google.com [209.85.222.72]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-413-0afXBNmlPY-dc60kdihwrw-1; Fri, 25 Sep 2020 08:31:42 -0400 X-MC-Unique: 0afXBNmlPY-dc60kdihwrw-1 Received: by mail-ua1-f72.google.com with SMTP id f7so701404uap.17 for ; Fri, 25 Sep 2020 05:31:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=zuI2FmGYTAKhdZWgLdsTNcIbTMW5mKPRdDc7PaA8fNo=; b=YRtKpf6xVs6LIkYdU1rbRoIlYUiZoAjthUqrpZXD/KjvkalgaGF2CfWb230znjI7Bm gcqEWfvruNasx34GsjlZ4Un82xBKQ3gGMZPb5vNs4+62TYlA68pnRNwbn2swu6+Ozai4 ANvoN897S3oFONoyZxHk0BCM94s1iY6TzmYMHdNgS8U0UN+caAtY+W5spnd7iBXRpiw/ P23nCxd8WyfOS6STf5FKFRpny+3rw+hhgWgaKZJO8XSZRR8LhsACd376xqsjHbteRcHX juzKFvrksgMEOtIpa1eBe20o4qfQ0a9Tiq1RQcFdc2HkyU/SVYuYBtM+FqIiLkR64sq+ VdcQ== X-Gm-Message-State: AOAM530nqXN8ibUMC6T2SNkOTzSBfhxVwdrF6KqGQcc7DVMaUIEPABmY GR4R2CC70F0Uch9ZtwXia9uq3c22ifkxpBEbcgjyUUKJrZ29aq36+eh7CCDjs/57xa6nmmGZhlS 66VyC+pwvSRqcjv/lL7U= X-Received: by 2002:a67:fd44:: with SMTP id g4mr2719401vsr.18.1601037102300; Fri, 25 Sep 2020 05:31:42 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzPYkypSc0qwwz2udFkh3ii0ND/VOF/HLSHEzGBeGGw9+EaYjeysYIh8/7Ilhd18Tmvz9CFdUctq/2gMWYjKcA= X-Received: by 2002:a67:fd44:: with SMTP id g4mr2719369vsr.18.1601037101913; Fri, 25 Sep 2020 05:31:41 -0700 (PDT) MIME-Version: 1.0 References: <20200923014713.16932-1-l.wojciechow@partner.samsung.com> <20200923132541.21417-1-l.wojciechow@partner.samsung.com> In-Reply-To: <20200923132541.21417-1-l.wojciechow@partner.samsung.com> From: David Marchand Date: Fri, 25 Sep 2020 14:31:30 +0200 Message-ID: To: Lukasz Wojciechowski Cc: dev , David Hunt Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Subject: Re: [dpdk-dev] [PATCH v3 0/8] fix distributor synchronization issues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Lukasz, On Wed, Sep 23, 2020 at 3:25 PM Lukasz Wojciechowski 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