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 B5A56A04B1; Mon, 23 Nov 2020 20:37:11 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 44128C900; Mon, 23 Nov 2020 20:37:09 +0100 (CET) Received: from mail-pl1-f194.google.com (mail-pl1-f194.google.com [209.85.214.194]) by dpdk.org (Postfix) with ESMTP id A5F5BC8E6 for ; Mon, 23 Nov 2020 20:37:06 +0100 (CET) Received: by mail-pl1-f194.google.com with SMTP id l1so1109584pld.5 for ; Mon, 23 Nov 2020 11:37:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=Ec/WniTFcSPOajugApbcJ8thpdJxuM0/14ZvIYGVlyU=; b=S0snDRXDHKfbevfYAhyngdJFeKnlFWuDRUqXZ1gkA2fyM2n7qM0i3mCtPkN1KoEpM8 tL2Teg7D4IciNKTbaFtPKYC3QDZn1LnoxyMf48J705fLdHv6iO+DulamKT7na45Cd1zc uHw76uyR7I5zHeyJQtX0NroV/ehiOS6LVc5l9VUywmZDJxKyUF8p3qZSBtGkIe+6MQ/8 qrGCgRNfOeQF27C7EXoQlLSmcBXSf4x4WltKBNc5ibPd0BZrNgbtH9OlIJK9WWWhe8vs UGp3IsC3dw85ISEFN1loL+ppnyh4W4kS4/YukPARup2eQIVhwOjRBITZUP7LjmLUWmoV cDUQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=Ec/WniTFcSPOajugApbcJ8thpdJxuM0/14ZvIYGVlyU=; b=cjwU4y2apSX8P0Ws1/CKaPJYZMWsXuYhDaK8tBuTjCekz27Q6qU52EWenN/kA1RoZF PtkbzJ8Ynzub1wjCoH89vhU/mfUuvjiZ0eocqOBn3nRdkPhBJ3ygD5x72+DgV0FXoXgT q/SHnstpAmyGOdpNiOURe53N3pTyevkD7jSVEZBotL8rdhXBVLS/shhcBpDw15ueg8dR MY6kKPrw7d3FAF+U7Q6evRK9MLZVsLWqN3TpAkllEtCr/0Ti61oak9ivJLAJIPKAYOZC v8Zx+8ZtQ5HQiae0DPH/VQkS9K0roa/O5XOCYvLYJKjq7/P39NviLEcqdHRw2GTWQ9c6 LVYg== X-Gm-Message-State: AOAM532Z+hYUo4RJ8moZLHclCDT2XOw9LyasRwTQT6gobDSAiB7CayTN /1qh3fF2J27m10WQSI0o6Z5/6A== X-Google-Smtp-Source: ABdhPJxCFukYAJc4061c+9/S8s7kzAD+m+KLadUGpAntKiyIni+fqTxaYf55rz8+LSFCSo8Q61G8Kg== X-Received: by 2002:a17:902:ea85:b029:da:b27:396a with SMTP id x5-20020a170902ea85b02900da0b27396amr910301plb.9.1606160225580; Mon, 23 Nov 2020 11:37:05 -0800 (PST) Received: from hermes.local (204-195-22-127.wavecable.com. [204.195.22.127]) by smtp.gmail.com with ESMTPSA id y10sm169773pjm.34.2020.11.23.11.37.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 23 Nov 2020 11:37:04 -0800 (PST) Date: Mon, 23 Nov 2020 11:36:51 -0800 From: Stephen Hemminger To: Honnappa Nagarahalli Cc: Diogo Behrens , "thomas@monjalon.net" , "david.marchand@redhat.com" , "dev@dpdk.org" , nd Message-ID: <20201123113651.46323c54@hermes.local> In-Reply-To: References: <20200826092002.19395-1-diogo.behrens@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH] librte_eal: fix mcslock hang on weak memory 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" On Mon, 23 Nov 2020 18:29:32 +0000 Honnappa Nagarahalli wrote: > > > > > > The initialization me->locked=1 in lock() must happen before > > next->locked=0 in unlock(), otherwise a thread may hang forever, > > waiting me->locked become 0. On weak memory systems (such as ARMv8), > > the current implementation allows me->locked=1 to be reordered with > > announcing the node (pred->next=me) and, consequently, to be > > reordered with next->locked=0 in unlock(). > > > > This fix adds a release barrier to pred->next=me, forcing > > me->locked=1 to happen before this operation. > > > > Signed-off-by: Diogo Behrens > The change looks fine to me. I have tested this on few x86 and Arm machines. > Acked-by: Honnappa Nagarahalli Maybe a simpler alternative would be as fast and safer. By using compare_exchange you can get same effect in one operation. Like the following UNTESTED. diff --git a/lib/librte_eal/include/generic/rte_mcslock.h b/lib/librte_eal/include/generic/rte_mcslock.h index 78b0df295e2d..9c537ce577e6 100644 --- a/lib/librte_eal/include/generic/rte_mcslock.h +++ b/lib/librte_eal/include/generic/rte_mcslock.h @@ -48,23 +48,23 @@ rte_mcslock_lock(rte_mcslock_t **msl, rte_mcslock_t *me) rte_mcslock_t *prev; /* Init me node */ - __atomic_store_n(&me->locked, 1, __ATOMIC_RELAXED); - __atomic_store_n(&me->next, NULL, __ATOMIC_RELAXED); + me->locked = 1; - /* If the queue is empty, the exchange operation is enough to acquire - * the lock. Hence, the exchange operation requires acquire semantics. - * The store to me->next above should complete before the node is - * visible to other CPUs/threads. Hence, the exchange operation requires - * release semantics as well. + /* + * Atomic insert into single linked list */ - prev = __atomic_exchange_n(msl, me, __ATOMIC_ACQ_REL); + do { + prev = __atomic_load_n(msl, __ATOMIC_RELAXED); + me->next = prev; + } while (!__atomic_compare_exchange_n(&msl, me, prev, + __ATOMIC_ACQUIRE, __ATOMIC_RELAXED)); + if (likely(prev == NULL)) { /* Queue was empty, no further action required, * proceed with lock taken. */ return; } - __atomic_store_n(&prev->next, me, __ATOMIC_RELAXED); /* The while-load of me->locked should not move above the previous * store to prev->next. Otherwise it will cause a deadlock. Need a