From owner-freebsd-arm@FreeBSD.ORG Sat Feb 21 06:34:42 2015 Return-Path: Delivered-To: freebsd-arm@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 46D40B21 for ; Sat, 21 Feb 2015 06:34:42 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [IPv6:2001:4f8:3:ffe0:406a:0:50:2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 09F21AD7 for ; Sat, 21 Feb 2015 06:34:42 +0000 (UTC) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [127.0.1.5]) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9) with ESMTP id t1L6YfZq044039 for ; Sat, 21 Feb 2015 06:34:41 GMT (envelope-from root@phabric-backend.isc.freebsd.org) Received: (from root@localhost) by phabric-backend.isc.freebsd.org (8.14.9/8.14.9/Submit) id t1L6Yft3044038; Sat, 21 Feb 2015 06:34:41 GMT (envelope-from root) Date: Sat, 21 Feb 2015 06:34:41 +0000 To: freebsd-arm@freebsd.org From: "meloun-miracle-cz (Michal Meloun)" Subject: [Differential] [Changed Subscribers] D1833: Add memory barriers to buf_ring Message-ID: X-Priority: 3 Thread-Topic: D1833: Add memory barriers to buf_ring X-Herald-Rules: none X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: OGRiNDkxY2NmMjRiNTc0MjQ4YTYwNWVkNzIyIFToJwE= X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: , MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 21 Feb 2015 06:34:42 -0000 meloun-miracle-cz added a subscriber: meloun-miracle-cz. meloun-miracle-cz added a comment. Even I’m not able to evaluate all aspect, I see some serious defects here - at least from ARM point of view. The buf_ring_enqueue() guarantees proper write ordering (and visibility): - Store with acquire to br->br_prod_head - Normal store to br_ring[] - Store with release to br_prod_tail. Unfortunately buf_ring_dequeue_sc() have not defined any read ordering and code can see updated br_prod_tail and stale br_ring[]. (imho, this is true for amd64/i386 too). Unlike of Semihalf guys, I see little different solution for race in buf_ring_dequeue_sc() read logic. The br_ring[] must be read after br_prod_tail, but read order of br_cons_head and br_prod_tail is not important. So (line 183) buf = atomic_load_rel_32(&br->br_ring[cons_head]); looks more correct to me. On ARM, all stores to variable referenced by atomic_cmpset() must be done by atomic_store (or by other atomic_* functions), normal store doesn’t clear exclusive monitor. Thus, for ARM we **!!MUST!!** use atomic_store() for each store to variable referenced by atomic_cmpset() ! Ohh, and note – current atomic_store() implementation on ARM is broken too, but fix is easy (see atomic store_*_64(). REVISION DETAIL https://reviews.freebsd.org/D1833 To: zbb, kmacy, rpaulo, imp Cc: meloun-miracle-cz, onwahe-gmail-com, andrew, ian, adrian, freebsd-arm