Re: Why is TUNABLE_INT discouraged?
2010/10/23 Garrett Cooper : > 2010/10/18 Dag-Erling Smørgrav : >> Garrett Cooper 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 #include #include +#include #include #include @@ -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 env
Re: Why is TUNABLE_INT discouraged?
2010/10/18 Dag-Erling Smørgrav : > Garrett Cooper 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 #include #include +#include #include #include @@ -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 environment variable. + */ +int +getenv_uquad(const char *name, uquad_t *data) +{ + intm
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper 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/10/16 Garrett Cooper : > 2010/8/12 Dag-Erling Smørgrav : >> Garrett Cooper writes: >>> Dag-Erling Smørgrav 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/12 Dag-Erling Smørgrav : > Garrett Cooper writes: >> Dag-Erling Smørgrav 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?
Garrett Cooper writes: > Dag-Erling Smørgrav 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?
2010/8/9 Dag-Erling Smørgrav : > Garrett Cooper 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 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?
Ivan Voras writes: > Dag-Erling Smørgrav 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?
On Sun, Aug 8, 2010 at 11:14 AM, M. Warner Losh wrote: > In message: <20100808130624.gb40...@sandvine.com> > Ed Maste writes: > : On Sun, Aug 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: > : > : > 2010/8/8 Dag-Erling Sm??rgrav : > : > > Garrett Cooper writes: > : > >> Dag-Erling Sm??rgrav 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"
Re: Why is TUNABLE_INT discouraged?
In message: <20100808130624.gb40...@sandvine.com> Ed Maste writes: : On Sun, Aug 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: : : > 2010/8/8 Dag-Erling Sm??rgrav : : > > Garrett Cooper writes: : > >> Dag-Erling Sm??rgrav 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 08, 2010 at 01:30:19AM +0200, Ivan Voras wrote: > 2010/8/8 Dag-Erling Sm??rgrav : > > Garrett Cooper writes: > >> Dag-Erling Sm??rgrav 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?
2010/8/7 Ivan Voras : > 2010/8/8 Dag-Erling Smørgrav : >> Garrett Cooper writes: >>> Dag-Erling Smørgrav 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?
2010/8/8 Dag-Erling Smørgrav : > Garrett Cooper writes: >> Dag-Erling Smørgrav 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"
Re: Why is TUNABLE_INT discouraged?
Garrett Cooper writes: > Dag-Erling Smørgrav 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/7 Dag-Erling Smørgrav : > Ivan Voras 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?
Ivan Voras 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?
On 7.8.2010 15:40, Dag-Erling Smørgrav wrote: > Garrett Cooper 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?
2010/8/7 Dag-Erling Smørgrav : > Garrett Cooper 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?
Garrett Cooper 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"