Content-Disposition: inline
Content-Transfer-Encoding: 8bit
Content-Type: text/plain; charset="US-ASCII"
MIME-Version: 1.0
Date: Fri, 11 Feb 2022 17:53:28 +0000
From: "Taylor R Campbell" <riastradh@netbsd.org>
Subject: CVS commit: src/sys/kern
To: source-changes@NetBSD.org
Message-Id: <20220211175328.CAE7DFB24@cvs.NetBSD.org>
Sender: source-changes-owner@NetBSD.org
Precedence: bulk
Reply-To: source-changes-d@NetBSD.org
Mail-Reply-To: "Taylor R Campbell" <riastradh@netbsd.org>
Mail-Followup-To: source-changes-d@NetBSD.org

Module Name:	src
Committed By:	riastradh
Date:		Fri Feb 11 17:53:28 UTC 2022

Modified Files:
	src/sys/kern: subr_copy.c

Log Message:
ucas(9): Membar audit.

- Omit needless membar_enter before ipi_trigger_broadcast.  This was
  presumably intended to imply a happens-before relation for the
  following two CPUs:

	/* CPU doing ucas */
	ucas_critical_enter()
		ucas_critical_pausing_cpus = ncpu - 1	(A)
		ipi_trigger_broadcast()

	/* other CPU walking by whistling innocently */
	IPI handler
	ucas_critical_cpu_gate()
		load ucas_critical_pausing_cpus		(B)

  That is, this was presumably meant to ensure (A) happens-before (B).
  This relation is already guaranteed by ipi(9), so there is no need
  for any explicit memory barrier.

- Issue a store-release in ucas_critical_cpu_gate so we have the
  following happens-before relation which was otherwise not guaranteed
  except if __HAVE_ATOMIC_AS_MEMBAR:

	/* other CPU walking by whistling innocently */
	...other logic touching the target ucas word...	(A)
	IPI handler
	ucas_critical_cpu_gate()
		...
		atomic_dec_uint(&ucas_critical_pausing_cpus)

  happens-before

	/* CPU doing ucas */
	ucas_critical_enter() -> ucas_critical_wait();
	...touching the word with ufetch/ustore...	(B)

  We need to ensure the logic (A) on another CPU touching the target
  ucas word happens-before we actually do the ucas at (B).

  (a) This requires the other CPU to do a store-release on
      ucas_critical_pausing_cpus in ucas_critical_cpu_gate, and

  (b) this requires the ucas CPU to do a load-acquire on
      ucas_critical_pausing_cpus in ucas_critical_wait.

  Without _both_ sides -- store-release and then load-acquire -- there
  is no such happens-before guarantee; another CPU may have a buffered
  store, for instance, that clobbers the ucas.

  For now, do the store-release with membar_exit conditional on
  __HAVE_ATOMIC_AS_MEMBAR and then atomic_dec_uint -- later with the
  C11 API we can dispense with the #ifdef and just use
  atomic_fetch_add_explicit(..., memory_order_release).  The
  load-acquire we can do with atomic_load_acquire.

- Issue a load-acquire in ucas_critical_cpu_gate so we have the
  following happens-before relation which was otherwise not guaranteed:

	/* CPU doing ucas */
	...ufetch/ustore...				(A)
	ucas_critical_exit()
		ucas_critical_pausing_cpus = -1;

	/* other CPU walking by whistling innocently */
	IPI handler
	ucas_critical_cpu_gate()
		...
		while (ucas_critical_pausing_cpus != -1)
			spin;
	...other logic touching the target ucas word...	(B)

  We need to ensure the logic (A) to do the ucas happens-before logic
  that might use it on another CPU at (B).

  (a) This requires that the ucas CPU do a store-release on
      ucas_critical_pausing_cpus in ucas_critical_exit, and

  (b) this requires that the other CPU do a load-acquire on
      ucas_critical_pausing_cpus in ucas_critical_cpu_gate.

  Without _both_ sides -- store-release and then load-acquire -- there
  is no such happens-before guarantee; the other CPU might witness a
  cached stale value of the target location but a new value of some
  other location in the wrong order.

- Use atomic_load/store_* to avoid the appearance of races, e.g. for
  sanitizers.

- Document which barriers pair up with which barriers and what they're
  doing.


To generate a diff of this commit:
cvs rdiff -u -r1.14 -r1.15 src/sys/kern/subr_copy.c

Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.