Re: svn commit: r217830 - head/share/man/man9
On 26 Jan 2011, at 23:41, m...@freebsd.org wrote: Upon further consideration, I don't think sbuf_new_for_sysctl() should be doing the wire. Whether the buffer needs to be wired or not is up to the implementation of the individual sysctl; *most* of them will be holding a lock when doing sbuf_print, but there's no guarantee. It's simpler to just leave this in the hands of the implementor, and it also enables better error reporting. One pondering: normally, it's nice if functions that may sleep unconditionally trigger a WITNESS sleep warning even if they don't actually sleep this time (although conditioned on arguments: if you pass M_WAITOK to malloc, you always get the warning even if malloc doesn't sleep, whereas if you pass M_NOWAIT it doesn't). I'm wondering how we could do something similar here -- the problem is that sysctl copy routines don't currently know if a page is wired or not, and therefore whether they could sleep or not. I wonder, with a witness kernel, how expensive it would be to have witness check for each range it was copying in/out of, whether the page was wired (by asking VM presumably)... There might also be lock order issues with that query. One way to handle this would be to have the sbuf sysctl setup pass in a wired flag, or some other indication of wiredness, and then that could be saved with the sbuf -- when sbuf_printf and friends are called, and a non-wired sysctl has been set up, then the witness warning fires. Robert___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On Tue, 25 Jan 2011, Matthew D Fleming wrote: .Dv SBUF_AUTOEXTEND . .Pp The +.Fn sbuf_new_for_sysctl +function will set up an sbuf with a drain function to use +.Fn SYSCTL_OUT +when the internal buffer fills. +The sysctl old buffer will be wired, which allows for doing an +.Fn sbuf_printf +while holding a mutex. +.Pp +The .Fn sbuf_delete function clears the .Fa sbuf Hmm. Is this description missing mention of how wiring failures are handled? (Also, it should probably mention that this call can sleep for potentially quite long periods of time, even if sbuf_printf (and friends) can't). Robert ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On Wed, Jan 26, 2011 at 1:37 AM, Robert Watson rwat...@freebsd.org wrote: On Tue, 25 Jan 2011, Matthew D Fleming wrote: .Dv SBUF_AUTOEXTEND . .Pp The +.Fn sbuf_new_for_sysctl +function will set up an sbuf with a drain function to use +.Fn SYSCTL_OUT +when the internal buffer fills. +The sysctl old buffer will be wired, which allows for doing an +.Fn sbuf_printf +while holding a mutex. +.Pp +The .Fn sbuf_delete function clears the .Fa sbuf Hmm. Is this description missing mention of how wiring failures are handled? (Also, it should probably mention that this call can sleep for potentially quite long periods of time, even if sbuf_printf (and friends) can't). I'm not sure how much to write, since some of the wiring failures are dealt with by the sysctl subsystem and are not documented. The current state of the actual code is that a failure in vslock(9) is ignored, unless it's ENOMEM in which case sysctl_wire_old_buffer sets the sysctl_req-validlen to 0, which would behave perhaps slightly unexpectedly for the user since no data will be copied out. Any non-ENOMEM failure from vslock() presumably would also have been a failure from SYSCTL_OUT and this does get squashed, perhaps incorrectly. I'll think about saving the error code so that sbuf_finish can report it if nothing else has gone wrong. Thanks, matthew ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On 26 Jan 2011, at 17:12, m...@freebsd.org wrote: Hmm. Is this description missing mention of how wiring failures are handled? (Also, it should probably mention that this call can sleep for potentially quite long periods of time, even if sbuf_printf (and friends) can't). I'm not sure how much to write, since some of the wiring failures are dealt with by the sysctl subsystem and are not documented. The current state of the actual code is that a failure in vslock(9) is ignored, unless it's ENOMEM in which case sysctl_wire_old_buffer sets the sysctl_req-validlen to 0, which would behave perhaps slightly unexpectedly for the user since no data will be copied out. Any non-ENOMEM failure from vslock() presumably would also have been a failure from SYSCTL_OUT and this does get squashed, perhaps incorrectly. I'll think about saving the error code so that sbuf_finish can report it if nothing else has gone wrong. Yeah, no specific opinions on the right answer, except perhaps that sbuf_new_for_sysctl() failing due to ENOMEM is something worth making it easy to report to the user. I suppose an important question is now often we see this actually failing, and in what circumstances: if there's a moderate chance of it failing on low-memory machines under memory pressure, it suggests we've gone wrong somewhere... One nice thing about the non-wiring model is that it's pretty cheap in the common case, whereas the new code is pretty expensive in the common case. Maybe this doesn't matter too much. Robert___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On Wed, Jan 26, 2011 at 9:55 AM, Robert N. M. Watson rwat...@freebsd.org wrote: On 26 Jan 2011, at 17:12, m...@freebsd.org wrote: Hmm. Is this description missing mention of how wiring failures are handled? (Also, it should probably mention that this call can sleep for potentially quite long periods of time, even if sbuf_printf (and friends) can't). I'm not sure how much to write, since some of the wiring failures are dealt with by the sysctl subsystem and are not documented. The current state of the actual code is that a failure in vslock(9) is ignored, unless it's ENOMEM in which case sysctl_wire_old_buffer sets the sysctl_req-validlen to 0, which would behave perhaps slightly unexpectedly for the user since no data will be copied out. Any non-ENOMEM failure from vslock() presumably would also have been a failure from SYSCTL_OUT and this does get squashed, perhaps incorrectly. I'll think about saving the error code so that sbuf_finish can report it if nothing else has gone wrong. Yeah, no specific opinions on the right answer, except perhaps that sbuf_new_for_sysctl() failing due to ENOMEM is something worth making it easy to report to the user. The ENOMEM is already managed and squashed inside sysctl_wire_old_buffer(), so there's no way for sbuf_new_for_sysctl() to report it. It may end up happening automagically since it sets the validlen to 0. I suppose an important question is now often we see this actually failing I don't believe we've ever seen a memory failure relating to sysctls at Isilon and we've been using the equivalent of this code for a few years. Our machines aren't low memory but they are under memory pressure sometimes. Thanks, matthew ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On 26 Jan 2011, at 18:29, m...@freebsd.org wrote: I suppose an important question is now often we see this actually failing I don't believe we've ever seen a memory failure relating to sysctls at Isilon and we've been using the equivalent of this code for a few years. Our machines aren't low memory but they are under memory pressure sometimes. The kinds of cases I worry about are things like the tcp connection monitoring sysctls. Most systems have a dozen, hundred, or a thousand connections. Some have half a million or a million. If we switched to requiring wiring every page needed to store that list, it would do terrible things to the system. So really what I have in mind is: either we handle cases like that well, or we put in a clear warning and have obvious failure modes to catch the cases where it didn't work out. In practice, I think we would not want to switch the tcpcb/inpcb sysctl for this reason, but as people say ah, this is convenient we need to make sure it's handled well, and easy to debug problems when they do arise. Robert___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On Wed, Jan 26, 2011 at 1:10 PM, Robert N. M. Watson rwat...@freebsd.org wrote: On 26 Jan 2011, at 18:29, m...@freebsd.org wrote: I suppose an important question is now often we see this actually failing I don't believe we've ever seen a memory failure relating to sysctls at Isilon and we've been using the equivalent of this code for a few years. Our machines aren't low memory but they are under memory pressure sometimes. The kinds of cases I worry about are things like the tcp connection monitoring sysctls. Most systems have a dozen, hundred, or a thousand connections. Some have half a million or a million. If we switched to requiring wiring every page needed to store that list, it would do terrible things to the system. So really what I have in mind is: either we handle cases like that well, or we put in a clear warning and have obvious failure modes to catch the cases where it didn't work out. In practice, I think we would not want to switch the tcpcb/inpcb sysctl for this reason, but as people say ah, this is convenient we need to make sure it's handled well, and easy to debug problems when they do arise. But I think that problem exists today using sysctl for output, since it's non-iterative. In fact, it's often worse today, because in addition to the user-space buffer that needs to be large enough to hold the output, the kernel needs to malloc(9) a buffer to hold it before doing the one SYSCTL_OUT at the end that most routines I've seen use. For situations like this where there is a lot of output but it doesn't need to be serialized by a lock held across the whole data fetch, then yes, using sbuf_new_for_sysctl() would wire more memory. Thanks, matthew ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On 26 Jan 2011, at 21:14, m...@freebsd.org wrote: The kinds of cases I worry about are things like the tcp connection monitoring sysctls. Most systems have a dozen, hundred, or a thousand connections. Some have half a million or a million. If we switched to requiring wiring every page needed to store that list, it would do terrible things to the system. So really what I have in mind is: either we handle cases like that well, or we put in a clear warning and have obvious failure modes to catch the cases where it didn't work out. In practice, I think we would not want to switch the tcpcb/inpcb sysctl for this reason, but as people say ah, this is convenient we need to make sure it's handled well, and easy to debug problems when they do arise. But I think that problem exists today using sysctl for output, since it's non-iterative. In fact, it's often worse today, because in addition to the user-space buffer that needs to be large enough to hold the output, the kernel needs to malloc(9) a buffer to hold it before doing the one SYSCTL_OUT at the end that most routines I've seen use. For situations like this where there is a lot of output but it doesn't need to be serialized by a lock held across the whole data fetch, then yes, using sbuf_new_for_sysctl() would wire more memory. Right -- hence my concern about (a) appropriate documentation and (b) proper error handling. The sbuf routine looks convenient, easy to use, and exactly the semantic that I want in most cases. However, sometimes, it may silently break based on something rather abstract getting too big. We need users of the KPI to be aware of that limitation and hence not use it when that could occur, and when it does occur, generate a clear notice of some sort so that it can be tracked down. Robert___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r217830 - head/share/man/man9
On Wed, Jan 26, 2011 at 1:19 PM, Robert N. M. Watson rwat...@freebsd.org wrote: On 26 Jan 2011, at 21:14, m...@freebsd.org wrote: The kinds of cases I worry about are things like the tcp connection monitoring sysctls. Most systems have a dozen, hundred, or a thousand connections. Some have half a million or a million. If we switched to requiring wiring every page needed to store that list, it would do terrible things to the system. So really what I have in mind is: either we handle cases like that well, or we put in a clear warning and have obvious failure modes to catch the cases where it didn't work out. In practice, I think we would not want to switch the tcpcb/inpcb sysctl for this reason, but as people say ah, this is convenient we need to make sure it's handled well, and easy to debug problems when they do arise. But I think that problem exists today using sysctl for output, since it's non-iterative. In fact, it's often worse today, because in addition to the user-space buffer that needs to be large enough to hold the output, the kernel needs to malloc(9) a buffer to hold it before doing the one SYSCTL_OUT at the end that most routines I've seen use. For situations like this where there is a lot of output but it doesn't need to be serialized by a lock held across the whole data fetch, then yes, using sbuf_new_for_sysctl() would wire more memory. Right -- hence my concern about (a) appropriate documentation and (b) proper error handling. The sbuf routine looks convenient, easy to use, and exactly the semantic that I want in most cases. However, sometimes, it may silently break based on something rather abstract getting too big. We need users of the KPI to be aware of that limitation and hence not use it when that could occur, and when it does occur, generate a clear notice of some sort so that it can be tracked down. Upon further consideration, I don't think sbuf_new_for_sysctl() should be doing the wire. Whether the buffer needs to be wired or not is up to the implementation of the individual sysctl; *most* of them will be holding a lock when doing sbuf_print, but there's no guarantee. It's simpler to just leave this in the hands of the implementor, and it also enables better error reporting. Thanks, matthew ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r217830 - head/share/man/man9
Author: mdf Date: Tue Jan 25 17:39:52 2011 New Revision: 217830 URL: http://svn.freebsd.org/changeset/base/217830 Log: Document sbuf_new_for_sysctl(9). Pointed out by: lstewart Modified: head/share/man/man9/Makefile head/share/man/man9/sbuf.9 Modified: head/share/man/man9/Makefile == --- head/share/man/man9/MakefileTue Jan 25 17:15:23 2011 (r217829) +++ head/share/man/man9/MakefileTue Jan 25 17:39:52 2011 (r217830) @@ -1031,6 +1031,7 @@ MLINKS+=sbuf.9 sbuf_bcat.9 \ sbuf.9 sbuf_finish.9 \ sbuf.9 sbuf_len.9 \ sbuf.9 sbuf_new.9 \ + sbuf.9 sbuf_new_for_sysctl.9 \ sbuf.9 sbuf_printf.9 \ sbuf.9 sbuf_putc.9 \ sbuf.9 sbuf_set_drain.9 \ Modified: head/share/man/man9/sbuf.9 == --- head/share/man/man9/sbuf.9 Tue Jan 25 17:15:23 2011(r217829) +++ head/share/man/man9/sbuf.9 Tue Jan 25 17:39:52 2011(r217830) @@ -25,13 +25,14 @@ .\ .\ $FreeBSD$ .\ -.Dd May 17, 2009 +.Dd January 25, 2011 .Dt SBUF 9 .Os .Sh NAME .Nm sbuf , .Nm sbuf_new , .Nm sbuf_new_auto , +.Nm sbuf_new_for_sysctl , .Nm sbuf_clear , .Nm sbuf_setpos , .Nm sbuf_bcat , @@ -99,6 +100,9 @@ .Fn sbuf_done struct sbuf *s .Ft void .Fn sbuf_delete struct sbuf *s +.In sys/sysctl.h +.Ft struct sbuf * +.Fn sbuf_new_for_sysctl struct sbuf *s char *buf int length struct sysctl_req *req .Sh DESCRIPTION The .Nm @@ -169,6 +173,15 @@ and .Dv SBUF_AUTOEXTEND . .Pp The +.Fn sbuf_new_for_sysctl +function will set up an sbuf with a drain function to use +.Fn SYSCTL_OUT +when the internal buffer fills. +The sysctl old buffer will be wired, which allows for doing an +.Fn sbuf_printf +while holding a mutex. +.Pp +The .Fn sbuf_delete function clears the .Fa sbuf ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
