From owner-svn-src-head@freebsd.org Wed Sep 16 20:54:50 2015 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 185CE9CEA04 for ; Wed, 16 Sep 2015 20:54:50 +0000 (UTC) (envelope-from rrs@netflix.com) Received: from mail-pa0-x22b.google.com (mail-pa0-x22b.google.com [IPv6:2607:f8b0:400e:c03::22b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D3AF81DC1 for ; Wed, 16 Sep 2015 20:54:49 +0000 (UTC) (envelope-from rrs@netflix.com) Received: by pacex6 with SMTP id ex6so219425420pac.0 for ; Wed, 16 Sep 2015 13:54:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=netflix.com; s=google; h=content-type:mime-version:subject:from:in-reply-to:date:cc :message-id:references:to; bh=WSQ1Qgqyj+1LNw2wDWbeHuBfNOmguEJre+DC0jUSsFk=; b=IHXkxw1LFxT1KvIQ2s2Rwxl/0qXEwW5U0TJgqUVAhsZl8wg8l+M35gDAdRrKcTZ9D6 ewfZB/FW5icVJjXJ+fQbfVS5ZKutqN/LdnnyPi6RYkzaAGLQ3vc2oghk3NHHzegQ34kt RSUiLPIHKn+Ofz56c4BgLti2eQAxfLEitNbOE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:content-type:mime-version:subject:from :in-reply-to:date:cc:message-id:references:to; bh=WSQ1Qgqyj+1LNw2wDWbeHuBfNOmguEJre+DC0jUSsFk=; b=eoiJeewr+mSW1MFrSRDkSsEG5LMB/hoXliP/JACFyckjs/eVmMWRxAV3IzhofHNe69 Su6MoqRNawymPa0G+VocDCjZuD894GsvKkWfqzu7+W+IAjxlixZ9p0WYNXFKhDLp9NZI 5u4iN7URL0VZuG8DBFXo/Nyu/0tQmsimHyc8+wIUlkWKIiRe2iDAzDh4ULdFHGETBC3h ANuSF3ZZa7WQn4IisdqX2elXQTXSQl5rTFG+mEvAxAWDwUOuJgxRk9AMH8ZCD8hFAOUQ 5gufzOK96EUGfRO0KWaG0EGHMRhohNHiu4zUF94FDBmRThNeO1SP1iChleef19MdECSO P9iA== X-Gm-Message-State: ALoCoQlowv4nfMr9Pd/x9HuZ1419CIpw4IOhLdxETBApbp7M3f90yqJKVb9bEJuHmKanCnhVIarK X-Received: by 10.68.109.2 with SMTP id ho2mr64533732pbb.158.1442436889355; Wed, 16 Sep 2015 13:54:49 -0700 (PDT) Received: from ?IPv6:2607:fb10:16:208:9168:1ab3:84c7:f324? ([2607:fb10:16:208:9168:1ab3:84c7:f324]) by smtp.gmail.com with ESMTPSA id lo10sm29624106pab.16.2015.09.16.13.54.48 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 16 Sep 2015 13:54:48 -0700 (PDT) Mime-Version: 1.0 (Mac OS X Mail 7.3 \(1878.6\)) Subject: Re: svn commit: r287780 - in head: share/man/man9 sys/kern sys/sys From: Randall Stewart In-Reply-To: <201509141052.t8EAqRWf008293@repo.freebsd.org> Date: Wed, 16 Sep 2015 13:54:47 -0700 Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Message-Id: References: <201509141052.t8EAqRWf008293@repo.freebsd.org> To: Hans Petter Selasky X-Mailer: Apple Mail (2.1878.6) Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.20 X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Sep 2015 20:54:50 -0000 Hans: By outside prompting, I have finally had a chance to look at this: What it appears you do here is: a) stop the callout, not paying attention to if it stopped or not. b) Then get the callout systems lock and check if your callout is up and = running, storing that in your retval. Where 1 is it was running, and 0 is no it was not running. c) Assuming that your callout is running. 1) assert the user has the lock (if its a mtx associated lock), which = means the callout is blocked on you finishing this. 2) change the nature of the callout so that it will return-unlocked = (no matter what the caller thought he set in his callout). 3) Start a timeout using this same callout that calls the function = (async drain function) that the user supplied *instead* of the original callout. Now this is *not* how I would have done it and I question c.2 = especially. I don=92t think you should be changing the nature of the callout and its lock. =20 Overall I really doubt this will work well since the callout that you = start will call to the user function that says =93I am done with the callout memory.. which is = usually what you want this async-drain for=94 but using the very memory that you want to free for the callout. I wonder how tested this code is? =20 I would think one would be better off having a way to set a callback if = you are trying to stop and drain-async that the actual callout code itself calls to say =93I am done=94.. not = this interesting recursive use of the callout itself. R On Sep 14, 2015, at 3:52 AM, Hans Petter Selasky = wrote: > Author: hselasky > Date: Mon Sep 14 10:52:26 2015 > New Revision: 287780 > URL: https://svnweb.freebsd.org/changeset/base/287780 >=20 > Log: > Implement callout_drain_async(), inspired by the projects/hps_head > branch. >=20 > This function is used to drain a callout via a callback instead of > blocking the caller until the drain is complete. Refer to the > callout_drain_async() manual page for a detailed description. >=20 > Limitation: If a lock is used with the callout, the callout can only > be drained asynchronously one time unless the callout_init_mtx() > function is called again. This limitation is not present in > projects/hps_head and will require more invasive changes to the > timeout code, which was not in the scope of this patch. >=20 > Differential Revision: https://reviews.freebsd.org/D3521 > Reviewed by: wblock > MFC after: 1 month >=20 > Modified: > head/share/man/man9/Makefile > head/share/man/man9/timeout.9 > head/sys/kern/kern_timeout.c > head/sys/sys/_callout.h > head/sys/sys/callout.h >=20 > Modified: head/share/man/man9/Makefile > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > --- head/share/man/man9/Makefile Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/share/man/man9/Makefile Mon Sep 14 10:52:26 2015 = (r287780) > @@ -1641,6 +1641,7 @@ MLINKS+=3Dtimeout.9 callout.9 \ > timeout.9 callout_active.9 \ > timeout.9 callout_deactivate.9 \ > timeout.9 callout_drain.9 \ > + timeout.9 callout_drain_async.9 \ > timeout.9 callout_handle_init.9 \ > timeout.9 callout_init.9 \ > timeout.9 callout_init_mtx.9 \ >=20 > Modified: head/share/man/man9/timeout.9 > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > --- head/share/man/man9/timeout.9 Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/share/man/man9/timeout.9 Mon Sep 14 10:52:26 2015 = (r287780) > @@ -36,6 +36,7 @@ > .Nm callout_active , > .Nm callout_deactivate , > .Nm callout_drain , > +.Nm callout_drain_async , > .Nm callout_handle_init , > .Nm callout_init , > .Nm callout_init_mtx , > @@ -70,6 +71,8 @@ typedef void timeout_t (void *); > .Fn callout_deactivate "struct callout *c" > .Ft int > .Fn callout_drain "struct callout *c" > +.Ft int > +.Fn callout_drain_async "struct callout *c" "callout_func_t *fn" = "void *arg" > .Ft void > .Fn callout_handle_init "struct callout_handle *handle" > .Bd -literal > @@ -264,6 +267,24 @@ fully stopped before > .Fn callout_drain > returns. > .Pp > +The function > +.Fn callout_drain_async > +is non-blocking and works the same as the > +.Fn callout_stop > +function. > +When this function returns non-zero, do not call it again until the = callback function given by > +.Fa fn > +has been called with argument > +.Fa arg . > +Only one of > +.Fn callout_drain > +or > +.Fn callout_drain_async > +should be called at a time to drain a callout. > +If this function returns zero, it is safe to free the callout = structure pointed to by the > +.Fa c > +argument immediately. > +.Pp > The > .Fn callout_reset > and >=20 > Modified: head/sys/kern/kern_timeout.c > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > --- head/sys/kern/kern_timeout.c Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/kern/kern_timeout.c Mon Sep 14 10:52:26 2015 = (r287780) > @@ -1145,6 +1145,45 @@ callout_schedule(struct callout *c, int=20 > } >=20 > int > +callout_drain_async(struct callout *c, callout_func_t *func, void = *arg) > +{ > + struct callout_cpu *cc; > + struct lock_class *class; > + int retval; > + int direct; > + > + /* stop callout */ > + callout_stop(c); > + > + /* check if callback is being called */ > + cc =3D callout_lock(c); > + if (c->c_iflags & CALLOUT_DIRECT) { > + direct =3D 1; > + } else { > + direct =3D 0; > + } > + retval =3D (cc_exec_curr(cc, direct) =3D=3D c); > + > + /* drop locks, if any */ > + if (retval && c->c_lock !=3D NULL && > + c->c_lock !=3D &Giant.lock_object) { > + /* ensure we are properly locked */ > + class =3D LOCK_CLASS(c->c_lock); > + class->lc_assert(c->c_lock, LA_XLOCKED); > + /* the final callback should not be called locked */ > + c->c_lock =3D NULL; > + c->c_iflags |=3D CALLOUT_RETURNUNLOCKED; > + } > + CC_UNLOCK(cc); > + > + /* check if we should queue final callback */ > + if (retval) > + callout_reset(c, 1, func, arg); > + > + return (retval); > +} > + > +int > _callout_stop_safe(struct callout *c, int safe) > { > struct callout_cpu *cc, *old_cc; >=20 > Modified: head/sys/sys/_callout.h > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > --- head/sys/sys/_callout.h Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/sys/_callout.h Mon Sep 14 10:52:26 2015 = (r287780) > @@ -46,6 +46,8 @@ LIST_HEAD(callout_list, callout); > SLIST_HEAD(callout_slist, callout); > TAILQ_HEAD(callout_tailq, callout); >=20 > +typedef void callout_func_t(void *); > + > struct callout { > union { > LIST_ENTRY(callout) le; >=20 > Modified: head/sys/sys/callout.h > = =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D > --- head/sys/sys/callout.h Mon Sep 14 10:28:47 2015 = (r287779) > +++ head/sys/sys/callout.h Mon Sep 14 10:52:26 2015 = (r287780) > @@ -82,6 +82,7 @@ struct callout_handle { > #define callout_active(c) ((c)->c_flags & CALLOUT_ACTIVE) > #define callout_deactivate(c) ((c)->c_flags &=3D = ~CALLOUT_ACTIVE) > #define callout_drain(c) _callout_stop_safe(c, 1) > +int callout_drain_async(struct callout *, callout_func_t *, void *); > void callout_init(struct callout *, int); > void _callout_init_lock(struct callout *, struct lock_object *, int); > #define callout_init_mtx(c, mtx, flags) = \ >=20 -------- Randall Stewart rrs@netflix.com 803-317-4952