Re: Why is TUNABLE_INT discouraged?
2010/10/18 Dag-Erling Smørgrav d...@des.no:
Garrett Cooper gcoo...@freebsd.org writes:
PS I added uquad_t for consistency in the APIs, even though quad_t was
deprecated, but I didn't bump __FreeBSD_version__ -- wasn't sure if I
should have done that).
You should bump __FreeBSD_version__ anyway so third-party or shared code
can use the new API if it is available and the old one if it isn't.
Let's try with the patch attached...
Mailman strips binary attachments. The correct MIME type is
text/x-patch.
Ok. Let's retry with a .patch extension and by bumping __FreeBSD_version__.
Thanks!
-Garrett
Index: sys/kern/kern_environment.c
===
--- sys/kern/kern_environment.c (revision 214258)
+++ sys/kern/kern_environment.c (working copy)
@@ -50,6 +50,7 @@
#include sys/sysent.h
#include sys/sysproto.h
#include sys/libkern.h
+#include sys/limits.h
#include sys/kenv.h
#include security/mac/mac_framework.h
@@ -442,32 +443,115 @@
}
/*
- * Return a string value from an environment variable.
+ * Return the multiplier for a user specified suffix from a getenv call.
*/
+uintmax_t
+getenv_parse_suffix(const char suffix)
+{
+ uintmax_t multiplier = 1;
+
+ switch (suffix) {
+ case 't': case 'T':
+ multiplier *= 1024;
+ case 'g': case 'G':
+ multiplier *= 1024;
+ case 'm': case 'M':
+ multiplier *= 1024;
+ case 'k': case 'K':
+ multiplier *= 1024;
+ case '\0':
+ break;
+ default:
+ multiplier = 0;
+ }
+
+ return (multiplier);
+}
+
+/*
+ * Return a signed quantity from an environment variable.
+ */
int
-getenv_string(const char *name, char *data, int size)
+getenv_signed(const char *name, intmax_t *data, const intmax_t min,
+const intmax_t max)
{
- char *tmp;
+ uintmax_t multiplier;
+ intmax_t iv;
+ int ret_code;
+ char *value;
+ char *vtp;
- tmp = getenv(name);
- if (tmp != NULL) {
- strlcpy(data, tmp, size);
- freeenv(tmp);
- return (1);
- } else
- return (0);
+ ret_code = 0;
+
+ value = getenv(name);
+ if (value == NULL)
+ return (ret_code);
+ iv = strtoq(value, vtp, 0);
+ if (vtp == value || (vtp[0] != '\0' vtp[1] != '\0') ||
+ (multiplier = getenv_parse_suffix(vtp[0])) == 0) {
+ /*
+ * Catch errors in parsing the number, bad input, or the
+ * suffix if one was specified.
+ */
+ ;
+ } else {
+ iv *= multiplier;
+ if (min = iv iv = max) {
+ *data = iv;
+ ret_code = 1;
+ }
+ }
+ freeenv(value);
+ return (ret_code);
}
/*
+ * Return an unsigned quantity from an environment variable.
+ */
+int
+getenv_unsigned(const char *name, uintmax_t *data, const uintmax_t min,
+const uintmax_t max)
+{
+ uintmax_t multiplier;
+ uintmax_t uiv;
+ int ret_code;
+ char *value;
+ char *vtp;
+
+ ret_code = 0;
+
+ value = getenv(name);
+ if (value == NULL)
+ return (ret_code);
+ uiv = strtouq(value, vtp, 0);
+ if (vtp == value || (vtp[0] != '\0' vtp[1] != '\0') ||
+ (multiplier = getenv_parse_suffix(vtp[0])) == 0) {
+ /*
+ * Catch errors in parsing the number, bad input, or the
+ * suffix if one was specified.
+ */
+ ;
+ } else {
+ uiv *= multiplier;
+ if (min = uiv uiv = max) {
+ *data = uiv;
+ ret_code = 1;
+ }
+ }
+ freeenv(value);
+ return (ret_code);
+}
+
+/*
* Return an integer value from an environment variable.
*/
int
getenv_int(const char *name, int *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_signed(name, tmp, INT_MIN, INT_MAX);
if (rval)
*data = (int) tmp;
return (rval);
@@ -479,10 +563,10 @@
int
getenv_uint(const char *name, unsigned int *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_unsigned(name, tmp, 0, UINT_MAX);
if (rval)
*data = (unsigned int) tmp;
return (rval);
@@ -494,10 +578,10 @@
int
getenv_long(const char *name, long *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_signed(name, tmp, LONG_MIN, LONG_MAX);
if (rval)
*data = (long) tmp;
return (rval);
@@ -509,51 +593,90 @@
int
getenv_ulong(const char *name, unsigned long *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_unsigned(name, tmp, 0, ULONG_MAX);
if (rval)
*data = (unsigned long) tmp;
return (rval);
}
/*
+ * Return a pointer from an environment variable.
+ */
+int
+getenv_pointer(const char *name, uintptr_t *data)
+{
+ intmax_t tmp;
+ int rval;
+
+ rval = getenv_unsigned(name, tmp, 0, UINTPTR_MAX);
+ if (rval)
+ *data = (uintptr_t) tmp;
+ return (rval);
+}
+
+/*
* Return a quad_t value from an environment variable.
*/
int
getenv_quad(const char *name, quad_t *data)
{
- char *value;
- char *vtp;
- quad_t iv;
+ intmax_t tmp;
+ int rval;
- value = getenv(name);
- if (value == NULL)
+ rval = getenv_signed(name, tmp, QUAD_MIN, QUAD_MAX);
+ if (rval)
+ *data = (quad_t) tmp;
+ return (rval);
+}
+
+/*
+ * Return a uquad_t value from an
Re: Why is TUNABLE_INT discouraged?
2010/10/23 Garrett Cooper gcoo...@freebsd.org:
2010/10/18 Dag-Erling Smørgrav d...@des.no:
Garrett Cooper gcoo...@freebsd.org writes:
PS I added uquad_t for consistency in the APIs, even though quad_t was
deprecated, but I didn't bump __FreeBSD_version__ -- wasn't sure if I
should have done that).
You should bump __FreeBSD_version__ anyway so third-party or shared code
can use the new API if it is available and the old one if it isn't.
Let's try with the patch attached...
Mailman strips binary attachments. The correct MIME type is
text/x-patch.
Ok. Let's retry with a .patch extension and by bumping __FreeBSD_version__.
Sorry.. forgot the uquad_t addition to sys/types.h :(...
Index: sys/kern/kern_environment.c
===
--- sys/kern/kern_environment.c (revision 214258)
+++ sys/kern/kern_environment.c (working copy)
@@ -50,6 +50,7 @@
#include sys/sysent.h
#include sys/sysproto.h
#include sys/libkern.h
+#include sys/limits.h
#include sys/kenv.h
#include security/mac/mac_framework.h
@@ -442,32 +443,115 @@
}
/*
- * Return a string value from an environment variable.
+ * Return the multiplier for a user specified suffix from a getenv call.
*/
+uintmax_t
+getenv_parse_suffix(const char suffix)
+{
+ uintmax_t multiplier = 1;
+
+ switch (suffix) {
+ case 't': case 'T':
+ multiplier *= 1024;
+ case 'g': case 'G':
+ multiplier *= 1024;
+ case 'm': case 'M':
+ multiplier *= 1024;
+ case 'k': case 'K':
+ multiplier *= 1024;
+ case '\0':
+ break;
+ default:
+ multiplier = 0;
+ }
+
+ return (multiplier);
+}
+
+/*
+ * Return a signed quantity from an environment variable.
+ */
int
-getenv_string(const char *name, char *data, int size)
+getenv_signed(const char *name, intmax_t *data, const intmax_t min,
+const intmax_t max)
{
- char *tmp;
+ uintmax_t multiplier;
+ intmax_t iv;
+ int ret_code;
+ char *value;
+ char *vtp;
- tmp = getenv(name);
- if (tmp != NULL) {
- strlcpy(data, tmp, size);
- freeenv(tmp);
- return (1);
- } else
- return (0);
+ ret_code = 0;
+
+ value = getenv(name);
+ if (value == NULL)
+ return (ret_code);
+ iv = strtoq(value, vtp, 0);
+ if (vtp == value || (vtp[0] != '\0' vtp[1] != '\0') ||
+ (multiplier = getenv_parse_suffix(vtp[0])) == 0) {
+ /*
+ * Catch errors in parsing the number, bad input, or the
+ * suffix if one was specified.
+ */
+ ;
+ } else {
+ iv *= multiplier;
+ if (min = iv iv = max) {
+ *data = iv;
+ ret_code = 1;
+ }
+ }
+ freeenv(value);
+ return (ret_code);
}
/*
+ * Return an unsigned quantity from an environment variable.
+ */
+int
+getenv_unsigned(const char *name, uintmax_t *data, const uintmax_t min,
+const uintmax_t max)
+{
+ uintmax_t multiplier;
+ uintmax_t uiv;
+ int ret_code;
+ char *value;
+ char *vtp;
+
+ ret_code = 0;
+
+ value = getenv(name);
+ if (value == NULL)
+ return (ret_code);
+ uiv = strtouq(value, vtp, 0);
+ if (vtp == value || (vtp[0] != '\0' vtp[1] != '\0') ||
+ (multiplier = getenv_parse_suffix(vtp[0])) == 0) {
+ /*
+ * Catch errors in parsing the number, bad input, or the
+ * suffix if one was specified.
+ */
+ ;
+ } else {
+ uiv *= multiplier;
+ if (min = uiv uiv = max) {
+ *data = uiv;
+ ret_code = 1;
+ }
+ }
+ freeenv(value);
+ return (ret_code);
+}
+
+/*
* Return an integer value from an environment variable.
*/
int
getenv_int(const char *name, int *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_signed(name, tmp, INT_MIN, INT_MAX);
if (rval)
*data = (int) tmp;
return (rval);
@@ -479,10 +563,10 @@
int
getenv_uint(const char *name, unsigned int *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_unsigned(name, tmp, 0, UINT_MAX);
if (rval)
*data = (unsigned int) tmp;
return (rval);
@@ -494,10 +578,10 @@
int
getenv_long(const char *name, long *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_signed(name, tmp, LONG_MIN, LONG_MAX);
if (rval)
*data = (long) tmp;
return (rval);
@@ -509,51 +593,90 @@
int
getenv_ulong(const char *name, unsigned long *data)
{
- quad_t tmp;
+ intmax_t tmp;
int rval;
- rval = getenv_quad(name, tmp);
+ rval = getenv_unsigned(name, tmp, 0, ULONG_MAX);
if (rval)
*data = (unsigned long) tmp;
return (rval);
}
/*
+ * Return a pointer from an environment variable.
+ */
+int
+getenv_pointer(const char *name, uintptr_t *data)
+{
+ intmax_t tmp;
+ int rval;
+
+ rval = getenv_unsigned(name, tmp, 0, UINTPTR_MAX);
+ if (rval)
+ *data = (uintptr_t) tmp;
+ return (rval);
+}
+
+/*
* Return a quad_t value from an environment variable.
*/
int
getenv_quad(const char *name, quad_t *data)
{
- char *value;
- char *vtp;
- quad_t iv;
+ intmax_t tmp;
+ int rval;
- value = getenv(name);
- if (value == NULL)
+ rval = getenv_signed(name, tmp, QUAD_MIN, QUAD_MAX);
+ if (rval)
+
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper gcoo...@freebsd.org writes: PS I added uquad_t for consistency in the APIs, even though quad_t was deprecated, but I didn't bump __FreeBSD_version__ -- wasn't sure if I should have done that). You should bump __FreeBSD_version__ anyway so third-party or shared code can use the new API if it is available and the old one if it isn't. Let's try with the patch attached... Mailman strips binary attachments. The correct MIME type is text/x-patch. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/12 Dag-Erling Smørgrav d...@des.no:
Garrett Cooper gcoo...@freebsd.org writes:
Dag-Erling Smørgrav d...@des.no writes:
It might be a good idea to introduce TUNABLE_POINTER and TUNABLE_SIZE.
I would actually argue against doing that because it would only create
divergence between sysctl and tunable KPIs...
Not if we also introduce corresponding SYSCTLs. Note that my idea is to
have these as aliases for the correct primitive types.
(BTW, when you say TUNABLE_SIZE, I assume it would be a size_t quantity?)
Yes.
Something might need to be done to the values returned by the tunables
though, because they don't respect boundaries, and can overflow right
now (which exacerbates the issue with values crammed into smaller
datatypes)...
Yes. How about this:
- rename getenv_quad() to getenv_signed() and getenv_unsigned()
- add min / max arguments
- implement getenv_quad() (and all the others) in terms of
getenv_number()
e.g.
int
getenv_uint(const char *name, unsigned int *data)
{
unsigned long long tmp;
int rval;
if ((rval = getenv_unsigned(name, tmp, 0, UINT_MAX)) == 0)
*data = (unsigned int)tmp;
return (rval);
}
(note that due to the min / max arguments, the complexity of handling
both signed and unsigned values in the same function probably exceeds
the complexity of having two very similar functions)
Here's a draft of this work des@ challenged me to a while back. It
works well as demonstrated with my tests. The only catch with
detecting bounds is that if it's the minimum in the case of signed or
maximum representable value in the case of unsigned, then strtoq, etc
will clamp the value to the maximum representable value.
Other than that it works well, and now tunables represented by
unsigned values should work better.
Please let me know what you all think.
Thanks!
-Garrett
PS I added uquad_t for consistency in the APIs, even though quad_t was
deprecated, but I didn't bump __FreeBSD_version__ -- wasn't sure if I
should have done that).
test-tunables-draft1.log
Description: Binary data
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/10/16 Garrett Cooper gcoo...@freebsd.org:
2010/8/12 Dag-Erling Smørgrav d...@des.no:
Garrett Cooper gcoo...@freebsd.org writes:
Dag-Erling Smørgrav d...@des.no writes:
It might be a good idea to introduce TUNABLE_POINTER and TUNABLE_SIZE.
I would actually argue against doing that because it would only create
divergence between sysctl and tunable KPIs...
Not if we also introduce corresponding SYSCTLs. Note that my idea is to
have these as aliases for the correct primitive types.
(BTW, when you say TUNABLE_SIZE, I assume it would be a size_t quantity?)
Yes.
Something might need to be done to the values returned by the tunables
though, because they don't respect boundaries, and can overflow right
now (which exacerbates the issue with values crammed into smaller
datatypes)...
Yes. How about this:
- rename getenv_quad() to getenv_signed() and getenv_unsigned()
- add min / max arguments
- implement getenv_quad() (and all the others) in terms of
getenv_number()
e.g.
int
getenv_uint(const char *name, unsigned int *data)
{
unsigned long long tmp;
int rval;
if ((rval = getenv_unsigned(name, tmp, 0, UINT_MAX)) == 0)
*data = (unsigned int)tmp;
return (rval);
}
(note that due to the min / max arguments, the complexity of handling
both signed and unsigned values in the same function probably exceeds
the complexity of having two very similar functions)
Here's a draft of this work des@ challenged me to a while back. It
works well as demonstrated with my tests. The only catch with
detecting bounds is that if it's the minimum in the case of signed or
maximum representable value in the case of unsigned, then strtoq, etc
will clamp the value to the maximum representable value.
Other than that it works well, and now tunables represented by
unsigned values should work better.
Please let me know what you all think.
Thanks!
-Garrett
PS I added uquad_t for consistency in the APIs, even though quad_t was
deprecated, but I didn't bump __FreeBSD_version__ -- wasn't sure if I
should have done that).
Let's try with the patch attached...
tunables-enhancement.diff
Description: Binary data
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/9 Dag-Erling Smørgrav d...@des.no: Garrett Cooper gcoo...@freebsd.org writes: Why would someone express a tunable in a memory address (not being sarcastic... I just don't see why it makes sense right now, but if there's a valid reason I'm more than happy to be educated :)..)? A few examples: hw.acpi.host_mem_start hw.pci.host_mem_start hw.physmemstart The following are not addresses, but can be 32 bits on 64-bit machines and even on some 32-bit machines using PAE / PTE: hw.physmem vm.kmem_size vm.kmem_size_max vm.kmem_size_min Ok -- good to know. It might be a good idea to introduce TUNABLE_POINTER and TUNABLE_SIZE. I would actually argue against doing that because it would only create divergence between sysctl and tunable KPIs... but that's just me. The patch I provided before would converge sysctl and tunables a bit more (and I'd more than happily submit patches for the other missing cases on the sysctl side to get parity with the tunables). If it makes sense to add the sysctls _and_ the tunables for POINTER and SIZE though, I'll provide a patch for both cases in one shot. (BTW, when you say TUNABLE_SIZE, I assume it would be a size_t quantity?) Something might need to be done to the values returned by the tunables though, because they don't respect boundaries, and can overflow right now (which exacerbates the issue with values crammed into smaller datatypes)... Thanks! -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper gcoo...@freebsd.org writes:
Dag-Erling Smørgrav d...@des.no writes:
It might be a good idea to introduce TUNABLE_POINTER and TUNABLE_SIZE.
I would actually argue against doing that because it would only create
divergence between sysctl and tunable KPIs...
Not if we also introduce corresponding SYSCTLs. Note that my idea is to
have these as aliases for the correct primitive types.
(BTW, when you say TUNABLE_SIZE, I assume it would be a size_t quantity?)
Yes.
Something might need to be done to the values returned by the tunables
though, because they don't respect boundaries, and can overflow right
now (which exacerbates the issue with values crammed into smaller
datatypes)...
Yes. How about this:
- rename getenv_quad() to getenv_signed() and getenv_unsigned()
- add min / max arguments
- implement getenv_quad() (and all the others) in terms of
getenv_number()
e.g.
int
getenv_uint(const char *name, unsigned int *data)
{
unsigned long long tmp;
int rval;
if ((rval = getenv_unsigned(name, tmp, 0, UINT_MAX)) == 0)
*data = (unsigned int)tmp;
return (rval);
}
(note that due to the min / max arguments, the complexity of handling
both signed and unsigned values in the same function probably exceeds
the complexity of having two very similar functions)
DES
--
Dag-Erling Smørgrav - d...@des.no
___
freebsd-hackers@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Ivan Voras ivo...@freebsd.org writes: Dag-Erling Smørgrav d...@des.no writes: Not sure what you mean. The original issue was that someone had used TUNABLE_INT() for something that was actually a memory address. I changed it to TUNABLE_ULONG(). Of course, if your tunable is a boolean value or something like maxprocs, an int is fine - but so is a long. Semantically valid but using TUNABLE_INT to hold pointers is a developer bug, not the fault of the API, and there's nothing wrong with int as a data type in this context. That's the point. There was no TUNABLE_ULONG() at the time. I added it to fix the bug. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper gcoo...@freebsd.org writes: Why would someone express a tunable in a memory address (not being sarcastic... I just don't see why it makes sense right now, but if there's a valid reason I'm more than happy to be educated :)..)? A few examples: hw.acpi.host_mem_start hw.pci.host_mem_start hw.physmemstart The following are not addresses, but can be 32 bits on 64-bit machines and even on some 32-bit machines using PAE / PTE: hw.physmem vm.kmem_size vm.kmem_size_max vm.kmem_size_min It might be a good idea to introduce TUNABLE_POINTER and TUNABLE_SIZE. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/7 Ivan Voras ivo...@freebsd.org: 2010/8/8 Dag-Erling Smørgrav d...@des.no: Garrett Cooper gcoo...@freebsd.org writes: Dag-Erling Smørgrav d...@des.no writes: Perhaps. I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... It does matter for integers on 64-bit vs 32-bit architectures though, right Not sure what you mean. The original issue was that someone had used TUNABLE_INT() for something that was actually a memory address. I changed it to TUNABLE_ULONG(). Of course, if your tunable is a boolean value or something like maxprocs, an int is fine - but so is a long. Why would someone express a tunable in a memory address (not being sarcastic... I just don't see why it makes sense right now, but if there's a valid reason I'm more than happy to be educated :)..)? Semantically valid but using TUNABLE_INT to hold pointers is a developer bug, not the fault of the API, and there's nothing wrong with int as a data type in this context. Unless there is a real hidden danger in using TUNABLE_INT (and/or adding TUNABLE_UINT etc.) in the expected way, I'd vote for either removing the cautioning comment or rewriting it to say something like developers are hereby warned that ints cannot hold pointers on all architectures, if it is indeed such a little known fact among kernel developers :P *grins cheekily in agreement* :). Thanks, -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
On Sun, Aug 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: 2010/8/8 Dag-Erling Sm??rgrav d...@des.no: Garrett Cooper gcoo...@freebsd.org writes: Dag-Erling Sm??rgrav d...@des.no writes: Perhaps. ??I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... It does matter for integers on 64-bit vs 32-bit architectures though, right Not sure what you mean. ??The original issue was that someone had used TUNABLE_INT() for something that was actually a memory address. ??I changed it to TUNABLE_ULONG(). ??Of course, if your tunable is a boolean value or something like maxprocs, an int is fine - but so is a long. Semantically valid but using TUNABLE_INT to hold pointers is a developer bug, not the fault of the API, and there's nothing wrong with int as a data type in this context. I agree with Ivan here. If we can't find an actual reason to universally prefer long I'd like to remove this comment. As a counterpoint to the preference for long I can offer a reason to prefer int: the same variable is often exposed by both a tunable and a sysctls, and a sysctl using long can have 32-bit compat issues. (That is, a 32-bit app using sysctlbyname will try to use 4 bytes for a long.) -Ed ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
In message: 20100808130624.gb40...@sandvine.com Ed Maste ema...@freebsd.org writes: : On Sun, Aug 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: : : 2010/8/8 Dag-Erling Sm??rgrav d...@des.no: : Garrett Cooper gcoo...@freebsd.org writes: : Dag-Erling Sm??rgrav d...@des.no writes: :Perhaps. ??I don't remember all the details; I can't find a discussion in :the list archives (other than me announcing the change in response to a :bug report), but there must have been one, either on IRC or in Karlsruhe. :In any case, I never removed TUNABLE_INT(), so... : It does matter for integers on 64-bit vs 32-bit architectures though, : right : : Not sure what you mean. ??The original issue was that someone had used : TUNABLE_INT() for something that was actually a memory address. ??I : changed it to TUNABLE_ULONG(). ??Of course, if your tunable is a boolean : value or something like maxprocs, an int is fine - but so is a long. : : Semantically valid but using TUNABLE_INT to hold pointers is a : developer bug, not the fault of the API, and there's nothing wrong : with int as a data type in this context. : : I agree with Ivan here. If we can't find an actual reason to : universally prefer long I'd like to remove this comment. : : As a counterpoint to the preference for long I can offer a reason to : prefer int: the same variable is often exposed by both a tunable and a : sysctls, and a sysctl using long can have 32-bit compat issues. (That : is, a 32-bit app using sysctlbyname will try to use 4 bytes for a long.) There's absolutely no reason to not use TUNABLE_INT for simple integers. Back in the deep, dark, dim past, there was no TUNABLE_*LONG. TUNABLE_INT was introduce in r77900 by peter. DES added TUNABLE_LONG in r137099, as well as adding the comment. The comment is bogus, or at least not quite specific enough. It has been routinely ignored since it was added. There's absolutely nothing wrong with TUNABLE_INT. We really should have a TUNABLE_ADDRESS for this case, although there's at least three types of address that we need to worry about: Physical Addresses, Virtual Addresses and Bus Addresses. You don't know if ULONG or LONG is the right type for an address, or if you need a quad (for example, 32-bit MIPS can address, through PTE entries, addresses beyond the 32-bit barrier, so you'd need a QUAD). I'm in favor of changing the comment to: /* * Please do not use for addresses or pointers */ Warner ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
On Sun, Aug 8, 2010 at 11:14 AM, M. Warner Losh i...@bsdimp.com wrote: In message: 20100808130624.gb40...@sandvine.com Ed Maste ema...@freebsd.org writes: : On Sun, Aug 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: : : 2010/8/8 Dag-Erling Sm??rgrav d...@des.no: : Garrett Cooper gcoo...@freebsd.org writes: : Dag-Erling Sm??rgrav d...@des.no writes: :Perhaps. ??I don't remember all the details; I can't find a discussion in :the list archives (other than me announcing the change in response to a :bug report), but there must have been one, either on IRC or in Karlsruhe. :In any case, I never removed TUNABLE_INT(), so... : It does matter for integers on 64-bit vs 32-bit architectures though, : right : : Not sure what you mean. ??The original issue was that someone had used : TUNABLE_INT() for something that was actually a memory address. ??I : changed it to TUNABLE_ULONG(). ??Of course, if your tunable is a boolean : value or something like maxprocs, an int is fine - but so is a long. : : Semantically valid but using TUNABLE_INT to hold pointers is a : developer bug, not the fault of the API, and there's nothing wrong : with int as a data type in this context. : : I agree with Ivan here. If we can't find an actual reason to : universally prefer long I'd like to remove this comment. : : As a counterpoint to the preference for long I can offer a reason to : prefer int: the same variable is often exposed by both a tunable and a : sysctls, and a sysctl using long can have 32-bit compat issues. (That : is, a 32-bit app using sysctlbyname will try to use 4 bytes for a long.) There's absolutely no reason to not use TUNABLE_INT for simple integers. Back in the deep, dark, dim past, there was no TUNABLE_*LONG. TUNABLE_INT was introduce in r77900 by peter. DES added TUNABLE_LONG in r137099, as well as adding the comment. The comment is bogus, or at least not quite specific enough. It has been routinely ignored since it was added. There's absolutely nothing wrong with TUNABLE_INT. We really should have a TUNABLE_ADDRESS for this case, although there's at least three types of address that we need to worry about: Physical Addresses, Virtual Addresses and Bus Addresses. You don't know if ULONG or LONG is the right type for an address, or if you need a quad (for example, 32-bit MIPS can address, through PTE entries, addresses beyond the 32-bit barrier, so you'd need a QUAD). I'm in favor of changing the comment to: /* * Please do not use for addresses or pointers */ Then comes my next request: could someone please review this patch and commit it if it's ok? This adds comments to TUNABLE_INT (recommended by Warner) and also adds functionality for TUNABLE_UINT as well. I have a simple module and test script attached for it. Adding this functionality would make things more consistent with the TUNABLE KPIs, make my experimenting with sound(4) easier, and could help improve the sound(4) subsystem's code (I'll have to discuss some things with ariff@, because I'm not sure whether or not some quantities need to be signed or not). Thanks! -Garrett uint_tunable.diff Description: Binary data ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Why is TUNABLE_INT discouraged?
Hi Hackers, Poking around the sound(4) drivers, I looked into converting one of the TUNABLE_INTs to an unsigned tunable for testing purposes. I looked in kernel.h and I saw the following comment: /* * int * please avoid using for new tunables! */ I found the commit where it was made (by des@ -- cvs revision 1.120), but unfortunately I lack the context as to why that suggestion is made; the commit isn't very explicit as to why integers tunables should be discouraged -- and in the case of hw.sound.feeder_rate_round, it makes just as much sense to use an integer type or a long integer type, as accepted input values are small enough to fit in an integer value with a _lot_ of headroom: hw.snd.feeder_rate_round Sample rate rounding threshold, to avoid large prime division at the cost of accuracy. All requested sample rates will be rounded to the nearest threshold value. Possible values range between 0 (disabled) and 500. Default is 25. It would be nice to know why TUNABLE_INT is discouraged :). Thanks! -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper gcoo...@freebsd.org writes: I found the commit where it was made (by des@ -- cvs revision 1.120), but unfortunately I lack the context as to why that suggestion is made; the commit isn't very explicit as to why integers tunables should be discouraged You're supposed to use TUNABLE_LONG or TUNABLE_ULONG instead. From digging in the -current archives, it seems that the motivation was a bug that resulted from using a TUNABLE_INT for a value that was actually an address. It was doubly broken: first because it was too small on 64-bit systems, and second because it was signed. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/7 Dag-Erling Smørgrav d...@des.no: Garrett Cooper gcoo...@freebsd.org writes: I found the commit where it was made (by des@ -- cvs revision 1.120), but unfortunately I lack the context as to why that suggestion is made; the commit isn't very explicit as to why integers tunables should be discouraged You're supposed to use TUNABLE_LONG or TUNABLE_ULONG instead. From digging in the -current archives, it seems that the motivation was a bug that resulted from using a TUNABLE_INT for a value that was actually an address. It was doubly broken: first because it was too small on 64-bit systems, and second because it was signed. Thanks for the explanation. I just found it interesting how the interfaces to [PCI BUS] resources, sysctls, and tunables are inconsistent in terms of what data types they support; resources only supports signed integers of various widths, sysctls support everything under the sun, and we know the story on tunables now. It's ok most of the time, but as we all know there are limits to the ranges for integers, and there's something a bit quirky about some of the code in sound(4) that I'm experimenting with to see whether or not it was there's an issue with bad casting, comparison, an uninitialized value, or another random race condition, that's wreaking havoc with my Audigy card every time I attach the driver as a module. Thanks! -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
On 7.8.2010 15:40, Dag-Erling Smørgrav wrote: Garrett Cooper gcoo...@freebsd.org writes: I found the commit where it was made (by des@ -- cvs revision 1.120), but unfortunately I lack the context as to why that suggestion is made; the commit isn't very explicit as to why integers tunables should be discouraged You're supposed to use TUNABLE_LONG or TUNABLE_ULONG instead. From digging in the -current archives, it seems that the motivation was a bug that resulted from using a TUNABLE_INT for a value that was actually an address. It was doubly broken: first because it was too small on 64-bit systems, and second because it was signed. Ok, but still - if the underlying value really is declared as int, doesn't it make perfect sense to have something like TUNABLE_INT for it? Forcing long is a bit weird in this context, as C long is 32-bit on i386 and 64-bit on amd64. ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Ivan Voras ivo...@freebsd.org writes: Ok, but still - if the underlying value really is declared as int, doesn't it make perfect sense to have something like TUNABLE_INT for it? Perhaps. I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/7 Dag-Erling Smørgrav d...@des.no: Ivan Voras ivo...@freebsd.org writes: Ok, but still - if the underlying value really is declared as int, doesn't it make perfect sense to have something like TUNABLE_INT for it? Perhaps. I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... It does matter for integers on 64-bit vs 32-bit architectures though, right (feel free to ignore the second i386 value for _limits.h... it was a hack for gcc according to the comment)? $ egrep -nr '#define[[:space:]]+__LONG_MAX' amd64/include/ i386/include/ | grep -v svn amd64/include/_limits.h:63:#define __LONG_MAX 0x7fffL /* max for a long */ i386/include/_limits.h:65:#define __LONG_MAX 0x7fffL i386/include/_limits.h:69:#define __LONG_MAX 0x7fffL /* max value for a long */ $ egrep -nr '#define[[:space:]]+__INT_MAX' amd64/include/ i386/include/ | grep -v svn amd64/include/_limits.h:59:#define __INT_MAX 0x7fff /* max value for an int */ i386/include/_limits.h:59:#define __INT_MAX 0x7fff /* max value for an int */ I was originally asking because I didn't have the background to know why a TUNABLE_UINT set of macros didn't exist. Thanks, -Garrett ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper gcoo...@freebsd.org writes: Dag-Erling Smørgrav d...@des.no writes: Perhaps. I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... It does matter for integers on 64-bit vs 32-bit architectures though, right Not sure what you mean. The original issue was that someone had used TUNABLE_INT() for something that was actually a memory address. I changed it to TUNABLE_ULONG(). Of course, if your tunable is a boolean value or something like maxprocs, an int is fine - but so is a long. DES -- Dag-Erling Smørgrav - d...@des.no ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
Re: Why is TUNABLE_INT discouraged?
2010/8/8 Dag-Erling Smørgrav d...@des.no: Garrett Cooper gcoo...@freebsd.org writes: Dag-Erling Smørgrav d...@des.no writes: Perhaps. I don't remember all the details; I can't find a discussion in the list archives (other than me announcing the change in response to a bug report), but there must have been one, either on IRC or in Karlsruhe. In any case, I never removed TUNABLE_INT(), so... It does matter for integers on 64-bit vs 32-bit architectures though, right Not sure what you mean. The original issue was that someone had used TUNABLE_INT() for something that was actually a memory address. I changed it to TUNABLE_ULONG(). Of course, if your tunable is a boolean value or something like maxprocs, an int is fine - but so is a long. Semantically valid but using TUNABLE_INT to hold pointers is a developer bug, not the fault of the API, and there's nothing wrong with int as a data type in this context. Unless there is a real hidden danger in using TUNABLE_INT (and/or adding TUNABLE_UINT etc.) in the expected way, I'd vote for either removing the cautioning comment or rewriting it to say something like developers are hereby warned that ints cannot hold pointers on all architectures, if it is indeed such a little known fact among kernel developers :P ___ freebsd-hackers@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/freebsd-hackers To unsubscribe, send any mail to freebsd-hackers-unsubscr...@freebsd.org
