From owner-freebsd-net@FreeBSD.ORG Tue Feb 17 23:28:34 2015 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id B6168984 for ; Tue, 17 Feb 2015 23:28:34 +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 8D7646D6 for ; Tue, 17 Feb 2015 23:28:34 +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 t1HNSYkO082914 for ; Tue, 17 Feb 2015 23:28:34 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 t1HNSYgf082913; Tue, 17 Feb 2015 23:28:34 GMT (envelope-from root) Date: Tue, 17 Feb 2015 23:28:34 +0000 To: freebsd-net@freebsd.org From: "wblock (Warren Block)" Subject: [Differential] [Changed Subscribers] D1438: FreeBSD callout rewrite and cleanup Message-ID: <55424bb6afde62db71124c0bd96403f3@localhost.localdomain> X-Priority: 3 Thread-Topic: D1438: FreeBSD callout rewrite / cleanup X-Herald-Rules: none X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: YzU3ODk0MGM0Y2E4NmE3NjY4YjJlZmFkM2UyIFTjzqI= 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-net@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Feb 2015 23:28:34 -0000 wblock added a subscriber: wblock. wblock added a comment. This is as much as I have time for at the moment. I'll add doc to the reviewers list INLINE COMMENTS share/man/man9/timeout.9:73 That is kind of a difficult sentence to parse. Does this retain the meaning? API is used to schedule a one-time call to an arbitrary function at a specific future time. share/man/man9/timeout.9:76 The original is an "aside": Consumers of this API are required to allocate a callout structure (struct callout) for each pending function invocation. The parentheses are probably the wrong markup for the new form, and "struct callout structure" is redundant. Maybe this: Consumers of this API are required to allocate a .Ft struct callout for each pending function invocation. share/man/man9/timeout.9:79 As above, use .Ft and the word "structure" is redundant. share/man/man9/timeout.9:80 "should be" is a recommendation. Does it mean I ought to call those functions before freeing, but will work fine without it? Otherwise, consider "must be drained by a call to". share/man/man9/timeout.9:85 The rule is to use American English spellings unless the entire document was written with British spellings. See https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style-guidelines.html share/man/man9/timeout.9:88 It would be better to say .Pq Deprecated, see blahblah for current usage. This function is used to prepare a share/man/man9/timeout.9:90 Use .Ft and remove the redundant word "structure". share/man/man9/timeout.9:94 "Was" is a little odd here, but "were" is not quite right either. How about: function will return as if no timeout was pending. share/man/man9/timeout.9:97 Again, better to say the function is deprecated and give a pointer to the replacement in a separate sentence. Then explain what this one does in the next sentence. share/man/man9/timeout.9:101 .Ft and s/structure// share/man/man9/timeout.9:107 .Ft and s/structure// share/man/man9/timeout.9:114 Possessive: application's share/man/man9/timeout.9:116 Comma: functions, including the share/man/man9/timeout.9:121 s/Else/Otherwise,/ share/man/man9/timeout.9:122 Avoid simultaneous access by obtaining an exclusive lock share/man/man9/timeout.9:132 Split this long sentence: function is called. It is also expected, but currently not share/man/man9/timeout.9:135 This last part needs to be split out into a separate sentence, starting with ", except when the". share/man/man9/timeout.9:144 .Ft and s/structure// share/man/man9/timeout.9:149 Passive "should be" and should, and "specify a pointer"... How about: argument is a non-zero pointer to a valid share/man/man9/timeout.9:153 The specified mutex is expected to be locked when calling any share/man/man9/timeout.9:155 functions other than the share/man/man9/timeout.9:159 functions. share/man/man9/timeout.9:165 The callout function is assumed to have released the specified share/man/man9/timeout.9:167 s/Else/Otherwise,/ share/man/man9/timeout.9:173 This function is similar to the share/man/man9/timeout.9:175 function, but accepts a read-mostly type of lock. share/man/man9/timeout.9:176 s/initialised/initialized/ share/man/man9/timeout.9:182 This function is similar to the share/man/man9/timeout.9:184 function, but accepts a reader-writer type of lock. Should that be "read/write" instead? share/man/man9/timeout.9:188 As above, separate sentence to say it is deprecated and point to the replacement. share/man/man9/timeout.9:199 argument is a valid pointer to a function that takes a single share/man/man9/timeout.9:223 "canceled" is spelled "cancelled" elsewhere in this man page. The first is modern American. share/man/man9/timeout.9:243 s/Else/Otherwise,/ share/man/man9/timeout.9:246 s/called/called,/ share/man/man9/timeout.9:249 s/words/words,/ share/man/man9/timeout.9:250 s/Else/Otherwise,/ share/man/man9/timeout.9:273 s/Else/Otherwise,/ share/man/man9/timeout.9:295 s/Else/Otherwise,/ share/man/man9/timeout.9:309 .Fa pr argument. is redundant, and can be just .Fa pr . (There are lots of these.) share/man/man9/timeout.9:310 Avoid the informal use of "you": This function is used when high precision timeouts are needed. share/man/man9/timeout.9:313 Again, redundant. Can be just If .Fa sbt specifies... share/man/man9/timeout.9:324 "the" is not needed here. share/man/man9/timeout.9:334 Again, can just be By default, .Fa sbt is treated... Also, "is treated *as*". share/man/man9/timeout.9:346 Needs a serial comma before "and": ...1/4, and so on. share/man/man9/timeout.9:362 This function works like share/man/man9/timeout.9:363 .Fn callout_reset_sbt , share/man/man9/timeout.9:364 except the callback function given by share/man/man9/timeout.9:366 will be executed on the CPU which called this function. share/man/man9/timeout.9:375 Otherwise, the callback function given by share/man/man9/timeout.9:377 will be executed on the same CPU... The rest of that line is unclear. As shown previously in the man page? As run previously on the CPU? share/man/man9/timeout.9:382 This function works like .Fn callout_reset_sbt , except the callback function given by .Fa func will be executed on the CPU given by .Fa cpu . share/man/man9/timeout.9:392 Refer to .Fa callout_init . REVISION DETAIL https://reviews.freebsd.org/D1438 To: hselasky, jhb, adrian, markj, emaste, sbruno, imp, lstewart, rwatson, gnn, rrs, kostikbel, delphij, neel, erj Cc: wblock, freebsd-net