Page MenuHomeFreeBSD

Document seqc(9)
ClosedPublic

Authored by oshogbo on Aug 16 2018, 11:00 AM.

Details

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
bcr added inline comments.Aug 16 2018, 12:48 PM
share/man/man9/seq.9
119 ↗(On Diff #46766)

s/case reader/case the reader/

oshogbo updated this revision to Diff 46772.Aug 16 2018, 1:25 PM
oshogbo marked 13 inline comments as done.

Thank you.

share/man/man9/seq.9
68 ↗(On Diff #46766)

writers.

70 ↗(On Diff #46766)

I rewrite this part.

0mp requested changes to this revision.Aug 16 2018, 1:47 PM

Possibly a meta question: shouldn't some other manual pages cross reference this manual page?

share/man/man9/seq.9
3 ↗(On Diff #46772)
125 ↗(On Diff #46772)

Missing space before the dot at the end.

This revision now requires changes to proceed.Aug 16 2018, 1:47 PM
oshogbo updated this revision to Diff 46776.Aug 16 2018, 1:52 PM
oshogbo marked 2 inline comments as done.
0mp requested changes to this revision.Aug 16 2018, 2:43 PM

I've run igor and mandoc -Tlint on the manpage and I've found some minor problems.

share/man/man9/seq.9
33 ↗(On Diff #46776)

I guess that this is a convention for section 9 manuals that you only put actual function names in the NAME section, right?

It looks like it is done in a similar way in nv(9).

89 ↗(On Diff #46776)

The Bd macro has to be accompanied by a closing Ed macro.

100 ↗(On Diff #46776)

The Bd macro has to be accompanied by a closing Ed macro.

120 ↗(On Diff #46776)

mandoc -Tlint reports that the AUTHORS section is out of order. According to mdoc(7) the order should be:

...
.Sh STANDARDS
.Sh HISTORY
.Sh AUTHORS
.Sh CAVEATS
.Sh BUGS
This revision now requires changes to proceed.Aug 16 2018, 2:43 PM
oshogbo updated this revision to Diff 46829.Aug 17 2018, 11:30 AM
oshogbo marked 3 inline comments as done.
oshogbo added inline comments.
share/man/man9/seq.9
120 ↗(On Diff #46776)

I don't know if I agree with that.
I much more interested in CAVEATS and BUGS then in AUTHORS.
If there is really such requirements I can change it but I would prefer to stay that in this way.

0mp added inline comments.Aug 17 2018, 12:00 PM
share/man/man9/seq.9
120 ↗(On Diff #46776)

Well, I understand your argument but on the other hand everyone follows this order as this is what mdoc(7) says. By not putting sections in the correct order we will have mandoc -Tlint always complaining about it.

Actually, I always look for BUGS and CAVEATS at the end of the manpage because it's always there.

oshogbo updated this revision to Diff 46832.Aug 17 2018, 12:10 PM
mjg added a comment.Aug 17 2018, 2:59 PM

This page looks fishy, I'll have to think about.

The livelock caveat is trivially fixable, I just could not be arsed to do it. I'll take care of it this weekend. Meanwhile please wait.

0mp accepted this revision.Aug 17 2018, 9:27 PM

Mdoc seems fine.

This revision is now accepted and ready to land.Aug 17 2018, 9:27 PM
oshogbo updated this revision to Diff 57593.May 20 2019, 6:14 PM
oshogbo retitled this revision from Document seq(9) to Document seqc(9).

Refresh it.

This revision now requires review to proceed.May 20 2019, 6:14 PM

Suggested to use Event: Waterloo Hackathon 2019 for commits from here

share/man/man9/seqc.9
55 ↗(On Diff #57593)

has changed

58 ↗(On Diff #57593)

can drop had here

70 ↗(On Diff #57593)

AMD?

oshogbo updated this revision to Diff 57594.May 20 2019, 6:36 PM

Address things pointed out by @emaste

markj added inline comments.May 22 2019, 2:14 PM
share/man/man9/seqc.9
70 ↗(On Diff #57593)

Presumably amd64? I would not even write "modern" in that case.

52 ↗(On Diff #57594)

I think this can be more precise. Something like, seqc allows zero or more readers and zero or one writer to concurrently access an object, providing a consistent snapshot of the object for readers. No mutual exclusion between readers and writers is required, but readers may be starved indefinitely by writers.

54 ↗(On Diff #57594)

This is mixing the interface with the implementation details. I would separate the two: explain the interface first, and the optionally describe the implementation.

75 ↗(On Diff #57594)

typo: sequence

105 ↗(On Diff #57594)

Style: should be indented by a tab.

120 ↗(On Diff #57594)

... for readers.

123 ↗(On Diff #57594)

This part is not true, otherwise it would be a serious flaw. The algorithm does not handle multiple concurrent writers. The caller must provide mutual exclusion.

markj added inline comments.May 22 2019, 2:57 PM
share/man/man9/seqc.9
123 ↗(On Diff #57594)

Sorry, I misunderstood. You are right.

oshogbo updated this revision to Diff 57707.May 22 2019, 5:58 PM
oshogbo marked 6 inline comments as done.
oshogbo added inline comments.
share/man/man9/seqc.9
70 ↗(On Diff #57593)

This is what the comment say in seqc.9

54 ↗(On Diff #57594)

I moved it after discribing interface.

oshogbo updated this revision to Diff 57712.May 22 2019, 7:21 PM

Sorry. Wrong patch.
Thanks @markj

oshogbo updated this revision to Diff 57854.May 24 2019, 6:43 PM

One more nit.

oshogbo marked 4 inline comments as done.May 24 2019, 6:43 PM
markj added inline comments.May 24 2019, 7:39 PM
share/man/man9/seqc.9
55 ↗(On Diff #57854)

The sentence, "The writer functions..." is describing the implementation. I would just get rid of it.

66 ↗(On Diff #57854)

These two sentences aren't really right. They are conflating fences and locking. The point is that seqc(9) always provides the appropriate fencing, but on amd64 (and some other platforms), the implementation does not require any CPU fences at all. Look at atomic_thread_fence_rel() on amd64 for instance.

Consumers of the interface must ensure that two writers are not executing at the same time. It is probably worth stating that explicitly.

72 ↗(On Diff #57854)

Maybe, "If a writer has started a transaction, this function will spin until the transaction has ended."

oshogbo updated this revision to Diff 57858.May 24 2019, 8:32 PM
oshogbo marked 3 inline comments as done.

Fixes proposed by @markj.

markj added a comment.Jun 3 2019, 7:41 PM

This mostly looks ok to me, just some final nits.

share/man/man9/seqc.9
65 ↗(On Diff #57858)

"the current sequence number, beginning a read transaction."

66 ↗(On Diff #57858)

"If a writer is executing a transaction..."

70 ↗(On Diff #57858)

"... compares a sequence number returned by .Fn seqc_read with the current sequence number."

Then the next sentence can be deleted.

76 ↗(On Diff #57858)

"At the end of a read transaction, .Fn seqc_consistent should be used to determine whether a writer updated the object. If so, the read transaction should be restarted. Otherwise, the read was consistent."

84 ↗(On Diff #57858)

"In the following example a writer modifies the .Va var1 and .Va var2 fields in .Va obj ."

95 ↗(On Diff #57858)

These are just two parts of the same example. So I would write,

"The following code will obtain a consistent snapshot of .Va var1 and .Var var2:"

oshogbo updated this revision to Diff 58370.Jun 7 2019, 4:24 PM
oshogbo marked 2 inline comments as done.

Address markj@ suggestions.

mjg added inline comments.Jun 7 2019, 8:36 PM
share/man/man9/seqc.9
121 ↗(On Diff #58370)

It can be noted that a consumer possibly concerned with this can resort to locking after n failed attempts to read.

125 ↗(On Diff #58370)

This is inaccurate. The counter overflowing at some point is not a necessarily a problem, in fact it can keep overflowing without causing any issues. The problem is when it wraps back to the original value, as stated in the comment.

127 ↗(On Diff #58370)

A note can be added that the problem is fixable in practice by extending the counter type to 64 bit.

oshogbo updated this revision to Diff 60075.Jul 24 2019, 3:17 AM

ADdress mjg sugestions.

markj accepted this revision.Jul 29 2019, 3:13 PM
markj added inline comments.
share/man/man9/seqc.9
70 ↗(On Diff #60075)

"compares the sequence number with a previously fetched value."

84 ↗(On Diff #60075)

change -> changes

"var1", "var2" and "obj" should be annotated with .Va.

94 ↗(On Diff #60075)

Same comment as above, with read -> reads.

96 ↗(On Diff #60075)

"In the case where the..."

126 ↗(On Diff #60075)

The wording implies that the consumer can use 64-bit integers, but they can't because we use the opaque seqc_t typedef.

Maybe, "This could be avoided by extending the interface to allow 64-bit counters."

This revision is now accepted and ready to land.Jul 29 2019, 3:13 PM
This revision was automatically updated to reflect the committed changes.