From: Ingo Schwarze <schwarze@usta.de>
Subject: Re: producer/consumer locking
To: David Gwynne <david@gwynne.id.au>
Cc: tech@openbsd.org
Date: Sun, 4 May 2025 20:06:17 +0200

Hello David,

a few suggestions on the manual page, all optional, use or ignore as you
see fit.  I do not presime to really understand what is going on here.

David Gwynne wrote on Sun, May 04, 2025 at 05:20:41PM +1000:

> Index: share/man/man9/pc_lock_init.9
> ===================================================================
> RCS file: share/man/man9/pc_lock_init.9
> diff -N share/man/man9/pc_lock_init.9
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ share/man/man9/pc_lock_init.9	4 May 2025 07:18:11 -0000
> @@ -0,0 +1,212 @@
> +.\" $OpenBSD$
> +.\"
> +.\" Copyright (c) 2025 David Gwynne <dlg@openbsd.org>
> +.\" All rights reserved.
> +.\"
> +.\" Permission to use, copy, modify, and distribute this software for any
> +.\" purpose with or without fee is hereby granted, provided that the above
> +.\" copyright notice and this permission notice appear in all copies.
> +.\"
> +.\" THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
> +.\" WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> +.\" MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
> +.\" ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
> +.\" WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
> +.\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
> +.\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> +.\"
> +.Dd $Mdocdate: November 4 2019 $
> +.Dt PC_LOCK_INIT 9
> +.Os
> +.Sh NAME
> +.Nm pc_lock_init ,
> +.Nm pc_cons_enter ,
> +.Nm pc_cons_leave ,
> +.Nm pc_sprod_enter ,
> +.Nm pc_sprod_leave ,
> +.Nm pc_mprod_enter ,
> +.Nm pc_mprod_leave ,
> +.Nm PC_LOCK_INITIALIZER
> +.Nd producer/consumer locks
> +.Sh SYNOPSIS
> +.In sys/pclock.h
> +.Ft void
> +.Fn pc_lock_init "struct pc_lock *pcl"
> +.Ft void
> +.Fn pc_cons_enter "struct pc_lock *pcl" "unsigned int *genp"
> +.Ft int
> +.Fn pc_cons_leave "struct pc_lock *pcl" "unsigned int *genp"
> +.Ft unsigned int
> +.Fn pc_sprod_enter "struct pc_lock *pcl"
> +.Ft void
> +.Fn pc_sprod_leave "struct pc_lock *pcl" "unsigned int gen"
> +.Ft unsigned int
> +.Fn pc_mprod_enter "struct pc_lock *pcl"
> +.Ft void
> +.Fn pc_mprod_leave "struct pc_lock *pcl" "unsigned int gen"
> +.Fn PC_LOCK_INITIALIZER
> +.Sh DESCRIPTION
> +The producer/consumer lock functions provide mechanisms for a
> +consumer to read data without blocking or delaying another CPU or
> +an interrupt when it is updating or producing data.
> +A variant of the producer locking functions provides mutual exclusion
> +between multiple producers.
> +.Pp
> +This is implemented by having producers version the protected data
> +with a generation number.
> +Consumers of the data compare the generation number at the start
> +of the critical section to the generation number at the end, and
> +must retry reading the data if the generation number has changed.
> +.Pp
> +The
> +.Fn pc_lock_init
> +function is used to initialise the producer/consumer lock pointed to by
> +.Fa pcl .

Suggestion for conciseness:
s/is used to initialise/initialises/

Suggestion for precision:

  ... pointed to by
  .Fa pcl
  to contain the generation number 0.

> +.Pp
> +A producer/consumer lock declaration may be initialised with the
> +.Fn PC_LOCK_INITIALIZER
> +macro.

This confused me quite a bit.  Clearly, the syntax differs, but is
there a difference in purpose compared to pc_lock_init(3)?
Besides, it is clearly not the *declaration* that is initialised,
but some object - but which object?

Should you maybe say something like the following?

  .Pp
  Alternatively, a
  .Vt struct pc_lock
  object can be initialized to contain the generation number 0
  by assigning
  .Fn PC_LOCK_INITIALIZER
  to it when declaring it.

That way, it would seem clearer that the purpose is not substantially
different, and what the syntax actually is.

> +.Ss Consumer API
> +.Fn pc_cons_enter
> +reads the current generation number from
> +.Fa pcl
> +and stores it in the memory provided by the caller via
> +.Fa genp .

I suggest to insert here:
This function is typically called at the start of the critical section.

> +.Pp
> +.Fn pc_cons_leave
> +compares the generation number in
> +.Fa pcl
> +with the value stored in
> +.Fa genp

Here i suggest to put a full stop.

> +by
> +.Fn pc_cons_enter
> +at the start of the critical section,

I see no need to repeat this; exactly the same was said in the sentence
right before.  Also, it is slightly misplaced.  Right now, you are
documenting what pc_cons_leave(9) does, and how *genp was filled is
not part of that.

> and returns whether the reads
> +within the critical section need to be retried because the data has
> +been updated by the producer.

IIUC, this is intended to explain how to use the function, not what it
does, so i suggest saying something like:

  If the return value indicates a mismatch,
  the data has been updated by the producer,
  so the read operations within the critical section need to be retried.

> +.Ss Single Producer API
> +The single producer API is optimised for updating data from code

s/$/./

> +.Pp
> +.Fn pc_sprod_enter
> +marks the beginning of a single producer critical section for the
> +.Fa pcl
> +producer/consumer lock.
> +.Pp
> +.Fn pc_sprod_leave
> +marks the end of a single producer critical section for the
> +.Fa pcl
> +producer/consumer lock.
> +The
> +.Fa gen
> +argument must be the value returned from the preceding
> +.Fn pc_sprod_enter
> +call.
> +.Ss Multiple Producer API
> +The multiple producer API provides mutual exclusion between multiple
> +CPUs entering the critical section concurrently.
> +Unlike
> +.Xr mtx_enter 9 ,
> +the multiple producer

Is the word "API" missing here?  I'm not sure.

> does not prevent preemption by interrupts,
> +it only provides mutual exclusion between CPUs.
> +If protection from preemption is required,
> +.Xr splraise 9
> +can be used to protect the producer critical section.
> +.Pp
> +.Fn pc_mprod_enter
> +marks the beginning of a single producer critical section for the

Is that a copy-and-paste oversight?

Maybe  s/single/multiple/  ?

> +.Fa pcl
> +producer/consumer lock.
> +.Pp
> +.Fn pc_mprod_leave
> +marks the end of a single producer critical section for the

s/single/multiple/  ?

> +.Fa pcl
> +producer/consumer lock.
> +The
> +.Fa gen
> +argument must be the value returned from the preceding
> +.Fn pc_mprod_enter
> +call.
> +.Pp
> +On uniprocessor kernels the multiple producer API is aliased to the
> +single producer API.
> +.Sh CONTEXT
> +.Fn pc_lock_init ,
> +.Fn pc_cons_enter ,
> +.Fn pc_cons_leave ,
> +.Fn pc_sprod_enter ,
> +.Fn pc_sprod_leave ,
> +.Fn pc_mprod_enter ,

Insert the line

  and

here.

> +.Fn pc_mprod_leave ,

s/ ,//

Alternatively, you could simply say:

  All these functions

> +can be called during autoconf, from process context, or from interrupt context.

Even if someone thinks "all these functions" includes PC_LOCK_INITIALIZER(),
i expect that's still true.

> +.Pp
> +.Fn pc_sprod_enter ,
> +.Fn pc_sprod_leave ,
> +.Fn pc_mprod_enter ,
> +and
> +.Fn pc_mprod_leave
> +may run concurrently with (ie, on another CPU to)

s/ie/i.e./
and again right below.

> +or preempt (ie, run at a higher interrupt level) than

s/) than/ than)/

> +.Fn pc_cons_enter
> +and
> +.Fn pc_cons_leave .
> +.Pp
> +.Fn pc_sprod_enter ,
> +.Fn pc_sprod_leave ,
> +.Fn pc_mprod_enter ,
> +and
> +.Fn pc_mprod_leave
> +must not be preempted or interrupted by the producer or consumer
> +API for the same lock.
> +.Sh RETURN VALUES
> +.Fn pc_cons_leave
> +returns 0 if the critical section did not overlap with an update
> +from a producer, or non-zero if the critical section must be retried.

I don't like this, the RETURN VALUES section should specify what the API
returns, not how the API is supposed to be used.  The latter belongs
in the decsription.

Maybe something like:

  .Fn pc_cons_leave
  returns 0 if
  .Pf * Fa genp
  matches the generation number stored in
  .Fa pcl ,
  or non-zero otherwise.

Besides, two functions are missing.  Maybe something like:

  .Pp
  .Fn pc_sprod_enter
  and
  .Fn pc_mprod_enter
  generate and return a new generation number.

> +.Sh EXAMPLES
> +To produce or update data:

It would be better if the sentences introducing the examples all were
of the same form.  I like the imperative best because it is simplest
and most concise, for example:

  Produce or update data:
  Consistently read data from a consumer:

Alternative, you could use the infinitive:

  To produce or update data:
  To consistently read data from a consumer:

A wording using a substantive might also be possible, but feels
more complicated.

> +.Bd -literal -offset indent
> +struct pc_lock pc = PC_LOCK_INITIALIZER();
> +
> +void
> +producer(void)
> +{
> +	unsigned int gen;
> +
> +	gen = pc_sprod_enter(&pc);
> +	/* update data */
> +	pc_sprod_leave(&pc, gen);
> +}
> +.Ed
> +.Pp
> +A consistent read of the data from a consumer:

I suggest making this line consistent, see above.

> +.Bd -literal -offset indent
> +void
> +consumer(void)
> +{
> +	unsigned int gen;
> +
> +	pc_cons_enter(&pc, &gen);
> +	do {
> +		/* read data */
> +	} while (pc_cons_leave(&pc, &gen) != 0);
> +}
> +.Ed
> +.Sh SEE ALSO
> +.Xr mutex 9 ,
> +.Xr splraise 9
> +.Sh HISTORY
> +The
> +.Nm
> +functions first appeared in
> +.Ox 7.8 .
> +.Sh AUTHORS
> +The
> +.Nm
> +functions were written by
> +.An David Gwynne Aq Mt dlg@openbsd.org .
> +.Sh CAVEATS
> +Updates must be produced infrequently enough to allow time for
> +consumers to be able to get a consistent read without looping too
> +often.
> +.Pp
> +Because consuming the data may loop when retrying, care must be
> +taken to avoid side effects from reading the data multiple times,
> +eg, when accumulating values.

s/eg/e.g./

or maybe s/eg,/for example/ might read even better.

Yours,
  Ingo