From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0AD51A00C2; Sun, 3 Apr 2022 08:51:43 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A2C1D4068A; Sun, 3 Apr 2022 08:51:42 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 38F2840041 for ; Sun, 3 Apr 2022 08:51:41 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id ED56C13374 for ; Sun, 3 Apr 2022 08:51:40 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id EB8FF13BA9; Sun, 3 Apr 2022 08:51:40 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-3.9 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.6 X-Spam-Score: -3.9 Received: from [192.168.1.59] (unknown [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id BED5213373; Sun, 3 Apr 2022 08:51:39 +0200 (CEST) Message-ID: <17b2cdec-a56f-9069-c3ec-13f31fd72f57@lysator.liu.se> Date: Sun, 3 Apr 2022 08:51:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.7.0 Subject: Re: [PATCH v3] eal: add seqlock Content-Language: en-US To: Ola Liljedahl , dev@dpdk.org Cc: Thomas Monjalon , David Marchand , onar.olsen@ericsson.com, Honnappa.Nagarahalli@arm.com, nd@arm.com, konstantin.ananyev@intel.com, mb@smartsharesystems.com, stephen@networkplumber.org References: <20220401150749.136921-1-mattias.ronnblom@ericsson.com> <3efba44b-1a2e-831e-1e2c-782639d83c24@arm.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: <3efba44b-1a2e-831e-1e2c-782639d83c24@arm.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2022-04-02 20:15, Ola Liljedahl wrote: > On 4/1/22 17:07, Mattias Rönnblom wrote: >> + >> +/** >> + * End a read-side critical section. >> + * >> + * A call to this function marks the end of a read-side critical >> + * section, for @p seqlock. The application must supply the sequence >> + * number produced by the corresponding rte_seqlock_read_lock() (or, >> + * in case of a retry, the rte_seqlock_tryunlock()) call. >> + * >> + * After this function has been called, the caller should not access >> + * the protected data. >> + * >> + * In case this function returns true, the just-read data was >> + * consistent and the set of atomic and non-atomic load operations >> + * performed between rte_seqlock_read_lock() and >> + * rte_seqlock_read_tryunlock() were atomic, as a whole. >> + * >> + * In case rte_seqlock_read_tryunlock() returns false, the data was >> + * modified as it was being read and may be inconsistent, and thus >> + * should be discarded. The @p begin_sn is updated with the >> + * now-current sequence number. >> + * >> + * @param seqlock >> + *   A pointer to the seqlock. >> + * @param begin_sn >> + *   The seqlock sequence number returned by >> + *   rte_seqlock_read_lock() (potentially updated in subsequent >> + *   rte_seqlock_read_tryunlock() calls) for this critical section. >> + * @return >> + *   true or false, if the just-read seqlock-protected data was >> consistent >> + *   or inconsistent, respectively, at the time it was read. >> + * >> + * @see rte_seqlock_read_lock() >> + */ >> +__rte_experimental >> +static inline bool >> +rte_seqlock_read_tryunlock(const rte_seqlock_t *seqlock, uint32_t >> *begin_sn) >> +{ >> +    uint32_t end_sn; >> + >> +    /* make sure the data loads happens before the sn load */ >> +    rte_atomic_thread_fence(__ATOMIC_ACQUIRE); >> + >> +    end_sn = __atomic_load_n(&seqlock->sn, __ATOMIC_RELAXED); > > Since we are reading and potentially returning the sequence number here > (repeating the read of the protected data), we need to use load-acquire. > I assume it is not expected that the user will call > rte_seqlock_read_lock() again. > Good point. > Seeing this implementation, I might actually prefer the original > implementation, I think it is cleaner. Me too. > But I would like for the begin > function also to wait for an even sequence number, the end function > would only have to check for same sequence number, this might improve > performance a little bit as readers won't perform one or several broken > reads while a write is in progress. The function names are a different > thing though. > The odd sn should be a rare case, if the seqlock is used for relatively low frequency update scenarios, which is what I think it should be designed for. Waiting for an even sn in read_begin() would exclude the option for the caller to defer reading the new data to same later time, in case it's being written. That in turn would force even a single writer to make sure its thread is not preempted, or risk blocking all lcore worker cores attempting to read the protected data. You could complete the above API with a read_trybegin() function to address that issue, for those who care, but that would force some extra complexity on the user. > The writer side behaves much more like a lock with mutual exclusion so > write_lock/write_unlock makes sense. > >> + >> +    if (unlikely(end_sn & 1 || *begin_sn != end_sn)) { >> +        *begin_sn = end_sn; >> +        return false; >> +    } >> + >> +    return true; >> +} >> +