From owner-svn-src-all@freebsd.org Fri Sep 4 13:36:34 2020 Return-Path: Delivered-To: svn-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id D6B8F3C2E15 for ; Fri, 4 Sep 2020 13:36:34 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: from mail-wr1-f67.google.com (mail-wr1-f67.google.com [209.85.221.67]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4BjdxZ0nwYz43rt for ; Fri, 4 Sep 2020 13:36:33 +0000 (UTC) (envelope-from jrtc27@jrtc27.com) Received: by mail-wr1-f67.google.com with SMTP id k15so6754382wrn.10 for ; Fri, 04 Sep 2020 06:36:33 -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:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=5xrhtPTmjjeuEYVDZ4OEFtzGfLaJIMXboJ6zads1oJg=; b=kgHjCgtsmaMkaHLKDLg+ISqR1wFMhK4sXbliaJjpnghpD68jb4DjeMTqrXV+6JCV83 edPLYvsonLXA84CuyIxLnf89ZHiuXssLRQ8kPYy4NGtjRhJB9Z/euE7E8xBENMPNggoh ucmupRw6wUo5rXs8OC9tCyUn61sfwD3G3PtxzqxhKUY2WfkqJ5TcekzD7nftj4bLebap ym/lg5LUjh3Ck9uiN8aieVIWUZyUfEdXRfbRB6R9gBpF623YxSka88Q+nMTEXlucPsAn Nh4cWfnYikmQ+30G5Had3ALylWXsseQG516uxbS61QwDzg0JhPaaVJsp3k6gFfRSX+hM HeXw== X-Gm-Message-State: AOAM533HgrEq3oV49LqpZhN4rL07zrS3IKfMqj7wsRvkhTjbEdhdnJtr Nlm4UX1NkpVFfkBCabrAF3Axjg== X-Google-Smtp-Source: ABdhPJz3xMIi3L+prDTIMriSuy5RPADIt4MVC26xJkACFDAKc8/KmjjoNbiNdxnRgDP86vxv4n/14w== X-Received: by 2002:adf:f24d:: with SMTP id b13mr7519752wrp.316.1599226592511; Fri, 04 Sep 2020 06:36:32 -0700 (PDT) Received: from [192.168.149.251] (trinity-students-nat.trin.cam.ac.uk. [131.111.193.104]) by smtp.gmail.com with ESMTPSA id u126sm15308055wmu.9.2020.09.04.06.36.31 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Sep 2020 06:36:31 -0700 (PDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.1\)) Subject: Re: svn commit: r365326 - stable/12/sys/sys From: Jessica Clarke In-Reply-To: <20200904130946.GG94807@kib.kiev.ua> Date: Fri, 4 Sep 2020 14:36:29 +0100 Cc: Marcin Wojtas , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-12@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <202009041122.084BMIYn040628@repo.freebsd.org> <20200904130946.GG94807@kib.kiev.ua> To: Konstantin Belousov X-Mailer: Apple Mail (2.3608.120.23.2.1) X-Rspamd-Queue-Id: 4BjdxZ0nwYz43rt X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of jrtc27@jrtc27.com designates 209.85.221.67 as permitted sender) smtp.mailfrom=jrtc27@jrtc27.com X-Spamd-Result: default: False [-2.21 / 15.00]; RCVD_VIA_SMTP_AUTH(0.00)[]; TO_DN_SOME(0.00)[]; MV_CASE(0.50)[]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; RCPT_COUNT_FIVE(0.00)[6]; RCVD_COUNT_THREE(0.00)[3]; NEURAL_HAM_SHORT(-0.69)[-0.691]; FREEMAIL_TO(0.00)[gmail.com]; FORGED_SENDER(0.30)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; R_DKIM_NA(0.00)[]; MIME_TRACE(0.00)[0:+]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; MID_RHS_MATCH_FROM(0.00)[]; FROM_NEQ_ENVFROM(0.00)[jrtc27@freebsd.org,jrtc27@jrtc27.com]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.996]; FREEFALL_USER(0.00)[jrtc27]; FROM_HAS_DN(0.00)[]; NEURAL_HAM_LONG(-1.02)[-1.022]; MIME_GOOD(-0.10)[text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-all@freebsd.org]; DMARC_NA(0.00)[freebsd.org]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[209.85.221.67:from]; RWL_MAILSPIKE_POSSIBLE(0.00)[209.85.221.67:from]; RCVD_TLS_ALL(0.00)[]; MAILMAN_DEST(0.00)[svn-src-all] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.33 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Sep 2020 13:36:34 -0000 On 4 Sep 2020, at 14:09, Konstantin Belousov = wrote: > On Fri, Sep 04, 2020 at 11:22:18AM +0000, Marcin Wojtas wrote: >> Author: mw >> Date: Fri Sep 4 11:22:18 2020 >> New Revision: 365326 >> URL: https://svnweb.freebsd.org/changeset/base/365326 >>=20 >> Log: >> MFC: r346593 >>=20 >> Add barrier in buf ring peek function to prevent race in ARM and = ARM64. >>=20 >> Obtained from: Semihalf >> Sponsored by: Amazon, Inc. >>=20 >> Modified: >> stable/12/sys/sys/buf_ring.h >> Directory Properties: >> stable/12/ (props changed) >>=20 >> Modified: stable/12/sys/sys/buf_ring.h >> = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D >> --- stable/12/sys/sys/buf_ring.h Fri Sep 4 04:31:56 2020 = (r365325) >> +++ stable/12/sys/sys/buf_ring.h Fri Sep 4 11:22:18 2020 = (r365326) >> @@ -310,14 +310,23 @@ buf_ring_peek_clear_sc(struct buf_ring *br) >> if (!mtx_owned(br->br_lock)) >> panic("lock not held on single consumer dequeue"); >> #endif=09 >> - /* >> - * I believe it is safe to not have a memory barrier >> - * here because we control cons and tail is worst case >> - * a lagging indicator so we worst case we might >> - * return NULL immediately after a buffer has been enqueued >> - */ >> + >> if (br->br_cons_head =3D=3D br->br_prod_tail) >> return (NULL); >> + >> +#if defined(__arm__) || defined(__aarch64__) >> + /* >> + * The barrier is required there on ARM and ARM64 to ensure, = that >> + * br->br_ring[br->br_cons_head] will not be fetched before the = above >> + * condition is checked. >> + * Without the barrier, it is possible, that buffer will be = fetched >> + * before the enqueue will put mbuf into br, then, in the = meantime, the >> + * enqueue will update the array and the br_prod_tail, and the >> + * conditional check will be true, so we will return previously = fetched >> + * (and invalid) buffer. >> + */ >> + atomic_thread_fence_acq(); >> +#endif >=20 > Putting the semantic of the change aside, why did you added the fence = (it is > a fence, not barrier as stated in the comment) only to arm* ? If it = is > needed, it is needed for all arches. Agreed. The code looks fine, though I would have made it an acquire load of br_prod_tail myself to be able to take advantage load-acquire instructions when present, and better document what the exact issue is. I also don't think the comment needs to be quite so extensive (especially since atomic_load_acq_32 is somewhat self-documenting in terms of one half of the race); if we had a comment like this for every fence in the kernel we'd never get anything done. There's also an ARM-specific fence in buf_ring_dequeue_sc: > /* > * This is a workaround to allow using buf_ring on ARM and = ARM64. > * ARM64TODO: Fix buf_ring in a generic way. > * REMARKS: It is suspected that br_cons_head does not require > * load_acq operation, but this change was extensively tested > * and confirmed it's working. To be reviewed once again in > * FreeBSD-12. > * > * Preventing following situation: >=20 > * Core(0) - buf_ring_enqueue() = Core(1) - buf_ring_dequeue_sc() > * ----------------------------------------- = ---------------------------------------------- > * > * = cons_head =3D br->br_cons_head; > * atomic_cmpset_acq_32(&br->br_prod_head, ...)); > * = buf =3D br->br_ring[cons_head]; > > * br->br_ring[prod_head] =3D buf; > * atomic_store_rel_32(&br->br_prod_tail, ...); > * = prod_tail =3D br->br_prod_tail; > * = if (cons_head =3D=3D prod_tail)=20 > * = return (NULL); > * = `=09 > * > * <1> Load (on core 1) from br->br_ring[cons_head] can be = reordered (speculative readed) by CPU. > */=09 > #if defined(__arm__) || defined(__aarch64__) > cons_head =3D atomic_load_acq_32(&br->br_cons_head); > #else > cons_head =3D br->br_cons_head; > #endif > prod_tail =3D atomic_load_acq_32(&br->br_prod_tail); The comment is completely correct that the ARM-specific fence is a waste of time. It's the single-consumer path, so such fences are just synchronising with the current thread and thus pointless. The important one is the load-acquire of br_prod_tail, as has been discovered (sort of) in the peek case leading to this comment, which already stops the reordering in question. Jess