Re: svn commit: r220791 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys
On Mon, Apr 18, 2011 at 12:28 PM, Kostik Belousov wrote:
> On Mon, Apr 18, 2011 at 04:32:22PM +, Matthew D Fleming wrote:
>> Author: mdf
>> Date: Mon Apr 18 16:32:22 2011
>> New Revision: 220791
>> URL: http://svn.freebsd.org/changeset/base/220791
>>
>> Log:
>> Add the posix_fallocate(2) syscall. The default implementation in
>> vop_stdallocate() is filesystem agnostic and will run as slow as a
>> read/write loop in userspace; however, it serves to correctly
>> implement the functionality for filesystems that do not implement a
>> VOP_ALLOCATE.
>>
>> Note that __FreeBSD_version was already bumped today to 900036 for any
>> ports which would like to use this function.
>>
>> Also reserve space in the syscall table for posix_fadvise(2).
>> +#ifdef __notyet__
>> + /*
>> + * Check if the filesystem sets f_maxfilesize; if not use
>> + * VOP_SETATTR to perform the check.
>> + */
>> + error = VFS_STATFS(vp->v_mount, &sfs, td);
>> + if (error != 0)
>> + goto out;
>> + if (sfs.f_maxfilesize) {
>> + if (offset > sfs.f_maxfilesize || len > sfs.f_maxfilesize ||
>> + offset + len > sfs.f_maxfilesize) {
>> + error = EFBIG;
>> + goto out;
>> + }
>> + } else
>> +#endif
>> + if (offset + len > vap->va_size) {
>> + VATTR_NULL(vap);
>> + vap->va_size = offset + len;
>> + error = VOP_SETATTR(vp, vap, td->td_ucred);
>> + if (error != 0)
>> + goto out;
>> + }
> I still do not see a reason to do VOP_SETATTR() there. VOP_WRITE() will
> do auto-extend as needed. Also, see below.
The need is, as commented, to return EFBIG when the new file size will
be larger than the FS supports. Without this code, passing in
something like posix_fallocate(fd, 0, OFF_MAX) will run the filesystem
out of space.
>> +
>> + while (len > 0) {
>> + if (should_yield()) {
>> + VOP_UNLOCK(vp, 0);
>> + locked = 0;
>> + kern_yield(-1);
> Please note that, despite dropping the vnode lock, the snapshot creation
> is still blocked at this point, due to previous vn_start_write().
>
> If doing vn_finished_write() there, then bwillwrite() before
> next iteration is desired.
>> + error = vn_lock(vp, LK_EXCLUSIVE);
>> + if (error != 0)
>> + break;
>> + locked = 1;
>> + error = VOP_GETATTR(vp, vap, td->td_ucred);
>> + if (error != 0)
>> + break;
>> + }
>> +
>> + /*
>> + * Read and write back anything below the nominal file
>> + * size. There's currently no way outside the filesystem
>> + * to know whether this area is sparse or not.
>> + */
>> + cur = iosize;
>> + if ((offset % iosize) != 0)
>> + cur -= (offset % iosize);
>> + if (cur > len)
>> + cur = len;
>> + if (offset < vap->va_size) {
>> + aiov.iov_base = buf;
>> + aiov.iov_len = cur;
>> + auio.uio_iov = &aiov;
>> + auio.uio_iovcnt = 1;
>> + auio.uio_offset = offset;
>> + auio.uio_resid = cur;
>> + auio.uio_segflg = UIO_SYSSPACE;
>> + auio.uio_rw = UIO_READ;
>> + auio.uio_td = td;
>> + error = VOP_READ(vp, &auio, 0, td->td_ucred);
>> + if (error != 0)
>> + break;
>> + if (auio.uio_resid > 0) {
>> + bzero(buf + cur - auio.uio_resid,
>> + auio.uio_resid);
>> + }
>> + } else {
>> + bzero(buf, cur);
>> + }
> Wouldn't VOP_SETATTR() at the start of the function mostly prevent
> this bzero from executing ?
Yes. If struct statfs had a member indicating the file system's max
file size, then the extend wouldn't be necessary. We have that
feature locally, but it's only implemented for ufs and our custom file
system,
Re: svn commit: r220791 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys
On Mon, Apr 18, 2011 at 12:47 PM, Pawel Jakub Dawidek wrote:
> On Mon, Apr 18, 2011 at 10:28:10PM +0300, Kostik Belousov wrote:
>> > + if (offset + len > vap->va_size) {
>> > + VATTR_NULL(vap);
>> > + vap->va_size = offset + len;
>> > + error = VOP_SETATTR(vp, vap, td->td_ucred);
>> > + if (error != 0)
>> > + goto out;
>> > + }
>> I still do not see a reason to do VOP_SETATTR() there. VOP_WRITE() will
>> do auto-extend as needed. Also, see below.
>
> Yeah, also when we extend file size we could skip reading zeros.
>
>> > + if (offset < vap->va_size) {
> [...]
>> > + } else {
>> > + bzero(buf, cur);
>> > + }
>> Wouldn't VOP_SETATTR() at the start of the function mostly prevent
>> this bzero from executing ?
>
> Once we drop the vnode lock, the file size can change under us, no?
Yes, which is why the VOP_GETATTR is re-run. The SETATTR doesn't need
to be re-run, since the purpose was just to check that offset + len is
less than the filesystem's maximum supported file size.
>> I estimated what it would take to do the optimized implementation for UFS,
>> and I think that the following change would allow to lessen the code
>> duplication much.
>>
>> What if the vnode lock drop and looping be handled by the syscall, instead
>> of the vop implementation ? In other words, allow the VOP_ALLOCATE()
>> to allocate less then requested, and return the allocated amount to
>> the caller. The loop would be centralized then, freeing fs from doing
>> the dance. Also, if fs considers that suitable, it would do a whole
>> allocation in one run.
>
> I'd still go with SEEK_DATA/SEEK_HOLE loop as I suggested on arch@.
> If you would like to spend time on it, having SEEK_DATA/SEEK_HOLE
> support in UFS would be beneficial for other purposes too.
Well, if we had this functionality it could be used. I'd like the
framework but the filesystems need modification to support it. We
want it for OneFS at Isilon so eventually when I get to this part of
the project I'll have some wrapper code if someone doesn't get there
first.
Cheers,
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: r220791 - in head: lib/libc/sys sys/compat/freebsd32 sys/kern sys/sys
2011/4/18 Kostik Belousov : > On Mon, Apr 18, 2011 at 12:49:38PM -0700, m...@freebsd.org wrote: >> On Mon, Apr 18, 2011 at 12:28 PM, Kostik Belousov >> wrote: >> > On Mon, Apr 18, 2011 at 04:32:22PM +, Matthew D Fleming wrote: >> >> Author: mdf >> >> Date: Mon Apr 18 16:32:22 2011 >> >> New Revision: 220791 >> >> URL: http://svn.freebsd.org/changeset/base/220791 >> >> >> >> Log: >> >> Add the posix_fallocate(2) syscall. The default implementation in >> >> vop_stdallocate() is filesystem agnostic and will run as slow as a >> >> read/write loop in userspace; however, it serves to correctly >> >> implement the functionality for filesystems that do not implement a >> >> VOP_ALLOCATE. >> >> >> >> Note that __FreeBSD_version was already bumped today to 900036 for any >> >> ports which would like to use this function. >> >> >> >> Also reserve space in the syscall table for posix_fadvise(2). >> The need is, as commented, to return EFBIG when the new file size will >> be larger than the FS supports. Without this code, passing in >> something like posix_fallocate(fd, 0, OFF_MAX) will run the filesystem >> out of space. > Handling max file size and not overflowing the fs are different things. > VOP_WRITE() will handle file size on its own too. I see no problem with > exhausting free space if this is what user asked for. This violates the standard, though, since it would return ENOSPC instead of EFBIG. Also, this is just the default implementation. I'm adding a second VOP_SETATTR in vop_stdallocate() to restore the file size after extending, which will let the implementation use bzero/VOP_WRITE instead of VOP_READ/VOP_WRITE when past the original EOF. This is more of an issue when calling the vop iteratively, since subsequent calls don't know what the "correct" file size is. Cheers, 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: r220755 - in head: . contrib/gcc/doc contrib/gcc/objc contrib/libobjc etc/mtree gnu/lib gnu/lib/libobjc gnu/usr.bin/cc gnu/usr.bin/cc/cc1obj gnu/usr.bin/cc/cc_tools gnu/usr.bin/cc/doc
Trimming since I have a mostly-unrelated question... On Tue, Apr 19, 2011 at 5:40 AM, John Baldwin wrote: > On Monday, April 18, 2011 3:59:45 pm Warner Losh wrote: >> In this case, there was a new kernel thing just after, so it turned out OK. >> But let's not gratuitously bump the version since the granularity we have >> already allows the ports to make good choices on when to leave something in >> or >> out. > > Except that that directly contradicts our previously established policy that > these version bumps are cheap and that we should do more of them (this came up > a few years ago when we changed the policy so that the new "stable" branch > after a release starts at N + 500 (e.g. 802500) rather than N + 100 to give > more room for version bumps on current). I thought I remembered reading (within the past 2 years) that __FreeBSD_version should not be incremented more than once a day, since there was a limit of 100 before the version minor number was affected. Did I get the polarity backwards and that was the old policy? 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: r209119 - head/sys/sys
On Sun, Jun 13, 2010 at 10:10 AM, Pawel Jakub Dawidek wrote:
> On Sun, Jun 13, 2010 at 02:39:55AM +, Lawrence Stewart wrote:
>> Author: lstewart
>> Date: Sun Jun 13 02:39:55 2010
>> New Revision: 209119
>> URL: http://svn.freebsd.org/changeset/base/209119
>>
>> Log:
>> Add a utility macro to simplify calculating an aggregate sum from a DPCPU
>> counter variable.
>>
>> Sponsored by: FreeBSD Foundation
>> Reviewed by: jhb, rpaulo, rwatson (previous version of patch)
>> MFC after: 1 week
>>
>> Modified:
>> head/sys/sys/pcpu.h
>>
>> Modified: head/sys/sys/pcpu.h
>> ==
>> --- head/sys/sys/pcpu.h Sun Jun 13 01:27:29 2010 (r209118)
>> +++ head/sys/sys/pcpu.h Sun Jun 13 02:39:55 2010 (r209119)
>> @@ -106,6 +106,17 @@ extern uintptr_t dpcpu_off[];
>> #define DPCPU_ID_GET(i, n) (*DPCPU_ID_PTR(i, n))
>> #define DPCPU_ID_SET(i, n, v) (*DPCPU_ID_PTR(i, n) = v)
>>
>> +/*
>> + * Utility macros.
>> + */
>> +#define DPCPU_SUM(n, var, sum)
>> \
>> +do { \
>> + (sum) = 0; \
>> + u_int i; \
>> + CPU_FOREACH(i) \
>> + (sum) += (DPCPU_ID_PTR(i, n))->var; \
>> +} while (0)
>
> I'd suggest first swapping variable declaration and '(sum) = 0;'.
> Also using 'i' as a counter in macro can easly lead to name collision.
> If you need to do it, I'd suggest '_i' or something.
> Maybe it would be better to make it an inline function rather than macro?
(Relevant but almost a thread hijack):
At Isilon we've run into a lot of problems with variable declarations
in macros, especially with -Wshadow turned on. We ended up
backporting __COUNTER__ from later versions of gcc and then using it
to make unique variable names.
- is the backport (or a fresh implementation) something that could be
done within the scope of the GPL license?
- is it something FreeBSD would be interested in?
- is __COUNTER__ supported by clang?
- if not, could it be?
-Wshadow found several nasty bugs in our code, and apart from a few
spurious warnings it has been handy to have when building our
filesystem.
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: r209119 - head/sys/sys
> BTW, one reason I liked BSD code more than gnu code is that it didn't > use so many macros. Macros should only exist when they are not just > syntactic sugar, like DPCPU_SUM() and unlike CPU_FOREACH(). As a style question, I do understand (generally) why too many macros make the code confusing. However, the *FOREACH macros all fit the same pattern and having a macro to iterate protects one against changes in the implementation -- there's a single location to change if e.g. we want to make CPU_FOREACH use a bitwise operator to determine the next non-zero bit, rather than testing each individually. So what I'm asking is, how do we balance the simplicity of the code (no "unnecessary" macros) versus the simplicity of making certain kinds of infrastructure changes simpler? 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: r209578 - head/sys/sys
2010/6/29 Kostik Belousov :
> On Mon, Jun 28, 2010 at 02:07:02PM -0700, Matthew Fleming wrote:
>> On Mon, Jun 28, 2010 at 10:59 AM, Konstantin Belousov
>> wrote:
>> > Author: kib
>> > Date: Mon Jun 28 17:59:45 2010
>> > New Revision: 209578
>> > URL: http://svn.freebsd.org/changeset/base/209578
>> >
>> > Log:
>> > Use C99 initializers for the struct sysent generated by MAKE_SYSENT().
>> >
>> > MFC after: 1 week
>> >
>> > Modified:
>> > head/sys/sys/sysent.h
>> >
>> > Modified: head/sys/sys/sysent.h
>> > ==
>> > --- head/sys/sys/sysent.h Mon Jun 28 17:45:00 2010 (r209577)
>> > +++ head/sys/sys/sysent.h Mon Jun 28 17:59:45 2010 (r209578)
>> > @@ -144,10 +144,10 @@ struct syscall_module_data {
>> >
>> > #define MAKE_SYSENT(syscallname) \
>> > static struct sysent syscallname##_sysent = { \
>> > - (sizeof(struct syscallname ## _args ) \
>> > + .sy_narg = (sizeof(struct syscallname ## _args ) \
>> > / sizeof(register_t)), \
>> > - (sy_call_t *)& syscallname, \
>> > - SYS_AUE_##syscallname \
>> > + .sy_call = (sy_call_t *)& syscallname, \
>> > + .sy_auevent = SYS_AUE_##syscallname, \
>> > }
>> >
>> > #define SYSCALL_MODULE(name, offset, new_sysent, evh, arg) \
>> >
>>
>> This change prevents (I assume) the use of MAKE_SYSENT() in a C++
>> kernel module, as C++ does not support the .name = value style of
>> named initializers.
>>
>> gcc does allow name: value initializers and it's easy to patch it to
>> accept .name = value, but it's not strictly conforming C++ code
>> anymore.
> I do not mind reverting this, I think it would be better then
> having #ifdef __cplusplus and two definitions. I really wanted to
> have a way to provide sparce initializator for the struct sysent.
> I managed to not require it for r209579.
I agree it's really handy to have sparse initializers; I just haven't
thought of a way to do that and continue to allow 3rd party c++
modules. Perhaps we'll get a new c++ standard soon that supports this
syntax. :-)
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: r227873 - head/usr.bin/procstat
On Tue, Nov 22, 2011 at 11:34 PM, Mikolaj Golub wrote:
> Author: trociny
> Date: Wed Nov 23 07:34:09 2011
> New Revision: 227873
> URL: http://svn.freebsd.org/changeset/base/227873
>
> Log:
> Fix build, hopefully.
>
> Reviewed by: kib
>
> Modified:
> head/usr.bin/procstat/procstat_auxv.c
>
> Modified: head/usr.bin/procstat/procstat_auxv.c
> ==
> --- head/usr.bin/procstat/procstat_auxv.c Wed Nov 23 07:12:26 2011
> (r227872)
> +++ head/usr.bin/procstat/procstat_auxv.c Wed Nov 23 07:34:09 2011
> (r227873)
> @@ -42,14 +42,13 @@
>
> #include "procstat.h"
>
> -static char auxv[sizeof(Elf_Auxinfo) * 256];
> +static Elf_Auxinfo auxv[256];
>
> void
> procstat_auxv(struct kinfo_proc *kipp)
> {
> - Elf_Auxinfo *aux;
> - int i, error, name[4];
> - size_t len;
> + int error, name[4];
> + size_t len, i;
>
> if (!hflag)
> printf("%5s %-16s %-53s\n", "PID", "COMM", "AUXV");
> @@ -58,7 +57,7 @@ procstat_auxv(struct kinfo_proc *kipp)
> name[1] = KERN_PROC;
> name[2] = KERN_PROC_AUXV;
> name[3] = kipp->ki_pid;
> - len = sizeof(auxv);
> + len = sizeof(auxv) * sizeof(*auxv);
> error = sysctl(name, 4, auxv, &len, NULL, 0);
> if (error < 0 && errno != ESRCH) {
> warn("sysctl: kern.proc.auxv: %d: %d", kipp->ki_pid, errno);
> @@ -72,106 +71,116 @@ procstat_auxv(struct kinfo_proc *kipp)
> printf(" -\n");
> return;
> }
> - for (aux = (Elf_Auxinfo *)auxv, i = 0; i < 256; i++, aux++) {
> - switch(aux->a_type) {
> + for (i = 0; i < len; i++) {
> + switch(auxv[i].a_type) {
> case AT_NULL:
> - printf(" (%d)\n", i + 1);
> + printf(" (%zu)\n", i + 1);
> return;
> case AT_IGNORE:
> printf(" AT_IGNORE=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
I didn't see this before, but this gives very misleading output. The
0x prefix implies the output will be hex, but it's printed as decimal,
here and below.
I don't know if there's a style preference for 0x%lx versus %#lx,
though, or a preference for a different type for the print (uintmax_t,
for example). There is probably a preference for using u_long rather
than unsigned long, since it's shorter.
Thanks,
matthew
> break;
> case AT_EXECFD:
> printf(" AT_EXECFD=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_PHDR:
> printf(" AT_PHDR=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_PHENT:
> printf(" AT_PHENT=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_PHNUM:
> printf(" AT_PHNUM=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_PAGESZ:
> printf(" AT_PAGESZ=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_BASE:
> printf(" AT_BASE=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_FLAGS:
> printf(" AT_FLAGS=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> case AT_ENTRY:
> printf(" AT_ENTRY=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> +#ifdef AT_NOTELF
> case AT_NOTELF:
> printf(" AT_NOTELF=0x%lu",
> - (unsigned long)aux->a_un.a_val);
> + (unsigned long)auxv[i].a_un.a_val);
> break;
> +#endif
> +#ifdef AT_UID
> case AT_UID:
> printf(" AT_UID=0x%lu",
> - (unsigned long)aux->a_un.a_val);
>
Re: svn commit: r228625 - head/usr.bin/csup
On Sat, Dec 17, 2011 at 5:14 AM, Dimitry Andric wrote: > Author: dim > Date: Sat Dec 17 13:14:44 2011 > New Revision: 228625 > URL: http://svn.freebsd.org/changeset/base/228625 > > Log: > In usr.bin/csup/auth.c, use the correct number of bytes for zeroing the > shared secret, and use long long format to snprintf a time_t. If casting is necessary, style prefers intmax_t or uintmax_t, since those are always wide enough. Thanks, matthew > MFC after: 1 week > > Modified: > head/usr.bin/csup/auth.c > > Modified: head/usr.bin/csup/auth.c > == > --- head/usr.bin/csup/auth.c Sat Dec 17 12:52:58 2011 (r228624) > +++ head/usr.bin/csup/auth.c Sat Dec 17 13:14:44 2011 (r228625) > @@ -254,7 +254,7 @@ auth_makesecret(struct srvrecord *auth, > MD5_Update(&md5, ":", 1); > MD5_Update(&md5, auth->password, strlen(auth->password)); > MD5_Final(md5sum, &md5); > - memset(secret, 0, sizeof(secret)); > + memset(secret, 0, MD5_CHARS_MAX); > strcpy(secret, md5salt); > auth_readablesum(md5sum, secret + strlen(md5salt)); > } > @@ -302,8 +302,9 @@ auth_makechallenge(struct config *config > } > gettimeofday(&tv, NULL); > MD5_Init(&md5); > - snprintf(buf, sizeof(buf), "%s:%ld:%ld:%ld:%d:%d", > - inet_ntoa(laddr.sin_addr), tv.tv_sec, tv.tv_usec, random(), pid, > ppid); > + snprintf(buf, sizeof(buf), "%s:%lld:%ld:%ld:%d:%d", > + inet_ntoa(laddr.sin_addr), (long long)tv.tv_sec, tv.tv_usec, > + random(), pid, ppid); > MD5_Update(&md5, buf, strlen(buf)); > MD5_Final(md5sum, &md5); > auth_readablesum(md5sum, challenge); ___ 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: r228625 - head/usr.bin/csup
On Sat, Dec 17, 2011 at 1:54 PM, Dimitry Andric wrote: > On 2011-12-17 22:32, m...@freebsd.org wrote: > ... >>> In usr.bin/csup/auth.c, use the correct number of bytes for zeroing the >>> shared secret, and use long long format to snprintf a time_t. >> If casting is necessary, style prefers intmax_t or uintmax_t, since >> those are always wide enough. > > I don't see anything about that in style(9), maybe it should be added > then? Probably; Bruce has mentioned it many times in the past, and as bz@ notes, it's a well-defined type with a well-defined conversion specifier. Also, long long is a bit of a hack that came in before C99 standardized on a few wider types, and the PRIu64 macros are really hideous. Thanks, matthew > In any case, I only changed the %ld format to %lld, because > time_t is int, long or long long depending on arch. Long long is just > the widest type required in this case. > ___ 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: r228878 - head/include
On Thu, Dec 29, 2011 at 6:54 PM, Sean C. Farley wrote: > On Sun, 25 Dec 2011, Ed Schouten wrote: > >> Author: ed >> Date: Sun Dec 25 20:15:41 2011 >> New Revision: 228878 >> URL: http://svn.freebsd.org/changeset/base/228878 >> >> Log: >> Remove unneeded guard. >> >> There is no reason why needs an include guard. It is already >> protected by __bool_true_false_are_defined. >> >> Modified: >> head/include/stdbool.h >> >> Modified: head/include/stdbool.h >> >> == >> --- head/include/stdbool.h Sun Dec 25 18:15:31 2011 (r228877) >> +++ head/include/stdbool.h Sun Dec 25 20:15:41 2011 (r228878) >> @@ -26,9 +26,6 @@ >> * $FreeBSD$ >> */ >> >> -#ifndef _STDBOOL_H_ >> -#define _STDBOOL_H_ >> - >> #ifndef __bool_true_false_are_defined >> #define __bool_true_false_are_defined 1 >> >> @@ -44,5 +41,3 @@ typedef int _Bool; >> >> #endif /* !__cplusplus */ >> #endif /* __bool_true_false_are_defined */ >> - >> -#endif /* !_STDBOOL_H_ */ > > > I just thought of this while reviewing the change: should > __bool_true_false_are_defined be set only if __cplusplus is not set? It > should be set for C99, but I wonder if it should be set for C++. My quick googling didn't show anything at all about the C++ standard and stdbool.h or __bool_true_false_are_defined. It was probably originally set because bool, true, and false are all C++ keywords so certain code that wanted to ifdef on them didn't also need to check __cplusplus. > Also, is there a style requirement that the guard for a header file be based > off of the name of the file? I did not see anything obvious for this within > style(9), but I am curious. I think it's just common use to make sure different headers use a different include guard, so they only protect their contents, not any other file's. The C standard only mentions the symbols bool, true, false, and __bool_true_false_are_defined in regards to stdbool.h. Cheers, 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: r221604 - head/usr.sbin/usbdump
On Sat, May 7, 2011 at 9:36 AM, Hans Petter Selasky wrote: > On Saturday 07 May 2011 18:28:24 Hans Petter Selasky wrote: >> - Use memcpy() instead of bcopy(). > > - Use memset() instead of bzero(). Why? It usually falls through to the same code in libc. Is there some standardization on memfoo versus bfoo here? 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: r221993 - in head/sys: kern sys
On Mon, May 16, 2011 at 9:18 AM, Poul-Henning Kamp wrote:
> Author: phk
> Date: Mon May 16 16:18:40 2011
> New Revision: 221993
> URL: http://svn.freebsd.org/changeset/base/221993
>
> Log:
> Change the length quantities of sbufs to be ssize_t rather than int.
Why?
I don't object at all to changing the API to return ssize_t (though I
see no need), but changing the struct breaks the KBI for no
perceivable gain. Do we really expect someone to put more than 2GB
into something that is a string buffer?
Thanks,
matthew
> Constify a couple of arguments.
>
> Modified:
> head/sys/kern/subr_sbuf.c
> head/sys/sys/sbuf.h
>
> Modified: head/sys/kern/subr_sbuf.c
> ==
> --- head/sys/kern/subr_sbuf.c Mon May 16 15:59:50 2011 (r221992)
> +++ head/sys/kern/subr_sbuf.c Mon May 16 16:18:40 2011 (r221993)
> @@ -94,7 +94,8 @@ _assert_sbuf_integrity(const char *fun,
> KASSERT(s->s_buf != NULL,
> ("%s called with uninitialized or corrupt sbuf", fun));
> KASSERT(s->s_len < s->s_size,
> - ("wrote past end of sbuf (%d >= %d)", s->s_len, s->s_size));
> + ("wrote past end of sbuf (%jd >= %jd)",
> + (intmax_t)s->s_len, (intmax_t)s->s_size));
> }
>
> static void
> @@ -255,16 +256,17 @@ sbuf_clear(struct sbuf *s)
> * Effectively truncates the sbuf at the new position.
> */
> int
> -sbuf_setpos(struct sbuf *s, int pos)
> +sbuf_setpos(struct sbuf *s, ssize_t pos)
> {
>
> assert_sbuf_integrity(s);
> assert_sbuf_state(s, 0);
>
> KASSERT(pos >= 0,
> - ("attempt to seek to a negative position (%d)", pos));
> + ("attempt to seek to a negative position (%jd)", (intmax_t)pos));
> KASSERT(pos < s->s_size,
> - ("attempt to seek past end of sbuf (%d >= %d)", pos, s->s_size));
> + ("attempt to seek past end of sbuf (%jd >= %jd)",
> + (intmax_t)pos, (intmax_t)s->s_size));
>
> if (pos < 0 || pos > s->s_len)
> return (-1);
> @@ -640,7 +642,7 @@ sbuf_trim(struct sbuf *s)
> * Check if an sbuf has an error.
> */
> int
> -sbuf_error(struct sbuf *s)
> +sbuf_error(const struct sbuf *s)
> {
>
> return (s->s_error);
> @@ -691,7 +693,7 @@ sbuf_data(struct sbuf *s)
> /*
> * Return the length of the sbuf data.
> */
> -int
> +ssize_t
> sbuf_len(struct sbuf *s)
> {
>
> @@ -728,7 +730,7 @@ sbuf_delete(struct sbuf *s)
> * Check if an sbuf has been finished.
> */
> int
> -sbuf_done(struct sbuf *s)
> +sbuf_done(const struct sbuf *s)
> {
>
> return (SBUF_ISFINISHED(s));
>
> Modified: head/sys/sys/sbuf.h
> ==
> --- head/sys/sys/sbuf.h Mon May 16 15:59:50 2011 (r221992)
> +++ head/sys/sys/sbuf.h Mon May 16 16:18:40 2011 (r221993)
> @@ -44,8 +44,8 @@ struct sbuf {
> sbuf_drain_func *s_drain_func; /* drain function */
> void *s_drain_arg; /* user-supplied drain argument */
> int s_error; /* current error code */
> - int s_size; /* size of storage buffer */
> - int s_len; /* current length of string */
> + ssize_t s_size; /* size of storage buffer */
> + ssize_t s_len; /* current length of string */
> #define SBUF_FIXEDLEN 0x /* fixed length buffer
> (default) */
> #define SBUF_AUTOEXTEND 0x0001 /* automatically extend
> buffer */
> #define SBUF_USRFLAGMSK 0x /* mask of flags the user may
> specify */
> @@ -63,7 +63,7 @@ struct sbuf *sbuf_new(struct sbuf *, cha
> #define sbuf_new_auto() \
> sbuf_new(NULL, NULL, 0, SBUF_AUTOEXTEND)
> void sbuf_clear(struct sbuf *);
> -int sbuf_setpos(struct sbuf *, int);
> +int sbuf_setpos(struct sbuf *, ssize_t);
> int sbuf_bcat(struct sbuf *, const void *, size_t);
> int sbuf_bcpy(struct sbuf *, const void *, size_t);
> int sbuf_cat(struct sbuf *, const char *);
> @@ -75,11 +75,11 @@ int sbuf_vprintf(struct sbuf *, const
> int sbuf_putc(struct sbuf *, int);
> void sbuf_set_drain(struct sbuf *, sbuf_drain_func *, void *);
> int sbuf_trim(struct sbuf *);
> -int sbuf_error(struct sbuf *);
> +int sbuf_error(const struct sbuf *);
> int sbuf_finish(struct sbuf *);
> char *sbuf_data(struct sbuf *);
> -int sbuf_len(struct sbuf *);
> -int sbuf_done(struct sbuf *);
> +ssize_t sbuf_len(struct sbuf *);
> +int sbuf_done(const struct sbuf *);
> void sbuf_delete(struct sbuf *);
>
> #ifdef _KERNEL
>
___
svn-src-head@freebsd.org mailing list
h
Re: svn commit: r222084 - head/contrib/gperf/src
On Wed, May 18, 2011 at 2:31 PM, Dimitry Andric wrote: > On 2011-05-18 23:16, Pawel Jakub Dawidek wrote: >> >> On Wed, May 18, 2011 at 09:06:20PM +, Ben Laurie wrote: >>> >>> Author: benl >>> Date: Wed May 18 21:06:20 2011 >>> New Revision: 222084 >>> URL: http://svn.freebsd.org/changeset/base/222084 >>> >>> Log: >>> Fix clang warnings. >>> >>> Approved by: philip (mentor) >> >> [...] >>> >>> - fprintf (stderr, " by changing asso_value['%c'] (char #%d) >>> to %d\n", >>> + fprintf (stderr, " by changing asso_value['%c'] (char #%zd) >>> to %d\n", >>> *p, p - union_set + 1, asso_values[(unsigned >>> char)(*p)]); >> >> Hmm, both 'p' and 'union_set' are 'char *' and %zd is for ssize_t. It is >> a bit strange that it fixes the warning. > > If you subtract two pointers, such as in this case, you get a ptrdiff_t. > > Strictly, this doesn't have to be exactly the same type as ssize_t, but > in practice it will almost always be. > > You can also cast the result to intmax_t, and use %jd, then it will > always be correct, but possibly have some small overhead. Or you can use %td which is the C99 conversion specifier for ptrdiff_t. 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: r221853 - in head/sys: dev/md dev/null sys vm
On Sat, May 28, 2011 at 12:03 PM, Pieter de Goeje wrote: > On Friday 13 May 2011 20:48:01 Matthew D Fleming wrote: >> Author: mdf >> Date: Fri May 13 18:48:00 2011 >> New Revision: 221853 >> URL: http://svn.freebsd.org/changeset/base/221853 >> >> Log: >> Usa a globally visible region of zeros for both /dev/zero and the md >> device. There are likely other kernel uses of "blob of zeros" than can >> be converted. >> >> Reviewed by: alc >> MFC after: 1 week >> > > This change seems to reduce /dev/zero performance by 68% as measured by this > command: dd if=/dev/zero of=/dev/null bs=64k count=10. > > x dd-8-stable > + dd-9-current > +-+ > |+ | > |+ | > |+ | > |+ x x| > |+ x x x| > |A |MA_|| > +-+ > N Min Max Median Avg Stddev > x 5 1.2573578e+10 1.3156063e+10 1.2827355e+10 1.290079e+10 2.4951207e+08 > + 5 4.1271391e+09 4.1453925e+09 4.1295157e+09 4.1328097e+09 7487363.6 > Difference at 95.0% confidence > -8.76798e+09 +/- 2.57431e+08 > -67.9647% +/- 1.99547% > (Student's t, pooled s = 1.76511e+08) > > This particular measurement was against 8-stable but the results are the same > for -current just before this commit. Basically througput drops from > ~13GB/sec to 4GB/sec. > > Hardware is a Phenom II X4 945 with 8GB of 800Mhz DDR2 memory. FreeBSD/amd64 > is installed. This processor has 6MB of L3 cache. > > To me it looks like it's not able to cache the zeroes anymore. Is this > intentional? I tried to change ZERO_REGION_SIZE back to 64K but that didn't > help. Hmm. I don't have access to my FreeBSD box over the weekend, but I'll run this on my box when I get back to work. Meanwhile you could try setting ZERO_REGION_SIZE to PAGE_SIZE and I think that will restore things to the original performance. Cheers, 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: r221853 - in head/sys: dev/md dev/null sys vm
On Mon, May 30, 2011 at 8:25 AM, Bruce Evans wrote: > On Sat, 28 May 2011 m...@freebsd.org wrote: > >> On Sat, May 28, 2011 at 12:03 PM, Pieter de Goeje >> wrote: >>> >>> On Friday 13 May 2011 20:48:01 Matthew D Fleming wrote: >>>> >>>> Author: mdf >>>> Date: Fri May 13 18:48:00 2011 >>>> New Revision: 221853 >>>> URL: http://svn.freebsd.org/changeset/base/221853 >>>> >>>> Log: >>>> Usa a globally visible region of zeros for both /dev/zero and the md >>>> device. There are likely other kernel uses of "blob of zeros" than >>>> can >>>> be converted. >>>> >>>> Reviewed by: alc >>>> MFC after: 1 week >>> >>> This change seems to reduce /dev/zero performance by 68% as measured by >>> this >>> command: dd if=/dev/zero of=/dev/null bs=64k count=10. >>> >>> x dd-8-stable >>> + dd-9-current >>> >>> +-+ >>> |+ >>> | > > Argh, hard \xa0. > > [...binary garbage deleted] > >>> This particular measurement was against 8-stable but the results are the >>> same >>> for -current just before this commit. Basically througput drops from >>> ~13GB/sec to 4GB/sec. >>> >>> Hardware is a Phenom II X4 945 with 8GB of 800Mhz DDR2 memory. >>> FreeBSD/amd64 >>> is installed. This processor has 6MB of L3 cache. >>> >>> To me it looks like it's not able to cache the zeroes anymore. Is this >>> intentional? I tried to change ZERO_REGION_SIZE back to 64K but that >>> didn't >>> help. >> >> Hmm. I don't have access to my FreeBSD box over the weekend, but I'll >> run this on my box when I get back to work. >> >> Meanwhile you could try setting ZERO_REGION_SIZE to PAGE_SIZE and I >> think that will restore things to the original performance. > > Using /dev/zero always thrashes caches by the amount size> + (unless the arch uses nontemporal memory > accesses for uiomove, which none do AFAIK). So a large source buffer > is always just a pessimization. A large target buffer size is also a > pessimization, but for the target buffer a fairly large size is needed > to amortize the large syscall costs. In this PR, the target buffer > size is 64K. ZERO_REGION_SIZE is 64K on i386 and 2M on amd64. 64K+64K > on i386 is good for thrashing the L1 cache. That depends -- is the cache virtually or physically addressed? The zero_region only has 4k (PAGE_SIZE) of unique physical addresses. So most of the cache thrashing is due to the user-space buffer, if the cache is physically addressed. It will only have a > noticeable impact on a current L2 cache in competition with other > threads. It is hard to fit everything in the L1 cache even with > non-bloated buffer sizes and 1 thread (16 for the source (I)cache, 0 > for the source (D)cache and 4K for the target cache might work). On > amd64, 2M+2M is good for thrashing most L2 caches. In this PR, the > thrashing is limited by the target buffer size to about 64K+64K, up > from 4K+64K, and it is marginal whether the extra thrashing from the > larger source buffer makes much difference. > > The old zbuf source buffer size of PAGE_SIZE was already too large. Wouldn't this depend on how far down from the use of the buffer the actual copy happens? Another advantage to a large virtual buffer is that it reduces the number of times the copy loop in uiomove has to return up to the device layer that initiated the copy. This is all pretty fast, but again assuming a physical cache fewer trips is better. Thanks, matthew > The source buffer size only needs to be large enough to amortize > loop overhead. 1 cache line is enough in most cases. uiomove() > and copyout() unfortunately don't support copying from register > space, so there must be a source buffer. This may limit the bandwidth > by a factor of 2 in some cases, since most modern CPUs can execute > either 2 64-bit stores or 1 64-bit store and 1 64-bit load per cycle > if everything is already in the L1 cache. However, target buffers > for /dev/zero (or any user i/o) probably need to be larger than the > L1 cache to amortize the syscall overhead, so there are usually plenty > of cycles to spare for the unnecessary loads while the stores wait for > caches. > > This behaviour is easy to see for regular files too (regular files get > copied out from the buffer cache). You have limited control on the > amount of thrashing by changing the target buffer size, and can determine > cache sizes by looking at throughputs. > > Bruce ___ 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: r222537 - in head/sys: kern sys
On Tue, May 31, 2011 at 10:29 AM, Kenneth D. Merry wrote: > Author: ken > Date: Tue May 31 17:29:58 2011 > New Revision: 222537 > URL: http://svn.freebsd.org/changeset/base/222537 > > Log: > Fix apparent garbage in the message buffer. > > While we have had a fix in place (options PRINTF_BUFR_SIZE=128) to fix > scrambled console output, the message buffer and syslog were still getting > log messages one character at a time. While all of the characters still > made it into the log (courtesy of atomic operations), they were often > interleaved when there were multiple threads writing to the buffer at the > same time. This seems to panic my box with "lock "msgbuf" 0xfe0127e0 already initialized". Unfortunately, though I booted with a fresh CURRENT this morning successfully, both /boot/kernel and /boot/kernel.old give this panic. To add insult to injury, when the kernel drops into the debugger, my keyboard input no longer works so I can't get a stack, etc. So: 1) Is there anything else I can do to help debug this? 2) how can I resurrect this box without a reinstall? I will try to repro on a virtual machine so I have a snapshot to come back to. Thanks, matthew > This fixes message buffer accesses to use buffering logic as well, so that > strings that are less than PRINTF_BUFR_SIZE will be put into the message > buffer atomically. So now dmesg output should look the same as console > output. > > subr_msgbuf.c: Convert most message buffer calls to use a new > spin > lock instead of atomic variables in some places. > > Add a new routine, msgbuf_addstr(), that adds a > NUL-terminated string to a message buffer. This > takes a priority argument, which allows us to > eliminate some races (at least in the the string > at a time case) that are present in the > implementation of msglogchar(). (dangling and > lastpri are static variables, and are subject to > races when multiple callers are present.) > > msgbuf_addstr() also allows the caller to request > that carriage returns be stripped out of the > string. This matches the behavior of msglogchar(), > but in testing so far it doesn't appear that any > newlines are being stripped out. So the carriage > return removal functionality may be a candidate > for removal later on if further analysis shows > that it isn't necessary. > > subr_prf.c: Add a new msglogstr() routine that calls > msgbuf_logstr(). > > Rename putcons() to putbuf(). This now handles > buffered output to the message log as well as > the console. Also, remove the logic in putcons() > (now putbuf()) that added a carriage return before > a newline. The console path was the only path that > needed it, and cnputc() (called by cnputs()) > already adds a carriage return. So this > duplication resulted in kernel-generated console > output lines ending in '\r''\r''\n'. > > Refactor putchar() to handle the new buffering > scheme. > > Add buffering to log(). > > Change log_console() to use msglogstr() instead of > msglogchar(). Don't add extra newlines by default > in log_console(). Hide that behavior behind a > tunable/sysctl (kern.log_console_add_linefeed) for > those who would like the old behavior. The old > behavior led to the insertion of extra newlines > for log output for programs that print out a > string, and then a trailing newline on a separate > write. (This is visible with dmesg -a.) > > msgbuf.h: Add a prototype for msgbuf_addstr(). > > Add three new fields to struct msgbuf, msg_needsnl, > msg_lastpri and msg_lock. The first two are needed > for log message functionality previously handled > by msglogchar(). (Which is still active if > buffering isn't enabled.) > > Include sys/lock.h and sys/mutex.h for the new > mutex. > > Reviewed by: gibbs > > Modified: > head/sys/kern/subr_msgbuf.c > head/sys/kern/subr_prf.c > head/sys/sys/msgbuf.h > > Modified: head/sys/kern/subr_msgbuf.c
Re: svn commit: r222537 - in head/sys: kern sys
On Tue, May 31, 2011 at 2:46 PM, Kenneth D. Merry wrote: > On Tue, May 31, 2011 at 14:00:18 -0700, m...@freebsd.org wrote: >> On Tue, May 31, 2011 at 10:29 AM, Kenneth D. Merry wrote: >> > Author: ken >> > Date: Tue May 31 17:29:58 2011 >> > New Revision: 222537 >> > URL: http://svn.freebsd.org/changeset/base/222537 >> > >> > Log: >> > ?Fix apparent garbage in the message buffer. >> > >> > ?While we have had a fix in place (options PRINTF_BUFR_SIZE=128) to fix >> > ?scrambled console output, the message buffer and syslog were still getting >> > ?log messages one character at a time. ?While all of the characters still >> > ?made it into the log (courtesy of atomic operations), they were often >> > ?interleaved when there were multiple threads writing to the buffer at the >> > ?same time. >> >> This seems to panic my box with "lock "msgbuf" 0xfe0127e0 >> already initialized". >> >> Unfortunately, though I booted with a fresh CURRENT this morning >> successfully, both /boot/kernel and /boot/kernel.old give this panic. >> To add insult to injury, when the kernel drops into the debugger, my >> keyboard input no longer works so I can't get a stack, etc. > > Uh-oh! > >> So: >> >> 1) Is there anything else I can do to help debug this? >> 2) how can I resurrect this box without a reinstall? >> >> I will try to repro on a virtual machine so I have a snapshot to come back >> to. > > My guess is that this is an issue with the message buffer reinitialization > path. lock_init() (called by mtx_init()) has an assert to make sure that > the lock is initialized, and that is just a flag check. > > Since the spin lock is part of the message buffer structure, if it is held > over from a previous boot, the LO_INITIALIZED flag may still be set. > > Try power cycling the machine. If it is an issue with re-initialization, > that should clear the memory and allow you to boot. Hmm, apparently my previous presses of the power button weren't long enough. I let it sit off for 20 seconds and it boots okay now. Thanks, matthew > My testing has been with VMs (under Xen), so the reinit path has probably > not been tested as fully as it should have been. Sorry about that! > > As for the debugger, that's another issue altogether. It does work for me, > but then again if the spin lock initialization is broken for the message > buffer that may affect things. > > Try a cold boot and see if that helps. If so, I think we can probably just > bzero the mutex in msgbuf_reinit() and that will fix things. > > Ken > -- > Kenneth Merry > k...@freebsd.org > ___ 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: r222537 - in head/sys: kern sys
On Tue, May 31, 2011 at 3:11 PM, Kenneth D. Merry wrote: > On Tue, May 31, 2011 at 15:02:37 -0700, m...@freebsd.org wrote: >> On Tue, May 31, 2011 at 2:46 PM, Kenneth D. Merry wrote: >> > On Tue, May 31, 2011 at 14:00:18 -0700, m...@freebsd.org wrote: >> >> On Tue, May 31, 2011 at 10:29 AM, Kenneth D. Merry >> >> wrote: >> >> > Author: ken >> >> > Date: Tue May 31 17:29:58 2011 >> >> > New Revision: 222537 >> >> > URL: http://svn.freebsd.org/changeset/base/222537 >> >> > >> >> > Log: >> >> > ?Fix apparent garbage in the message buffer. >> >> > >> >> > ?While we have had a fix in place (options PRINTF_BUFR_SIZE=128) to fix >> >> > ?scrambled console output, the message buffer and syslog were still >> >> > getting >> >> > ?log messages one character at a time. ?While all of the characters >> >> > still >> >> > ?made it into the log (courtesy of atomic operations), they were often >> >> > ?interleaved when there were multiple threads writing to the buffer at >> >> > the >> >> > ?same time. >> >> >> >> This seems to panic my box with "lock "msgbuf" 0xfe0127e0 >> >> already initialized". >> >> >> >> Unfortunately, though I booted with a fresh CURRENT this morning >> >> successfully, both /boot/kernel and /boot/kernel.old give this panic. >> >> To add insult to injury, when the kernel drops into the debugger, my >> >> keyboard input no longer works so I can't get a stack, etc. >> > >> > Uh-oh! >> > >> >> So: >> >> >> >> 1) Is there anything else I can do to help debug this? >> >> 2) how can I resurrect this box without a reinstall? >> >> >> >> I will try to repro on a virtual machine so I have a snapshot to come >> >> back to. >> > >> > My guess is that this is an issue with the message buffer reinitialization >> > path. ?lock_init() (called by mtx_init()) has an assert to make sure that >> > the lock is initialized, and that is just a flag check. >> > >> > Since the spin lock is part of the message buffer structure, if it is held >> > over from a previous boot, the LO_INITIALIZED flag may still be set. >> > >> > Try power cycling the machine. ?If it is an issue with re-initialization, >> > that should clear the memory and allow you to boot. >> >> Hmm, apparently my previous presses of the power button weren't long >> enough. I let it sit off for 20 seconds and it boots okay now. > > Okay, so it probably is the re-initialization code. Can you try this patch > and see if it survives a warm boot? I also changed the initialization > path, so we don't get tripped up by garbage left in memory. This patch survived a warm reboot (shutdown -r now). > Also, does the debugger work now that it has booted successfully? The debugger (or rather, my keyboard in the debugger) works on a successful boot. I used sysctl debug.kdb.enter=1 to test it. 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: r221853 - in head/sys: dev/md dev/null sys vm
On Tue, May 31, 2011 at 2:48 PM, Pieter de Goeje wrote: > On Sunday 29 May 2011 05:01:57 m...@freebsd.org wrote: >> On Sat, May 28, 2011 at 12:03 PM, Pieter de Goeje wrote: >> > To me it looks like it's not able to cache the zeroes anymore. Is this >> > intentional? I tried to change ZERO_REGION_SIZE back to 64K but that >> > didn't help. >> >> Hmm. I don't have access to my FreeBSD box over the weekend, but I'll >> run this on my box when I get back to work. >> >> Meanwhile you could try setting ZERO_REGION_SIZE to PAGE_SIZE and I >> think that will restore things to the original performance. > > Indeed it does. I couldn't find any authoritative docs stating wether or not > the cache on this CPU is virtually indexed, but apparently at least some of > it is. On my physical box (some Dell thing from about 2008), I ran 10 loops of dd if=/dev/zero of=/dev/null bs=XX count=XX where bs went by powers of 2 from 512 bytes to 2M, and count was set so that the dd always transferred 8GB. I compared ZERO_REGION_SIZE of 64k and 2M on amd64. The summary of the ministat(1) output is: bs=512b - no difference bs=1K - no difference bs=2k - no difference bs=4k - no difference bs=8k - no difference bs=16k - no difference bs=32k - no difference bs=64k - no difference bs=128k - 2M is 0.69% faster bs=256k - 2M is 0.98% faster bs=512k - 2M is 0.65% faster bs=1M - 2M is 1.02% faster bs=2M - 2M is 2.17% slower I'll play again with a 4K buffer. For some applications (/dev/zero) a small size is sufficient. For some (md(4)) a ZERO_REGION_SIZE at least as large as the sectorsize is desired so that a single kernel buffer pointer can be used to set up a uio for VOP_WRITE(9). Attached is the ministat output; I hope it makes it. :-) Thanks, matthew x /data/zero-amd64-small/zero-512.txt + /data/zero-amd64-large/zero-512.txt +--+ | + x| |+ + +x*x+ +x x * x+x +x| | |__|__AM___MA___|___| | +--+ N Min MaxMedian AvgStddev x 10 13.564276 13.666499 13.590373 13.591993 0.030172083 + 10 13.49174 13.616263 13.569925 13.568006 0.033884281 No difference proven at 95.0% confidence x /data/zero-amd64-small/zero-1024.txt + /data/zero-amd64-large/zero-1024.txt +--+ |++ ++xx x x +* ++ xx++| | ||___AAM__M|_| | +--+ N Min MaxMedian AvgStddev x 10 7.155384 7.182849 7.168076 7.16613820.01041489 + 10 7.124263 7.207363 7.170449 7.1647896 0.023453662 No difference proven at 95.0% confidence x /data/zero-amd64-small/zero-2048.txt + /data/zero-amd64-large/zero-2048.txt +--+ | + | |+ + +xx *x +* xx+ ++xx x| ||_|A_M__M__A_|_| | +--+ N Min MaxMedian AvgStddev x 10 3.827242 3.867095 3.837901 3.839988 0.012983755 + 10 3.809213 3.843682 3.835748 3.8302765 0.011340307 No difference proven at 95.0% confidence x /data/zero-amd64-small/zero-4096.txt + /data/zero-amd64-large/zero-4096.txt +--+ |+ + ++xxx x + + * x+ ++ x x x| | |___AM_M_A___|_| | +--+ N Min MaxMedian AvgStddev x 10 2.165541 2.201224 2.173227 2.1769029 0.013803193 + 10 2.161362 2.185911 2.172388 2.1719634 0.0088129371 No difference proven at 95.0% confidence x /data/zero-amd64-small/zero-8192.txt + /data/zero-amd64-large/zero-8192.txt +--+ |+x| |+ x + + +x +x++x+ x xx + x x| | |__|___A__M_A_|___| | +--
Re: svn commit: r221853 - in head/sys: dev/md dev/null sys vm
On Tue, May 31, 2011 at 3:47 PM, wrote: > On Tue, May 31, 2011 at 2:48 PM, Pieter de Goeje wrote: >> On Sunday 29 May 2011 05:01:57 m...@freebsd.org wrote: >>> On Sat, May 28, 2011 at 12:03 PM, Pieter de Goeje wrote: >>> > To me it looks like it's not able to cache the zeroes anymore. Is this >>> > intentional? I tried to change ZERO_REGION_SIZE back to 64K but that >>> > didn't help. >>> >>> Hmm. I don't have access to my FreeBSD box over the weekend, but I'll >>> run this on my box when I get back to work. >>> >>> Meanwhile you could try setting ZERO_REGION_SIZE to PAGE_SIZE and I >>> think that will restore things to the original performance. >> >> Indeed it does. I couldn't find any authoritative docs stating wether or not >> the cache on this CPU is virtually indexed, but apparently at least some of >> it is. > > On my physical box (some Dell thing from about 2008), I ran 10 loops > of dd if=/dev/zero of=/dev/null bs=XX count=XX where bs went by powers > of 2 from 512 bytes to 2M, and count was set so that the dd always > transferred 8GB. I compared ZERO_REGION_SIZE of 64k and 2M on amd64. > > The summary of the ministat(1) output is: > > bs=512b - no difference > bs=1K - no difference > bs=2k - no difference > bs=4k - no difference > bs=8k - no difference > bs=16k - no difference > bs=32k - no difference > bs=64k - no difference > bs=128k - 2M is 0.69% faster > bs=256k - 2M is 0.98% faster > bs=512k - 2M is 0.65% faster > bs=1M - 2M is 1.02% faster > bs=2M - 2M is 2.17% slower > > I'll play again with a 4K buffer. The data is harder to parse precisely, but in general it looks like on my box using a 4K buffer results in significantly worse performance when the dd(1) block size is larger than 4K. How much worse depends on the block size, but it goes from 6% at bs=8k to 17% at bs>=256k. Showing 4k/64k/2M ZERO_REGION_SIZE graphically in the ministat(1) output also makes it clear that the difference between 64k and 2M is nearly insignificant on my box compared to using 4k. http://people.freebsd.org/~mdf/zero-ministat.txt Cheers, 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: r211228 - head/sys/kern
On Thu, Aug 12, 2010 at 9:13 AM, Jung-uk Kim wrote:
> Author: jkim
> Date: Thu Aug 12 16:13:24 2010
> New Revision: 211228
> URL: http://svn.freebsd.org/changeset/base/211228
>
> Log:
> Provide description for 'machdep.disable_rtc_set' sysctl. Clean up style(9)
> nits. Remove a redundant return statement and an unnecessary variable.
>
> Modified:
> head/sys/kern/subr_rtc.c
>
> Modified: head/sys/kern/subr_rtc.c
> ==
> --- head/sys/kern/subr_rtc.c Thu Aug 12 15:46:15 2010 (r211227)
> +++ head/sys/kern/subr_rtc.c Thu Aug 12 16:13:24 2010 (r211228)
> @@ -65,8 +65,8 @@ static long clock_res;
>
> /* XXX: should be kern. now, it's no longer machdep. */
> static int disable_rtc_set;
> -SYSCTL_INT(_machdep, OID_AUTO, disable_rtc_set,
> - CTLFLAG_RW, &disable_rtc_set, 0, "");
> +SYSCTL_INT(_machdep, OID_AUTO, disable_rtc_set, CTLFLAG_RW, &disable_rtc_set,
> + 0, "Disallow adjusting time-of-day clock");
>
> void
> clock_register(device_t dev, long res) /* res has units of microseconds */
> @@ -74,26 +74,22 @@ clock_register(device_t dev, long res) /
>
> if (clock_dev != NULL) {
> if (clock_res > res) {
> - if (bootverbose) {
> + if (bootverbose)
> device_printf(dev, "not installed as "
> "time-of-day clock: clock %s has higher "
> "resolution\n",
> device_get_name(clock_dev));
> - }
While the device_printf() is a single statement, it spans multiple
lines. It seems in this instance that clarity is preserved better by
using braces even though not required by C.
Quoting style(9):
Space after keywords (if, while, for, return, switch). No braces ('{'
and '}') are used for control statements with zero or only a single
statement unless that statement is more than a single line in which case
they are permitted.
So both styles are accepted, and I would think that changing this is
needless code churn.
Thanks,
matthew
> return;
> - } else {
> - if (bootverbose) {
> - device_printf(clock_dev, "removed as "
> - "time-of-day clock: clock %s has higher "
> - "resolution\n", device_get_name(dev));
> - }
> }
> + if (bootverbose)
> + device_printf(clock_dev, "removed as "
> + "time-of-day clock: clock %s has higher "
> + "resolution\n", device_get_name(dev));
> }
> clock_dev = dev;
> clock_res = res;
> - if (bootverbose) {
> + if (bootverbose)
> device_printf(dev, "registered as a time-of-day clock "
> "(resolution %ldus)\n", res);
> - }
> }
>
> /*
> @@ -109,7 +105,7 @@ clock_register(device_t dev, long res) /
> void
> inittodr(time_t base)
> {
> - struct timespec ref, ts;
> + struct timespec ts;
> int error;
>
> if (clock_dev == NULL) {
> @@ -136,9 +132,9 @@ inittodr(time_t base)
>
> wrong_time:
> if (base > 0) {
> - ref.tv_sec = base;
> - ref.tv_nsec = 0;
> - tc_setclock(&ref);
> + ts.tv_sec = base;
> + ts.tv_nsec = 0;
> + tc_setclock(&ts);
> }
> }
>
> @@ -157,9 +153,7 @@ resettodr(void)
> getnanotime(&ts);
> ts.tv_sec -= utc_offset();
> /* XXX: We should really set all registered RTCs */
> - if ((error = CLOCK_SETTIME(clock_dev, &ts)) != 0) {
> + if ((error = CLOCK_SETTIME(clock_dev, &ts)) != 0)
> printf("warning: clock_settime failed (%d), time-of-day clock "
> "not adjusted to system time\n", error);
> - return;
> - }
> }
>
___
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: r211284 - head/sys/kern
On Fri, Aug 13, 2010 at 12:20 PM, Pawel Jakub Dawidek wrote:
> Author: pjd
> Date: Fri Aug 13 19:20:35 2010
> New Revision: 211284
> URL: http://svn.freebsd.org/changeset/base/211284
>
> Log:
> Simplify taskqueue_drain() by using proved macros.
Thanks! This was on my to-do list since I'd been in this file, once I
could figure out why it hand't already been done. :-)
Cheers,
matthew
>
> Modified:
> head/sys/kern/subr_taskqueue.c
>
> Modified: head/sys/kern/subr_taskqueue.c
> ==
> --- head/sys/kern/subr_taskqueue.c Fri Aug 13 18:17:32 2010
> (r211283)
> +++ head/sys/kern/subr_taskqueue.c Fri Aug 13 19:20:35 2010
> (r211284)
> @@ -248,23 +248,16 @@ taskqueue_run(struct taskqueue *queue, s
> void
> taskqueue_drain(struct taskqueue *queue, struct task *task)
> {
> - if (queue->tq_spin) { /* XXX */
> - mtx_lock_spin(&queue->tq_mutex);
> - while (task->ta_pending != 0 ||
> - (task->ta_running != NULL && task == *task->ta_running)) {
> - msleep_spin(task, &queue->tq_mutex, "-", 0);
> - }
> - mtx_unlock_spin(&queue->tq_mutex);
> - } else {
> +
> + if (!queue->tq_spin)
> WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL, __func__);
>
> - mtx_lock(&queue->tq_mutex);
> - while (task->ta_pending != 0 ||
> - (task->ta_running != NULL && task == *task->ta_running)) {
> - msleep(task, &queue->tq_mutex, PWAIT, "-", 0);
> - }
> - mtx_unlock(&queue->tq_mutex);
> + TQ_LOCK(queue);
> + while (task->ta_pending != 0 ||
> + (task->ta_running != NULL && task == *task->ta_running)) {
> + TQ_SLEEP(queue, task, &queue->tq_mutex, PWAIT, "-", 0);
> }
> + TQ_UNLOCK(queue);
> }
>
> static void
>
___
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: r211304 - head/lib/libutil
On Sat, Aug 14, 2010 at 2:34 PM, Dag-Erling Smorgrav wrote:
> Author: des
> Date: Sat Aug 14 14:34:36 2010
> New Revision: 211304
> URL: http://svn.freebsd.org/changeset/base/211304
>
> Log:
> Simplify expand_number() by combining the (unrolled) loop with the
> switch. Since expand_number() does not accept negative numbers, switch
> from int64_t to uint64_t; this makes it easier to check for overflow.
>
> MFC after: 3 weeks
>
> Modified:
> head/lib/libutil/expand_number.c
> head/lib/libutil/libutil.h
>
> Modified: head/lib/libutil/expand_number.c
> ==
> --- head/lib/libutil/expand_number.c Sat Aug 14 14:18:02 2010
> (r211303)
> +++ head/lib/libutil/expand_number.c Sat Aug 14 14:34:36 2010
> (r211304)
> @@ -37,7 +37,7 @@ __FBSDID("$FreeBSD$");
>
> /*
> * Convert an expression of the following forms to a int64_t.
> - * 1) A positive decimal number.
> + * 1) A positive decimal number.
> * 2) A positive decimal number followed by a 'b' or 'B' (mult by 1).
> * 3) A positive decimal number followed by a 'k' or 'K' (mult by 1 <<
> 10).
> * 4) A positive decimal number followed by a 'm' or 'M' (mult by 1 <<
> 20).
> @@ -47,14 +47,12 @@ __FBSDID("$FreeBSD$");
> * 8) A positive decimal number followed by a 'e' or 'E' (mult by 1 <<
> 60).
> */
> int
> -expand_number(const char *buf, int64_t *num)
> +expand_number(const char *buf, uint64_t *num)
> {
> - static const char unit[] = "bkmgtpe";
> - char *endptr, s;
> - int64_t number;
> - int i;
> + uint64_t number;
> + char *endptr;
>
> - number = strtoimax(buf, &endptr, 0);
> + number = strtoumax(buf, &endptr, 0);
>
> if (endptr == buf) {
> /* No valid digits. */
> @@ -68,15 +66,23 @@ expand_number(const char *buf, int64_t *
> return (0);
> }
>
> - s = tolower(*endptr);
> - switch (s) {
> - case 'b':
> - case 'k':
> - case 'm':
> - case 'g':
> - case 't':
> - case 'p':
> +#define SHIFT(n, b) \
> + do { if ((n << b) < n) goto overflow; n <<= b; } while (0)
I think it's possible for the number to overflow but also not shrink.
e.g. 0x12345678T.
Perhaps if (((n << b) >> b) != n) would be better.
Thanks,
matthew
> +
> + switch (tolower((unsigned char)*endptr)) {
> case 'e':
> + SHIFT(number, 10);
> + case 'p':
> + SHIFT(number, 10);
> + case 't':
> + SHIFT(number, 10);
> + case 'g':
> + SHIFT(number, 10);
> + case 'm':
> + SHIFT(number, 10);
> + case 'k':
> + SHIFT(number, 10);
> + case 'b':
> break;
> default:
> /* Unrecognized unit. */
> @@ -84,17 +90,11 @@ expand_number(const char *buf, int64_t *
> return (-1);
> }
>
> - for (i = 0; unit[i] != '\0'; i++) {
> - if (s == unit[i])
> - break;
> - if ((number < 0 && (number << 10) > number) ||
> - (number >= 0 && (number << 10) < number)) {
> - errno = ERANGE;
> - return (-1);
> - }
> - number <<= 10;
> - }
> -
> *num = number;
> return (0);
> +
> +overflow:
> + /* Overflow */
> + errno = ERANGE;
> + return (-1);
> }
>
> Modified: head/lib/libutil/libutil.h
> ==
> --- head/lib/libutil/libutil.h Sat Aug 14 14:18:02 2010 (r211303)
> +++ head/lib/libutil/libutil.h Sat Aug 14 14:34:36 2010 (r211304)
> @@ -109,7 +109,7 @@ int forkpty(int *_amaster, char *_name,
> struct termios *_termp, struct winsize *_winp);
> int humanize_number(char *_buf, size_t _len, int64_t _number,
> const char *_suffix, int _scale, int _flags);
> -int expand_number(const char *_buf, int64_t *_num);
> +int expand_number(const char *_buf, uint64_t *_num);
> const char *uu_lockerr(int _uu_lockresult);
> int uu_lock(const char *_ttyname);
> int uu_unlock(const char *_ttyname);
>
___
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: r211463 - head/usr.bin/grep
On Wed, Aug 18, 2010 at 10:40 AM, Gabor Kovesdan wrote:
> Author: gabor
> Date: Wed Aug 18 17:40:10 2010
> New Revision: 211463
> URL: http://svn.freebsd.org/changeset/base/211463
>
> Log:
> - Refactor file reading code to use pure syscalls and an internal buffer
> instead of stdio. This gives BSD grep a very big performance boost,
> its speed is now almost comparable to GNU grep.
I didn't read all of the details in the profiling mails in the thread,
but does this mean that work on stdio would give a performance boost
to many apps? Or is there something specific about how grep(1) is
using its input that makes it a horse of a different color?
Thanks,
matthew
>
> Submitted by: Dimitry Andric
> Approved by: delphij (mentor)
>
> Modified:
> head/usr.bin/grep/fastgrep.c
> head/usr.bin/grep/file.c
> head/usr.bin/grep/grep.h
> head/usr.bin/grep/util.c
>
> Modified: head/usr.bin/grep/fastgrep.c
> ==
> --- head/usr.bin/grep/fastgrep.c Wed Aug 18 17:39:47 2010
> (r211462)
> +++ head/usr.bin/grep/fastgrep.c Wed Aug 18 17:40:10 2010
> (r211463)
> @@ -198,7 +198,7 @@ fastcomp(fastgrep_t *fg, const char *pat
> }
>
> int
> -grep_search(fastgrep_t *fg, unsigned char *data, size_t len, regmatch_t
> *pmatch)
> +grep_search(fastgrep_t *fg, const unsigned char *data, size_t len,
> regmatch_t *pmatch)
> {
> unsigned int j;
> int ret = REG_NOMATCH;
>
> Modified: head/usr.bin/grep/file.c
> ==
> --- head/usr.bin/grep/file.c Wed Aug 18 17:39:47 2010 (r211462)
> +++ head/usr.bin/grep/file.c Wed Aug 18 17:40:10 2010 (r211463)
> @@ -2,7 +2,8 @@
>
> /*-
> * Copyright (c) 1999 James Howard and Dag-Erling Coïdan Smørgrav
> - * Copyright (C) 2008-2009 Gabor Kovesdan
> + * Copyright (C) 2008-2010 Gabor Kovesdan
> + * Copyright (C) 2010 Dimitry Andric
> * All rights reserved.
> *
> * Redistribution and use in source and binary forms, with or without
> @@ -37,7 +38,8 @@ __FBSDID("$FreeBSD$");
> #include
> #include
> #include
> -#include
> +#include
> +#include
> #include
> #include
> #include
> @@ -47,222 +49,204 @@ __FBSDID("$FreeBSD$");
>
> #include "grep.h"
>
> -static char fname[MAXPATHLEN]; /* file name */
> +#define MAXBUFSIZ (32 * 1024)
> +#define LNBUFBUMP 80
>
> -#define MAXBUFSIZ (16 * 1024)
> -#define PREREAD_M 0.2
> +static gzFile gzbufdesc;
> +static BZFILE* bzbufdesc;
>
> -/* Some global variables for the buffering and reading. */
> -static char *lnbuf;
> -static size_t lnbuflen;
> -static unsigned char *binbuf;
> -static int binbufsiz;
> -unsigned char *binbufptr;
> -static int bzerr;
> +static unsigned char buffer[MAXBUFSIZ];
> +static unsigned char *bufpos;
> +static size_t bufrem;
>
> -#define iswbinary(ch) (!iswspace((ch)) && iswcntrl((ch)) && \
> - (ch != L'\b') && (ch != L'\0'))
> +static unsigned char *lnbuf;
> +static size_t lnbuflen;
>
> -/*
> - * Returns a single character according to the file type.
> - * Returns -1 on failure.
> - */
> static inline int
> -grep_fgetc(struct file *f)
> +grep_refill(struct file *f)
> {
> - unsigned char c;
> + ssize_t nr;
> + int bzerr;
>
> - switch (filebehave) {
> - case FILE_STDIO:
> - return (getc_unlocked(f->f));
> - case FILE_GZIP:
> - return (gzgetc(f->gzf));
> - case FILE_BZIP:
> - BZ2_bzRead(&bzerr, f->bzf, &c, 1);
> - if (bzerr == BZ_STREAM_END)
> - return (-1);
> - else if (bzerr != BZ_SEQUENCE_ERROR && bzerr != BZ_OK)
> - errx(2, "%s", getstr(2));
> - return (c);
> - }
> - return (-1);
> + bufpos = buffer;
> + bufrem = 0;
> +
> + if (filebehave == FILE_GZIP)
> + nr = gzread(gzbufdesc, buffer, MAXBUFSIZ);
> + else if (filebehave == FILE_BZIP && bzbufdesc != NULL) {
> + nr = BZ2_bzRead(&bzerr, bzbufdesc, buffer, MAXBUFSIZ);
> + switch (bzerr) {
> + case BZ_OK:
> + case BZ_STREAM_END:
> + /* No problem, nr will be okay */
> + break;
> + case BZ_DATA_ERROR_MAGIC:
> + /*
> + * As opposed to gzread(), which simply returns the
> + * plain file data, if it is not in the correct
> + * compressed format, BZ2_bzRead() instead aborts.
> + *
> + * So, just restart at the beginning of the file
> again,
> + * and use plain reads from now on.
> + */
> + BZ2_bzReadClose(&bzerr,
Re: svn commit: r212115 - head/sys/kern
On Wed, Sep 1, 2010 at 2:31 PM, John Baldwin wrote: > On Wednesday, September 01, 2010 4:32:48 pm Matthew D Fleming wrote: >> Author: mdf >> Date: Wed Sep 1 20:32:47 2010 >> New Revision: 212115 >> URL: http://svn.freebsd.org/changeset/base/212115 >> >> Log: >> Fix a bug with sched_affinity() where it checks td_pinned of another >> thread in a racy manner, which can lead to attempting to migrate a >> thread that is pinned to a CPU. Instead, have sched_switch() determine >> which CPU a thread should run on if the current one is not allowed. >> >> KASSERT in sched_bind() that the thread is not yet pinned to a CPU. >> >> KASSERT in sched_switch() that only migratable threads or those moving >> due to a sched_bind() are changing CPUs. >> >> sched_affinity code came from j...@. >> >> MFC after: 2 weeks > > Cool, I guess this fixed it in your tests then? Yes, the stress case I pointed out in the earlier thread ran for quite a few minutes before I killed it. Previously it would crash (with the added assert) in a few seconds. 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: r212153 - head/sys/kern
On Thu, Sep 2, 2010 at 9:23 AM, Matthew D Fleming wrote:
> Author: mdf
> Date: Thu Sep 2 16:23:05 2010
> New Revision: 212153
> URL: http://svn.freebsd.org/changeset/base/212153
>
> Log:
> Fix UP build.
Noticed by b.f. (bf1783 at gmail dot com)
We apologize for the inconvenience.
matthew
> MFC after: 2 weeks
>
> Modified:
> head/sys/kern/sched_ule.c
>
> Modified: head/sys/kern/sched_ule.c
> ==
> --- head/sys/kern/sched_ule.c Thu Sep 2 16:11:12 2010 (r212152)
> +++ head/sys/kern/sched_ule.c Thu Sep 2 16:23:05 2010 (r212153)
> @@ -1797,8 +1797,10 @@ sched_switch(struct thread *td, struct t
> srqflag = (flags & SW_PREEMPT) ?
> SRQ_OURSELF|SRQ_YIELDING|SRQ_PREEMPTED :
> SRQ_OURSELF|SRQ_YIELDING;
> +#ifdef SMP
> if (THREAD_CAN_MIGRATE(td) && !THREAD_CAN_SCHED(td,
> ts->ts_cpu))
> ts->ts_cpu = sched_pickcpu(td, 0);
> +#endif
> if (ts->ts_cpu == cpuid)
> tdq_runq_add(tdq, td, srqflag);
> else {
>
___
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: r212182 - head/sys/kern
On Fri, Sep 3, 2010 at 10:23 AM, Matthew D Fleming wrote: > Author: mdf > Date: Fri Sep 3 17:23:26 2010 > New Revision: 212182 > URL: http://svn.freebsd.org/changeset/base/212182 > > Log: > Fix user-space libsbuf build. Why isn't CTASSERT available to > user-space? Sorry for the churn. I am having a un-careful morning, it appears. Thanks, matthew > > Modified: > head/sys/kern/subr_sbuf.c > > Modified: head/sys/kern/subr_sbuf.c > == > --- head/sys/kern/subr_sbuf.c Fri Sep 3 16:12:39 2010 (r212181) > +++ head/sys/kern/subr_sbuf.c Fri Sep 3 17:23:26 2010 (r212182) > @@ -116,8 +116,10 @@ _assert_sbuf_state(const char *fun, stru > > #endif /* _KERNEL && INVARIANTS */ > > +#ifdef _KERNEL > CTASSERT(powerof2(SBUF_MAXEXTENDSIZE)); > CTASSERT(powerof2(SBUF_MAXEXTENDINCR)); > +#endif > > static int > sbuf_extendsize(int size) > ___ 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: r212182 - head/sys/kern
On Mon, Sep 6, 2010 at 6:45 AM, Bruce Evans wrote: > On Fri, 3 Sep 2010, pluknet wrote: > >> On 3 September 2010 21:23, Matthew D Fleming wrote: >>> >>> Log: >>> Fix user-space libsbuf build. Why isn't CTASSERT available to >>> user-space? > > Well, user headers shouldn't be enlisted to check for kernel bugs that > can be checked well enough in the kernel. I agree, but in this case one could define different constants for user space and kernel space, and the code could be wrong only for user-space, where there isn't a compile-time assert. I do always appreciate style and standards advice from Mr Bruce. It's the only way to learn (usually, hopefully, from other people's mistakes ;-) Thanks, matthew >>> Modified: >>> head/sys/kern/subr_sbuf.c >>> >>> Modified: head/sys/kern/subr_sbuf.c >>> >>> == >>> --- head/sys/kern/subr_sbuf.c Fri Sep 3 16:12:39 2010 (r212181) >>> +++ head/sys/kern/subr_sbuf.c Fri Sep 3 17:23:26 2010 (r212182) >>> @@ -116,8 +116,10 @@ _assert_sbuf_state(const char *fun, stru >>> >>> #endif /* _KERNEL && INVARIANTS */ >>> >>> +#ifdef _KERNEL >>> CTASSERT(powerof2(SBUF_MAXEXTENDSIZE)); >>> CTASSERT(powerof2(SBUF_MAXEXTENDINCR)); >>> +#endif >>> >>> static int >>> sbuf_extendsize(int size) >> >> Hi, >> >> as I can see, the next (and maybe preferred) model is used in system >> headers: >> >> #ifdef CTASSERT >> CTASSERT(...); >> #endif > > Needed, even in the kernel, since CTASSERT() is only defined if the > kernel-only header has been included. > > If this macro were defined in a user header, then it would have to be > more global (probably defined in ) but not in the application > namespace (probably spelled __CTASSERT()), so it would be uglier. > > Bruce ___ 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: r212368 - head/sys/dev/pci
On Thu, Sep 9, 2010 at 11:19 AM, John Baldwin wrote: > Author: jhb > Date: Thu Sep 9 18:19:15 2010 > New Revision: 212368 > URL: http://svn.freebsd.org/changeset/base/212368 > > Log: > - Rename the constant for the Master Data Parity Error flag in the > PCI status register to map its current name. > - Use PCIM_* rather than PCIR_* for constants for fields in various AER > registers. I got about half of them right in the previous commit. > > MFC after: 1 week > > Modified: > head/sys/dev/pci/pcireg.h This seems to break building CURRENT with this error: /data/sb/bsd.git/sys/dev/msk/if_msk.c: In function 'mskc_reset': /data/sb/bsd.git/sys/dev/msk/if_msk.c:1337: error: 'PCIM_STATUS_PERRREPORT' undeclared (first use in this function) /data/sb/bsd.git/sys/dev/msk/if_msk.c:1337: error: (Each undeclared identifier is reported only once /data/sb/bsd.git/sys/dev/msk/if_msk.c:1337: error: for each function it appears in.) /data/sb/bsd.git/sys/dev/msk/if_msk.c: In function 'msk_intr_hwerr': /data/sb/bsd.git/sys/dev/msk/if_msk.c:3408: error: 'PCIM_STATUS_PERRREPORT' undeclared (first use in this function) Thanks, matthew > > Modified: head/sys/dev/pci/pcireg.h > == > --- head/sys/dev/pci/pcireg.h Thu Sep 9 17:49:18 2010 (r212367) > +++ head/sys/dev/pci/pcireg.h Thu Sep 9 18:19:15 2010 (r212368) > @@ -67,7 +67,7 @@ > #define PCIM_STATUS_CAPPRESENT 0x0010 > #define PCIM_STATUS_66CAPABLE 0x0020 > #define PCIM_STATUS_BACKTOBACK 0x0080 > -#define PCIM_STATUS_PERRREPORT 0x0100 > +#define PCIM_STATUS_MDPERR 0x0100 > #define PCIM_STATUS_SEL_FAST 0x > #define PCIM_STATUS_SEL_MEDIMUM 0x0200 > #define PCIM_STATUS_SEL_SLOW 0x0400 > @@ -689,18 +689,18 @@ > > /* Advanced Error Reporting */ > #define PCIR_AER_UC_STATUS 0x04 > -#define PCIR_AER_UC_TRAINING_ERROR 0x0001 > -#define PCIR_AER_UC_DL_PROTOCOL_ERROR 0x0010 > -#define PCIR_AER_UC_POISONED_TLP 0x1000 > -#define PCIR_AER_UC_FC_PROTOCOL_ERROR 0x2000 > -#define PCIR_AER_UC_COMPLETION_TIMEOUT 0x4000 > -#define PCIR_AER_UC_COMPLETER_ABORT 0x8000 > -#define PCIR_AER_UC_UNEXPECTED_COMPLETION 0x0001 > -#define PCIR_AER_UC_RECEIVER_OVERFLOW 0x0002 > -#define PCIR_AER_UC_MALFORMED_TLP 0x0004 > -#define PCIR_AER_UC_ECRC_ERROR 0x0008 > -#define PCIR_AER_UC_UNSUPPORTED_REQUEST 0x0010 > -#define PCIR_AER_UC_ACS_VIOLATION 0x0020 > +#define PCIM_AER_UC_TRAINING_ERROR 0x0001 > +#define PCIM_AER_UC_DL_PROTOCOL_ERROR 0x0010 > +#define PCIM_AER_UC_POISONED_TLP 0x1000 > +#define PCIM_AER_UC_FC_PROTOCOL_ERROR 0x2000 > +#define PCIM_AER_UC_COMPLETION_TIMEOUT 0x4000 > +#define PCIM_AER_UC_COMPLETER_ABORT 0x8000 > +#define PCIM_AER_UC_UNEXPECTED_COMPLETION 0x0001 > +#define PCIM_AER_UC_RECEIVER_OVERFLOW 0x0002 > +#define PCIM_AER_UC_MALFORMED_TLP 0x0004 > +#define PCIM_AER_UC_ECRC_ERROR 0x0008 > +#define PCIM_AER_UC_UNSUPPORTED_REQUEST 0x0010 > +#define PCIM_AER_UC_ACS_VIOLATION 0x0020 > #define PCIR_AER_UC_MASK 0x08 /* Shares bits with UC_STATUS > */ > #define PCIR_AER_UC_SEVERITY 0x0c /* Shares bits with UC_STATUS > */ > #define PCIR_AER_COR_STATUS 0x10 > @@ -718,18 +718,18 @@ > #define PCIM_AER_ECRC_CHECK_ENABLE 0x0100 > #define PCIR_AER_HEADER_LOG 0x1c > #define PCIR_AER_ROOTERR_CMD 0x2c /* Only for root complex > ports */ > -#define PCIR_AER_ROOTERR_COR_ENABLE 0x0001 > -#define PCIR_AER_ROOTERR_NF_ENABLE 0x0002 > -#define PCIR_AER_ROOTERR_F_ENABLE 0x0004 > +#define PCIM_AER_ROOTERR_COR_ENABLE 0x0001 > +#define PCIM_AER_ROOTERR_NF_ENABLE 0x0002 > +#define PCIM_AER_ROOTERR_F_ENABLE 0x0004 > #define PCIR_AER_ROOTERR_STATUS 0x30 /* Only for root complex > ports */ > -#define PCIR_AER_ROOTERR_COR_ERR 0x0001 > -#define PCIR_AER_ROOTERR_MULTI_COR_ERR 0x0002 > -#define PCIR_AER_ROOTERR_UC_ERR 0x0004 > -#define PCIR_AER_ROOTERR_MULTI_UC_ERR 0x0008 > -#define PCIR_AER_ROOTERR_FIRST_UC_FATAL 0x0010 > -#define PCIR_AER_ROOTERR_NF_ERR 0x0020 > -#define PCIR_AER_ROOTERR_F_ERR 0x0040 > -#define PCIR_AER_ROOTERR_INT_MESSAGE 0xf800 > +#define PCIM_AER_ROOTERR_COR_ERR 0x0001 > +#define PCIM_AER_ROOTERR_MULTI_COR_ERR 0x0002 > +#define PCIM_AER_ROOTERR_UC_ERR 0x0004 > +#define PCIM_AER_ROOT
Re: svn commit: r212478 - head/sys/kern
On Sat, Sep 11, 2010 at 7:42 PM, Alexander Kabaev wrote:
> Author: kan
> Date: Sat Sep 11 19:42:50 2010
> New Revision: 212478
> URL: http://svn.freebsd.org/changeset/base/212478
>
> Log:
> Add missing pointer increment to sbuf_cat.
>
> Modified:
> head/sys/kern/subr_sbuf.c
>
> Modified: head/sys/kern/subr_sbuf.c
> ==
> --- head/sys/kern/subr_sbuf.c Sat Sep 11 18:55:00 2010 (r212477)
> +++ head/sys/kern/subr_sbuf.c Sat Sep 11 19:42:50 2010 (r212478)
> @@ -441,7 +441,7 @@ sbuf_cat(struct sbuf *s, const char *str
> return (-1);
>
> while (*str != '\0') {
> - sbuf_put_byte(*str, s);
> + sbuf_put_byte(*str++, s);
> if (s->s_error != 0)
> return (-1);
> }
D'oh! Yeah, this looks right. I guess the sbuf functions that ran
when I tested didn't use sbuf_cat.
Sorry,
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: r212439 - head/sys/fs/nfs
On Sun, Sep 12, 2010 at 12:41 AM, Rick Macklem wrote:
>> Then, fid_reserved is no more reserved ? Should we rename it ?
>>
>> Comment for fid_reserved about longword alignment is wrong.
>
> Well, it's actually more broken than that.
> fid_len - Most file systems set it to the size of their variant
> of the entire structure, including the Xfid_len field.
> ZFS sets it to the size of the structure - sizeof(uint16_t)
> { presumably subtracting out the size if Xfid_len? }.
> And xfs, well, it does weird stuff with it I can't figure
> out, but it is definitely not the size of the entire struct.
>
> As such, exposing fid_len above the VOP_xxx() doesn't make much sense.
> (After my commit yesterday, nothing above the VOP_VPTOFH() uses it.)
>
> Personally, I'd lean towards a generic struct fid like...
> struct fid {
> uint8_t fid_data[MAXFIDSZ];
> };
Isilon would love a generic struct like this, as to fit our filesystem
we had to make such a change locally anyways. :-)
Cheers,
matthew
> with MAXFIDSZ increased appropriately, but this will require changes
> to xfs and zfs, since they both set the generic fid_len.
>
> If you go with...
> struct fid {
> uint16_t fid_len;
> uint8_t fid_data[MAXFIDSZ];
> };
> then the hash functions in the two NFS servers need to be changed
> (they assume 32bit alignment of fid_data), but they should be fixed
> anyhow, since they mostly hash to 0 for ZFS at this time. (From what
> I see ZFS file handles looking like.)
>
> Or, you could just rename fid_reserved to fid_pad and not worry about it.
>
> Maybe the ZFS folks could decide what they would prefer? rick
>
___
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: r212964 - head/sys/kern
On Tue, Sep 21, 2010 at 8:07 AM, Andriy Gapon wrote:
> Author: avg
> Date: Tue Sep 21 15:07:44 2010
> New Revision: 212964
> URL: http://svn.freebsd.org/changeset/base/212964
>
> Log:
> kdb_backtrace: stack(9)-based code to print backtrace without any backend
>
> The idea is to add KDB and KDB_TRACE options to GENERIC kernels on
> stable branches, so that at least the minimal information is produced
> for non-specific panics like traps on page faults.
> The GENERICs in stable branches seem to already include STACK option.
>
> Reviewed by: attilio
> MFC after: 2 weeks
>
> Modified:
> head/sys/kern/subr_kdb.c
>
> Modified: head/sys/kern/subr_kdb.c
> ==
> --- head/sys/kern/subr_kdb.c Tue Sep 21 12:57:43 2010 (r212963)
> +++ head/sys/kern/subr_kdb.c Tue Sep 21 15:07:44 2010 (r212964)
> @@ -28,6 +28,7 @@
> __FBSDID("$FreeBSD$");
>
> #include "opt_kdb.h"
> +#include "opt_stack.h"
>
> #include
> #include
> @@ -37,6 +38,7 @@ __FBSDID("$FreeBSD$");
> #include
> #include
> #include
> +#include
> #include
>
> #include
> @@ -300,6 +302,15 @@ kdb_backtrace(void)
> printf("KDB: stack backtrace:\n");
> kdb_dbbe->dbbe_trace();
> }
> +#ifdef STACK
> + else {
> + struct stack st;
> +
> + printf("KDB: stack backtrace:\n");
> + stack_save(&st);
> + stack_print(&st);
I'd recommend using stack_print_ddb(), as that avoids any locking
which may hang depending on how the kernel panic'd.
Thanks,
matthew
> + }
> +#endif
> }
>
> /*
>
___
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: r212964 - head/sys/kern
On Tue, Sep 21, 2010 at 8:40 AM, Andriy Gapon wrote: > on 21/09/2010 18:27 Andriy Gapon said the following: >> on 21/09/2010 18:17 m...@freebsd.org said the following: >>> >>> I'd recommend using stack_print_ddb(), as that avoids any locking >>> which may hang depending on how the kernel panic'd. >> >> It seems that stack_print_ddb() depends on DDB? > > But the point about locking is very good. > How do you suggest we can deal with it? > > A dirty hack would be to check panicstr in linker_search_symbol_name and avoid > locking, but I don't like that at all. > Perhaps, some code in subr_stack.c could be taken outside DDB ifdef? I keep forgetting, but actually _mtx_lock_sleep() will just return if panicstr is set. _mtx_assert() is similarly guarded, so actually it should be mostly okay. 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: r212964 - head/sys/kern
On Tue, Sep 21, 2010 at 9:36 AM, Andriy Gapon wrote: > on 21/09/2010 19:31 m...@freebsd.org said the following: >> I keep forgetting, but actually _mtx_lock_sleep() will just return if >> panicstr is set. _mtx_assert() is similarly guarded, so actually it >> should be mostly okay. > > But this doesn't seem to be the case for sx lock which is wrapped under > KLD_LOCK() ? Right, that was it. I knew I hit a problem on 7 in the past. The right thing is probably to guard the sx_[sx]lock_hard primitives and sx_assert in the same way. 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: r212964 - head/sys/kern
On Tue, Sep 21, 2010 at 9:50 AM, John Baldwin wrote: > On Tuesday, September 21, 2010 12:31:01 pm m...@freebsd.org wrote: >> On Tue, Sep 21, 2010 at 8:40 AM, Andriy Gapon wrote: >> > on 21/09/2010 18:27 Andriy Gapon said the following: >> >> on 21/09/2010 18:17 m...@freebsd.org said the following: >> >>> >> >>> I'd recommend using stack_print_ddb(), as that avoids any locking >> >>> which may hang depending on how the kernel panic'd. >> >> >> >> It seems that stack_print_ddb() depends on DDB? >> > >> > But the point about locking is very good. >> > How do you suggest we can deal with it? >> > >> > A dirty hack would be to check panicstr in linker_search_symbol_name and >> > avoid >> > locking, but I don't like that at all. >> > Perhaps, some code in subr_stack.c could be taken outside DDB ifdef? >> >> I keep forgetting, but actually _mtx_lock_sleep() will just return if >> panicstr is set. _mtx_assert() is similarly guarded, so actually it >> should be mostly okay. > > Err, I don't think _mtx_lock_sleep() is guarded in that fashion? I have an > old patch to do that but have never committed it. If we want that we should > probably change rwlocks and sxlocks to have also not block when panicstr is > set. D'oh! Once again I was looking at Isilon source but not CURRENT. We had patches for mtx (back on 6.1) and haven't updated them for sx/rw for 7. We're also running with local patches to stop CPUs in panic() that Mr Jacob has a copy of. Regarding the original issue, then, I think in the short term using a safe stack_print() would be the correct thing. Changing the locking and stop_cpus logic will not happen in the next week. :-) Thanks, matthew > > --- //depot/vendor/freebsd/src/sys/kern/kern_mutex.c 2010-05-11 > 18:31:25.0 > +++ //depot/projects/smpng/sys/kern/kern_mutex.c 2010-06-01 > 20:12:02.0 > @@ -348,6 +348,15 @@ > return; > } > > + /* > + * If we have already panic'd and this is the thread that called > + * panic(), then don't block on any mutexes but silently succeed. > + * Otherwise, the kernel will deadlock since the scheduler isn't > + * going to run the thread that holds the lock we need. > + */ > + if (panicstr != NULL && curthread->td_flags & TDF_INPANIC) > + return; > + > lock_profile_obtain_lock_failed(&m->lock_object, > &contested, &waittime); > if (LOCK_LOG_TEST(&m->lock_object, opts)) > @@ -664,6 +673,15 @@ > } > > /* > + * If we failed to unlock this lock and we are a thread that has > + * called panic(), it may be due to the bypass in _mtx_lock_sleep() > + * above. In that case, just return and leave the lock alone to > + * avoid changing the state. > + */ > + if (panicstr != NULL && curthread->td_flags & TDF_INPANIC) > + return; > + > + /* > * We have to lock the chain before the turnstile so this turnstile > * can be removed from the hash list if it is empty. > */ > > -- > John Baldwin > ___ 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: r213305 - in head/sys: gdb kern sys
On Thu, Sep 30, 2010 at 10:05 AM, Andriy Gapon wrote:
> Author: avg
> Date: Thu Sep 30 17:05:23 2010
> New Revision: 213305
> URL: http://svn.freebsd.org/changeset/base/213305
>
> Log:
> there must be only one SYSINIT with SI_SUB_RUN_SCHEDULER+SI_ORDER_ANY order
>
> SI_SUB_RUN_SCHEDULER+SI_ORDER_ANY should only be used to call
> scheduler() function which turns the initial thread into swapper proper
> and thus there is no further SYSINIT processing.
Does this imply that scheduler() shouldn't be called from a sysinit at
all, and instead a hand-call after processing all the boot-time
sysinit's would make more sense? This prevents the bug from
reoccuring, and also prevents bugs with adding a SYSINIT that runs at
SI_SUB_RUN_SCHEDULER + 1 time.
Thanks,
matthew
> Other SYSINITs with SI_SUB_RUN_SCHEDULER+SI_ORDER_ANY may get ordered
> after scheduler() and thus never executed. That particular relative
> order is semi-arbitrary.
>
> Thus, change such places to use SI_ORDER_MIDDLE.
> Also, use SI_ORDER_MIDDLE instead of correct, but less appealing,
> SI_ORDER_ANY - 1.
>
> MFC after: 1 week
>
> Modified:
> head/sys/gdb/gdb_cons.c
> head/sys/kern/kern_ntptime.c
> head/sys/sys/sched.h
>
> Modified: head/sys/gdb/gdb_cons.c
> ==
> --- head/sys/gdb/gdb_cons.c Thu Sep 30 16:47:01 2010 (r213304)
> +++ head/sys/gdb/gdb_cons.c Thu Sep 30 17:05:23 2010 (r213305)
> @@ -126,7 +126,7 @@ oktousecallout(void *data __unused)
> {
> calloutok = 1;
> }
> -SYSINIT(gdbhack, SI_SUB_RUN_SCHEDULER, SI_ORDER_ANY, oktousecallout, NULL);
> +SYSINIT(gdbhack, SI_SUB_RUN_SCHEDULER, SI_ORDER_MIDDLE, oktousecallout,
> NULL);
>
> static void
> gdb_cnputc(struct consdev *cp, int c)
>
> Modified: head/sys/kern/kern_ntptime.c
> ==
> --- head/sys/kern/kern_ntptime.c Thu Sep 30 16:47:01 2010
> (r213304)
> +++ head/sys/kern/kern_ntptime.c Thu Sep 30 17:05:23 2010
> (r213305)
> @@ -1035,5 +1035,5 @@ start_periodic_resettodr(void *arg __unu
> periodic_resettodr, NULL);
> }
>
> -SYSINIT(periodic_resettodr, SI_SUB_RUN_SCHEDULER, SI_ORDER_ANY - 1,
> +SYSINIT(periodic_resettodr, SI_SUB_RUN_SCHEDULER, SI_ORDER_MIDDLE,
> start_periodic_resettodr, NULL);
>
> Modified: head/sys/sys/sched.h
> ==
> --- head/sys/sys/sched.h Thu Sep 30 16:47:01 2010 (r213304)
> +++ head/sys/sys/sched.h Thu Sep 30 17:05:23 2010 (r213305)
> @@ -173,7 +173,7 @@ static void name ## _add_proc(void *dumm
> #name, CTLTYPE_LONG|CTLFLAG_RD|CTLFLAG_MPSAFE, \
> ptr, 0, sysctl_dpcpu_long, "LU", descr); \
> } \
> -SYSINIT(name, SI_SUB_RUN_SCHEDULER, SI_ORDER_ANY, name ## _add_proc, NULL);
> +SYSINIT(name, SI_SUB_RUN_SCHEDULER, SI_ORDER_MIDDLE, name ## _add_proc,
> NULL);
>
> #define SCHED_STAT_DEFINE(name, descr)
> \
> DPCPU_DEFINE(unsigned long, name); \
>
___
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: r213322 - head/sys/kern
On Fri, Oct 1, 2010 at 9:34 AM, Andriy Gapon wrote:
> Author: avg
> Date: Fri Oct 1 09:34:41 2010
> New Revision: 213322
> URL: http://svn.freebsd.org/changeset/base/213322
>
> Log:
> sysctls in kern_shutdown: add twin tunables
>
> also make couple of sysctl-controlled variables static
>
> Reviewed by: rwatson
> MFC after: 1 week
>
> Modified:
> head/sys/kern/kern_shutdown.c
>
> Modified: head/sys/kern/kern_shutdown.c
> ==
> --- head/sys/kern/kern_shutdown.c Fri Oct 1 09:18:30 2010
> (r213321)
> +++ head/sys/kern/kern_shutdown.c Fri Oct 1 09:34:41 2010
> (r213322)
> @@ -98,21 +98,24 @@ int debugger_on_panic = 0;
> #else
> int debugger_on_panic = 1;
> #endif
> -SYSCTL_INT(_debug, OID_AUTO, debugger_on_panic, CTLFLAG_RW,
> +SYSCTL_INT(_debug, OID_AUTO, debugger_on_panic, CTLFLAG_RW | CTLFLAG_TUN,
> &debugger_on_panic, 0, "Run debugger on kernel panic");
I thought CTLFLAG_TUN was only used to provide a more useful error
message when writing to a read-only sysctl? I think the CTLFLAG_TUN
should not be here for a RW sysctl.
Thanks,
matthew
> +TUNABLE_INT("debug.debugger_on_panic", &debugger_on_panic);
>
> #ifdef KDB_TRACE
> -int trace_on_panic = 1;
> +static int trace_on_panic = 1;
> #else
> -int trace_on_panic = 0;
> +static int trace_on_panic = 0;
> #endif
> -SYSCTL_INT(_debug, OID_AUTO, trace_on_panic, CTLFLAG_RW,
> +SYSCTL_INT(_debug, OID_AUTO, trace_on_panic, CTLFLAG_RW | CTLFLAG_TUN,
> &trace_on_panic, 0, "Print stack trace on kernel panic");
> +TUNABLE_INT("debug.trace_on_panic", &trace_on_panic);
> #endif /* KDB */
>
> -int sync_on_panic = 0;
> -SYSCTL_INT(_kern, OID_AUTO, sync_on_panic, CTLFLAG_RW,
> +static int sync_on_panic = 0;
> +SYSCTL_INT(_kern, OID_AUTO, sync_on_panic, CTLFLAG_RW | CTLFLAG_TUN,
> &sync_on_panic, 0, "Do a sync before rebooting from a panic");
> +TUNABLE_INT("kern.sync_on_panic", &sync_on_panic);
>
> SYSCTL_NODE(_kern, OID_AUTO, shutdown, CTLFLAG_RW, 0, "Shutdown
> environment");
>
>
___
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: r216161 - in head/sys: amd64/amd64 i386/i386
On Fri, Dec 3, 2010 at 1:54 PM, Jung-uk Kim wrote:
> Author: jkim
> Date: Fri Dec 3 21:54:10 2010
> New Revision: 216161
> URL: http://svn.freebsd.org/changeset/base/216161
>
> Log:
> Explicitly initialize TSC frequency. To calibrate TSC frequency, we use
> DELAY(9) and it may use TSC in turn if TSC frequency is non-zero.
Doesn't ELF guarantee that the static data is already 0? On AIX the
kernel had to explicitly zero the non-initialized data area, but AIX
uses the a.out format. I thought FreeBSD/ELF meant that global
variables without initializers were 0 by the time the OS started
running.
Thanks,
matthew
>
> MFC after: 3 days
>
> Modified:
> head/sys/amd64/amd64/tsc.c
> head/sys/i386/i386/tsc.c
>
> Modified: head/sys/amd64/amd64/tsc.c
> ==
> --- head/sys/amd64/amd64/tsc.c Fri Dec 3 21:52:01 2010 (r216160)
> +++ head/sys/amd64/amd64/tsc.c Fri Dec 3 21:54:10 2010 (r216161)
> @@ -46,7 +46,7 @@ __FBSDID("$FreeBSD$");
>
> #include "cpufreq_if.h"
>
> -uint64_t tsc_freq;
> +uint64_t tsc_freq = 0;
> int tsc_is_broken;
> int tsc_is_invariant;
> static eventhandler_tag tsc_levels_tag, tsc_pre_tag, tsc_post_tag;
>
> Modified: head/sys/i386/i386/tsc.c
> ==
> --- head/sys/i386/i386/tsc.c Fri Dec 3 21:52:01 2010 (r216160)
> +++ head/sys/i386/i386/tsc.c Fri Dec 3 21:54:10 2010 (r216161)
> @@ -46,7 +46,7 @@ __FBSDID("$FreeBSD$");
>
> #include "cpufreq_if.h"
>
> -uint64_t tsc_freq;
> +uint64_t tsc_freq = 0;
> int tsc_is_broken;
> int tsc_is_invariant;
> u_int tsc_present;
>
___
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: r216016 - head/sys/sparc64/include
On Mon, Dec 6, 2010 at 2:07 PM, Marius Strobl wrote: [lots of snip] > With that one the kernel now survies memguard_init() but then panics > right afterwards when kmeminit() calls kmem_suballoc(): > KDB: debugger backends: ddb > KDB: current backend: ddb > Copyright (c) 1992-2010 The FreeBSD Project. > Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 > The Regents of the University of California. All rights reserved. > FreeBSD is a registered trademark of The FreeBSD Foundation. > FreeBSD 9.0-CURRENT #18 r215249:216120M: Mon Dec 6 13:27:57 CET 2010 > > mar...@v20z.zeist.de:/home/marius/co/build/head2/sparc64.sparc64/usr/home/m4 > WARNING: WITNESS option enabled, expect reduced performance. > panic: kmem_suballoc: bad status return of 3 [more snip] Shooting in the dark a little... The bad status of 3 is presumably KERN_NO_SPACE because we attempted to allocate too much space from the kernel_map. What are the actual values of vm_kmem_size, kernel_map->min_offset, kernel_map->max_offset at panic time? How much virtual space does sparc64 support (since earlier it was reported it's computed based on hardware capability, for this specific machine?) 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: r216016 - head/sys/sparc64/include
On Tue, Dec 7, 2010 at 5:41 AM, Marius Strobl wrote: > On Mon, Dec 06, 2010 at 02:30:01PM -0800, m...@freebsd.org wrote: >> On Mon, Dec 6, 2010 at 2:07 PM, Marius Strobl >> wrote: >> [lots of snip] >> >> > With that one the kernel now survies memguard_init() but then panics >> > right afterwards when kmeminit() calls kmem_suballoc(): >> > KDB: debugger backends: ddb >> > KDB: current backend: ddb >> > Copyright (c) 1992-2010 The FreeBSD Project. >> > Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994 >> > ? ? ? ?The Regents of the University of California. All rights reserved. >> > FreeBSD is a registered trademark of The FreeBSD Foundation. >> > FreeBSD 9.0-CURRENT #18 r215249:216120M: Mon Dec ?6 13:27:57 CET 2010 >> > ? >> > ?mar...@v20z.zeist.de:/home/marius/co/build/head2/sparc64.sparc64/usr/home/m4 >> > WARNING: WITNESS option enabled, expect reduced performance. >> > panic: kmem_suballoc: bad status return of 3 >> >> [more snip] >> >> Shooting in the dark a little... >> >> The bad status of 3 is presumably KERN_NO_SPACE because we attempted >> to allocate too much space from the kernel_map. What are the actual >> values of vm_kmem_size, kernel_map->min_offset, kernel_map->max_offset >> at panic time? > > vm_kmem_size=5610405888 min_offset=3221225472 max_offset=13958643712 > This is on a US3i machine with 16GB RAM. So kernel_map is from 0xC000_ to 0x3_4000_, or 0x2_8000_ bytes large. Double the vm_kmem_size is 0x2_9CD0_, which is why this is failing. This to me says that, for the moment, you need the VM_MAX_KERNEL_ADDRESS define so that memguard does not take up too much virtual space; at the moment this hardware is somewhat constrained on virtual space. Thanks, matthew > >> How much virtual space does sparc64 support (since >> earlier it was reported it's computed based on hardware capability, >> for this specific machine?) >> > > Currently, the limiting factor is that the kernel TSB is addressed > virtually, which means that the TTEs for kernel TSB itself have to be > put into locked dTLB slots taking up 1 4MB dTLB slot per 1GB. US3 > family CPUs have 16 lockable 4MB dTLB and iTLB slots, which need to > be shared between the TSB and the kernel itself though, i.e. a 9MB > kernel also takes up 3 slots (US1/2 machines have 64 dTLB and iTLB > slots but there these are also used for unlocked TTEs so we don't use > more than 32 for kernel plus TSB on these). VM_MAX_KERNEL_ADDRESS is > limited according to how many slots are available for the TSB, that's > what I was referring to previously. > Actually, US3 and later as well as SPARC64 V and later CPUs have > a feature allowing the TSB to be addressed physically, circumventing > the need to lock the TSB into the TLB, thus allowing the full 64-bit > virtual address space to be used. Implementing that is on my TODO > list but unfortunately that's not exactly straight forward and also > requires some instructions to be patched at runtime so the kernel > still works on US1/2 machines. > > Marius > > ___ 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: r216616 - in head/sys: kern sys
On Tue, Dec 21, 2010 at 8:29 AM, Matthew D Fleming wrote:
> Author: mdf
> Date: Tue Dec 21 16:29:58 2010
> New Revision: 216616
> URL: http://svn.freebsd.org/changeset/base/216616
>
> Log:
> Move the fail_point_entry definition from fail.h to kern_fail.c, which
> allows putting the enumeration constants of fail point types with the
> text string that matches them.
Forgot the other part of the change. Mark the oid as MPSAFE since it
runs using an internal lock, add __BEGIN_DECLS/__END_DECLS tag around
the function declarations, and move a #define under the guard, since
using the define makes no sense if the sysctl node is not declared.
Thanks,
matthew
>
> MFC after: 1 week
>
> Modified:
> head/sys/kern/kern_fail.c
> head/sys/sys/fail.h
>
> Modified: head/sys/kern/kern_fail.c
> ==
> --- head/sys/kern/kern_fail.c Tue Dec 21 13:45:29 2010 (r216615)
> +++ head/sys/kern/kern_fail.c Tue Dec 21 16:29:58 2010 (r216616)
> @@ -76,6 +76,43 @@ MTX_SYSINIT(g_fp_mtx, &g_fp_mtx, "fail p
> #define FP_LOCK() mtx_lock(&g_fp_mtx)
> #define FP_UNLOCK() mtx_unlock(&g_fp_mtx)
>
> +/**
> + * Failpoint types.
> + * Don't change these without changing fail_type_strings in fail.c.
> + * @ingroup failpoint_private
> + */
> +enum fail_point_t {
> + FAIL_POINT_OFF, /**< don't fail */
> + FAIL_POINT_PANIC, /**< panic */
> + FAIL_POINT_RETURN, /**< return an errorcode */
> + FAIL_POINT_BREAK, /**< break into the debugger */
> + FAIL_POINT_PRINT, /**< print a message */
> + FAIL_POINT_SLEEP, /**< sleep for some msecs */
> + FAIL_POINT_INVALID, /**< placeholder */
> +};
> +
> +static const char *fail_type_strings[] = {
> + "off",
> + "panic",
> + "return",
> + "break",
> + "print",
> + "sleep",
> +};
> +
> +/**
> + * Internal structure tracking a single term of a complete failpoint.
> + * @ingroup failpoint_private
> + */
> +struct fail_point_entry {
> + enum fail_point_t fe_type; /**< type of entry */
> + int fe_arg; /**< argument to type (e.g. return
> value) */
> + int fe_prob; /**< likelihood of firing in
> millionths */
> + int fe_count; /**< number of times to fire, 0 means
> always */
> +
> + TAILQ_ENTRY(fail_point_entry) fe_entries; /**< next entry in fail
> point */
> +};
> +
> static inline void
> fail_point_sleep(struct fail_point *fp, struct fail_point_entry *ent,
> int msecs, enum fail_point_return_code *pret)
> @@ -102,15 +139,6 @@ enum {
> PROB_DIGITS = 6, /* number of zero's in above number */
> };
>
> -static const char *fail_type_strings[] = {
> - "off",
> - "panic",
> - "return",
> - "break",
> - "print",
> - "sleep",
> -};
> -
> static char *parse_fail_point(struct fail_point_entries *, char *);
> static char *parse_term(struct fail_point_entries *, char *);
> static char *parse_number(int *out_units, int *out_decimal, char *);
>
> Modified: head/sys/sys/fail.h
> ==
> --- head/sys/sys/fail.h Tue Dec 21 13:45:29 2010 (r216615)
> +++ head/sys/sys/fail.h Tue Dec 21 16:29:58 2010 (r216616)
> @@ -39,22 +39,6 @@
> #include
> #include
>
> -
> -/**
> - * Failpoint types.
> - * Don't change these without changing fail_type_strings in fail.c.
> - * @ingroup failpoint_private
> - */
> -enum fail_point_t {
> - FAIL_POINT_OFF, /**< don't fail */
> - FAIL_POINT_PANIC, /**< panic */
> - FAIL_POINT_RETURN, /**< return an errorcode */
> - FAIL_POINT_BREAK, /**< break into the debugger */
> - FAIL_POINT_PRINT, /**< print a message */
> - FAIL_POINT_SLEEP, /**< sleep for some msecs */
> - FAIL_POINT_INVALID, /**< placeholder */
> -};
> -
> /**
> * Failpoint return codes, used internally.
> * @ingroup failpoint_private
> @@ -65,6 +49,7 @@ enum fail_point_return_code {
> FAIL_POINT_RC_QUEUED, /**< sleep_fn will be called */
> };
>
> +struct fail_point_entry;
> TAILQ_HEAD(fail_point_entries, fail_point_entry);
> /**
> * Internal failpoint structure, tracking all the current details of t
Re: svn commit: r216616 - in head/sys: kern sys
On Tue, Dec 21, 2010 at 8:52 AM, Stefan Farfeleder wrote:
> On Tue, Dec 21, 2010 at 04:29:58PM +, Matthew D Fleming wrote:
>> Author: mdf
>> Date: Tue Dec 21 16:29:58 2010
>> New Revision: 216616
>> URL: http://svn.freebsd.org/changeset/base/216616
>>
>> Log:
>> Move the fail_point_entry definition from fail.h to kern_fail.c, which
>> allows putting the enumeration constants of fail point types with the
>> text string that matches them.
>
> [snip]
>
>> +enum fail_point_t {
>> + FAIL_POINT_OFF, /**< don't fail */
>> + FAIL_POINT_PANIC, /**< panic */
>> + FAIL_POINT_RETURN, /**< return an errorcode */
>> + FAIL_POINT_BREAK, /**< break into the debugger */
>> + FAIL_POINT_PRINT, /**< print a message */
>> + FAIL_POINT_SLEEP, /**< sleep for some msecs */
>> + FAIL_POINT_INVALID, /**< placeholder */
>> +};
>> +
>> +static const char *fail_type_strings[] = {
>> + "off",
>> + "panic",
>> + "return",
>> + "break",
>> + "print",
>> + "sleep",
>> +};
>
> FWIW, you can also do this in C99:
>
> static const char *fail_type_strings[] = {
> [FAIL_POINT_OFF] = "off",
> };
True. In this case I also wanted to get the stuff out of the header
that was really private.
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: r217330 - head/sys/x86/x86
On Wed, Jan 12, 2011 at 1:21 PM, John Baldwin wrote: > On Wednesday, January 12, 2011 4:08:50 pm Matthew D Fleming wrote: >> Author: mdf >> Date: Wed Jan 12 21:08:49 2011 >> New Revision: 217330 >> URL: http://svn.freebsd.org/changeset/base/217330 >> >> Log: >> Fix a brain fart. Since this file is shared between i386 and amd64, a >> bus_size_t may be 32 or 64 bits. Change the bounce_zone alignment field >> to explicitly be 32 bits, as I can't really imagine a DMA device that >> needs anything close to 2GB alignment of data. > > Hmm, we do have devices with 4GB boundaries though. I think I'd prefer it if > you instead if you did this: > > #if defined(amd64) || defined(PAE) > #define SYSCTL_ADD_BUS_SIZE_T SYSCTL_ADD_UQUAD > #else > #define SYSCTL_ADD_BUS_SIZE_T SYSCTL_ADD_UINT > #endif > > and then just used SYSCTL_ADD_BUS_SIZE_T() in the code so we could let the > members in the bounce zone retain the same types passed to > bus_dma_tag_create(). But would there be a device that can't start DMA except on a 4GB boundary? I thought that's what this member was for. 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: r217330 - head/sys/x86/x86
On Wed, Jan 12, 2011 at 4:06 PM, Bruce Evans wrote: > On Wed, 12 Jan 2011, John Baldwin wrote: > >>> Log: >>> Fix a brain fart. Since this file is shared between i386 and amd64, a >>> bus_size_t may be 32 or 64 bits. Change the bounce_zone alignment field >>> to explicitly be 32 bits, as I can't really imagine a DMA device that >>> needs anything close to 2GB alignment of data. >> >> Hmm, we do have devices with 4GB boundaries though. I think I'd prefer it >> if >> you instead if you did this: >> >> #if defined(amd64) || defined(PAE) >> #define SYSCTL_ADD_BUS_SIZE_T SYSCTL_ADD_UQUAD >> #else >> #define SYSCTL_ADD_BUS_SIZE_T SYSCTL_ADD_UINT >> #endif >> >> and then just used SYSCTL_ADD_BUS_SIZE_T() in the code so we could let the >> members in the bounce zone retain the same types passed to >> bus_dma_tag_create(). > > U_LONG should work on all arches. malloc(9) still uses u_long instead > of size_t. This works for scalars even on the recently removed i386's > with 32-bit longs where u_long is larger than size_t, since larger is > a fail-safe direction. This fails for pointers. Newer parts of malloc() > and uma are broken unless u_long is the same as uintptr_t, since they > cast pointers to u_long. This direction is fail-safe too, but gcc warns > about it. In this case for PAE u_long is (theoretically) too small, because a bus_size_t is an uint64_t. > uquad_t should never be used, like unsigned long long. Similarly for > signed types. Perhaps it could be removed in sysctl interfaces first. The name SYSCTL_ADD_UQUAD is a little misleading since it's really for a uint64_t. The name could be changed, but there's already plenty of existing uses of QUAD for int64_t which aren't being changed. 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: r217369 - in head/sys: cam/scsi sys
There appear to be 330 uses of SYSCTL and QUAD on the same line in
CURRENT. This seems reasonable to change them to S64, U64 and X64 so
they correctly reflect the size they operate upon.
What do y'all think?
Thanks,
matthew
On Thu, Jan 13, 2011 at 10:20 AM, Matthew D Fleming wrote:
> Author: mdf
> Date: Thu Jan 13 18:20:33 2011
> New Revision: 217369
> URL: http://svn.freebsd.org/changeset/base/217369
>
> Log:
> Add a 64-bit hex-printed sysctl(9) since there is at least one place in
> the code that wanted it. It is named X64 rather than XQUAD since the
> quad name is a historical abomination that should not be perpetuated.
>
> Modified:
> head/sys/cam/scsi/scsi_da.c
> head/sys/sys/sysctl.h
>
> Modified: head/sys/cam/scsi/scsi_da.c
> ==
> --- head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:27 2011 (r217368)
> +++ head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:33 2011 (r217369)
> @@ -1127,9 +1127,9 @@ dasysctlinit(void *context, int pending)
> struct ccb_trans_settings_fc *fc = &cts.xport_specific.fc;
> if (fc->valid & CTS_FC_VALID_WWPN) {
> softc->wwpn = fc->wwpn;
> - SYSCTL_ADD_XLONG(&softc->sysctl_ctx,
> + SYSCTL_ADD_X64(&softc->sysctl_ctx,
> SYSCTL_CHILDREN(softc->sysctl_tree),
> - OID_AUTO, "wwpn", CTLTYPE_QUAD | CTLFLAG_RD,
> + OID_AUTO, "wwpn", CTLFLAG_RD,
> &softc->wwpn, "World Wide Port Name");
> }
> }
>
> Modified: head/sys/sys/sysctl.h
> ==
> --- head/sys/sys/sysctl.h Thu Jan 13 18:20:27 2011 (r217368)
> +++ head/sys/sys/sysctl.h Thu Jan 13 18:20:33 2011 (r217369)
> @@ -245,6 +245,8 @@ SYSCTL_ALLOWED_TYPES(ULONG, unsigned lon
> SYSCTL_ALLOWED_TYPES(XLONG, unsigned long *a; long *b; );
> SYSCTL_ALLOWED_TYPES(INT64, int64_t *a; long long *b; );
> SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a; unsigned long long *b; );
> +SYSCTL_ALLOWED_TYPES(XINT64, uint64_t *a; int64_t *b;
> + unsigned long long *c; long long *d; );
>
> #ifdef notyet
> #define SYSCTL_ADD_ASSERT_TYPE(type, ptr) \
> @@ -389,7 +391,6 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a
> SYSCTL_ADD_ASSERT_TYPE(INT64, ptr), 0, \
> sysctl_handle_quad, "Q", __DESCR(descr))
>
> -/* Oid for a quad. The pointer must be non NULL. */
> #define SYSCTL_UQUAD(parent, nbr, name, access, ptr, val, descr)
> \
> SYSCTL_ASSERT_TYPE(UINT64, ptr, parent, name); \
> SYSCTL_OID(parent, nbr, name, \
> @@ -402,6 +403,18 @@ SYSCTL_ALLOWED_TYPES(UINT64, uint64_t *a
> SYSCTL_ADD_ASSERT_TYPE(UINT64, ptr), 0, \
> sysctl_handle_quad, "QU", __DESCR(descr))
>
> +#define SYSCTL_X64(parent, nbr, name, access, ptr, val, descr) \
> + SYSCTL_ASSERT_TYPE(XINT64, ptr, parent, name); \
> + SYSCTL_OID(parent, nbr, name, \
> + CTLTYPE_QUAD | CTLFLAG_MPSAFE | (access), \
> + ptr, val, sysctl_handle_quad, "QX", descr)
> +
> +#define SYSCTL_ADD_X64(ctx, parent, nbr, name, access, ptr, descr)
> \
> + sysctl_add_oid(ctx, parent, nbr, name, \
> + CTLTYPE_QUAD | CTLFLAG_MPSAFE | (access), \
> + SYSCTL_ADD_ASSERT_TYPE(XINT64, ptr), 0, \
> + sysctl_handle_quad, "QX", __DESCR(descr))
> +
> /* Oid for an opaque object. Specified by a pointer and a length. */
> #define SYSCTL_OPAQUE(parent, nbr, name, access, ptr, len, fmt, descr) \
> SYSCTL_OID(parent, nbr, name, CTLTYPE_OPAQUE|(access), \
>
___
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: r217369 - in head/sys: cam/scsi sys
On Thu, Jan 13, 2011 at 10:50 PM, Bruce Evans wrote: > On Thu, 13 Jan 2011 m...@freebsd.org wrote: > >> There appear to be 330 uses of SYSCTL and QUAD on the same line in >> CURRENT. This seems reasonable to change them to S64, U64 and X64 so >> they correctly reflect the size they operate upon. >> >> What do y'all think? > > Now I suggest delaying this until they can be renamed to a type- generic > SYSCTL_INT() (would probably need to be spelled differently, SYSCTL_I() > say, even if SYSCTL_INT() was changed at the same time). I'm torn on this one. The compiler knows the type (unless, for SYSCTL_INT, NULL/0 is used, but that is also a compile-time check), but to interpret it requires the use of __builtin_foo which is a gcc extension and not part of standard C. Philosophically, while I like this kind of letting the compiler do the work, if you want C++ you know where to find it. 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: r217369 - in head/sys: cam/scsi sys
On Thu, Jan 13, 2011 at 9:56 PM, Bruce Evans wrote:
> On Thu, 13 Jan 2011, Matthew D Fleming wrote:
>
>> Log:
>> Add a 64-bit hex-printed sysctl(9) since there is at least one place in
>> the code that wanted it. It is named X64 rather than XQUAD since the
>> quad name is a historical abomination that should not be perpetuated.
>
> :-). It is only long long that is abominable. Both are historical.
>
> I think these X formats shouldn't exist, even as defaults. Instead,
> sysctl(8) should have option(s) to control the format. I'm used to
> typing lots of "p/x"s to get hex formatting in gdb. There is little
> reason for sysctl(8) to have finer-grained control than gdb (sysctl
> now has hard-coded defaults instead of control).
>
> Now with stricter type checking, even formats for integers are redundant.
> The CTLTYPE now always matches the type, and the format should always
> match the type. The space wasted for the format is 1 pointer plus the
> string.
There are several issues here. First, for OPAQUE types the string is
interpreted by sysctl(8) to know how to interpret the binary blob.
Second, anyone can still use SYSCTL_OID(... CTLTYPE_INT, ..., "QU")
and there's not a lot that can be done to check it. Locally I have
another patch for the various sysctl_handle_foo that asserts the
CTLTYPE_FOO matches the handler, but looking around the FreeBSD code
there's plenty of hand-rolled instances that won't pass; some of these
come from a SYSCTL_PROC setup. Perhaps we could start with a warning
message and upgrade to a KASSERT later.
Thirdly, at the moment sysctl(8) doesn't fetch the access flags and
type specifier and only looks at the string. This is also fixable.
I do agree that the incredibly limited use of XINT, XLONG, and the one
use of X64 mean that it's not a widely used feature, and can probably
be better done with a -x flag to sysctl(8). Most bitflags are still
added as SYSCTL_UINT, so one has to re-interpret the value anyways to
see which bits are set.
> Perhaps there are some remaining type errors involving the kernel type
> being signed when it should be unsigned, or vice versa. This could
> be "fixed" by mis-specifying the format with a U, or vice versa. Since
> the specification is usually done by invoking a SYSCTL*() macro, most
> such fixes, if any, would have been undone by changing the macro to
> match the type, and it would take fixing the kernel type to fix the
> userland format.
The framework is in place to fix the obvious type errors for scalar
types, but I haven't yet finished making the kernel compile cleanly in
universe mode. Until then the check is still if 0'd out.
Thanks,
matthew
>
>> Modified: head/sys/cam/scsi/scsi_da.c
>>
>> ==
>> --- head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:27 2011 (r217368)
>> +++ head/sys/cam/scsi/scsi_da.c Thu Jan 13 18:20:33 2011 (r217369)
>> @@ -1127,9 +1127,9 @@ dasysctlinit(void *context, int pending)
>> struct ccb_trans_settings_fc *fc = &cts.xport_specific.fc;
>> if (fc->valid & CTS_FC_VALID_WWPN) {
>> softc->wwpn = fc->wwpn;
>> - SYSCTL_ADD_XLONG(&softc->sysctl_ctx,
>> + SYSCTL_ADD_X64(&softc->sysctl_ctx,
>> SYSCTL_CHILDREN(softc->sysctl_tree),
>> - OID_AUTO, "wwpn", CTLTYPE_QUAD | CTLFLAG_RD,
>> + OID_AUTO, "wwpn", CTLFLAG_RD,
>> &softc->wwpn, "World Wide Port Name");
>> }
>> }
>>
>
> Hmm, forcing hex might be best for flags (but I'll ask for binary then :-)
> and for mac addresses, but not for inet4 addresses. I don't know what sort
> of address this is.
>
> Bruce
>
___
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: r217369 - in head/sys: cam/scsi sys
On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans wrote: > On Sat, 15 Jan 2011, Garrett Cooper wrote: > >> On Fri, Jan 14, 2011 at 10:27 PM, Bruce Evans >> wrote: >>> >>> On Fri, 14 Jan 2011, Garrett Cooper wrote: >>> On Fri, Jan 14, 2011 at 6:42 PM, Bruce Evans wrote: > > ... > Oops. I think sizeof() and issigned() can be used to determine the > type > well enough in functions and initialized data (do a fuller type check > if > the compiler supports it), but I don't know how to do this in static > sysctl declarations (since sizeof() can't be used in cpp expressions). Why not just create some dumb testcases that can be run at build time to determine that for you? >>> >>> Well, how? You are given SYSCTL_I(&var, ...) and have to convert this >>> to what is now in SYSCTL_INT(), using only the type of var, in hundreds >>> or thousands of files. I don't even know how to do this with a test >>> ... >>> >>> /* Only works for arithmetic types: */ >>> #define isinteger(var) ((typeof(var))0.1 == 0) >>> #define issigned(var) ((typeof(var))-1 < 0) >>> ... >> >> This is what I meant: >> >> $ cat test_warnings.c >> #include >> >> size_t x = (int) -1; >> int y = 200L; >> $ gcc -Wconversion -Wstrict-overflow -Wsign-compare -c test_warnings.c >> test_size_t.c:3: warning: negative integer implicitly converted to >> unsigned type >> test_size_t.c:4: warning: overflow in implicit constant conversion >> $ >> >> With the right CFLAGS and a few properly written tests, and a few >> make rules, you can figure out what's what pretty easily *shrugs*. > > That's a lot more parsing than seems reasonable. > > Anyway, we already depend on gnu __typeof() being avaible for the much > more central API of pcpu. All pcpu accesses on amd64 and i386 use it, > except curthread (curthread has been hacked to use a more direct asm > for compile-time efficiency). > > SYSCTL_I() works even better that I first thought. It automatically > gives support for all typedefed integral types. No SYSCTL_FOO_T()s > with messy MD ifdefs for matching up foo_t with an integral type are > needed. Instead there are even messier MI ifdefs in SYSCTL_I() :-). > But not so many. There are hundreds of typedefed types of interest, > but only 8 different integer types to map to (8/16/32/64 bits signed > and unsigned). The complication that int64_t is plain long on 64 > bit arches but long long on 32-bit arches still arises. CTLTYPE_QUAD > can be associated with int64_t on all arches, but the format for printing > these is arch-dependent. (Note that quad_t's cannot be printed using > %qd, and %qd is unusable, since %qd is just an alias for %lld, while > quad_t is an alias for int64_t and int64_t is plain long on 64-bit > arches so its format is %ld on these arches and %lld on others.) The printing is done entirely in user-space, so it's not too bad. I had figured to upcast everything to [u]intmax_t anyways, and what to cast from would just be [u]int32_t and [u]int64_t depending on reported size. Anything smaller should be copyout(9)'d as a 4 bit quantity. But, there's two factors at play here. The first is sysctl(8) which asks for the size when reading and manages it. The second is sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit kernel looks like a long to a 32-bit app. It would be simpler to expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will deal with this just fine, but that would break some uses of sysctl(2). However, I *think* it is safe to check the sysctl_req's oldidx/oldlen to see what size the user-space expects and cast a kernel long to int before SYSCTL_OUT. $WORK has gotten busy so I haven't had a chance to work on this much but I'm hopeful that the missus will watch the kids this weekend and allow me to hack. 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: r217369 - in head/sys: cam/scsi sys
On Sat, Jan 15, 2011 at 7:06 PM, Bruce Evans wrote: > On Sat, 15 Jan 2011 m...@freebsd.org wrote: >> On Sat, Jan 15, 2011 at 6:55 AM, Bruce Evans wrote: > >> The printing is done entirely in user-space, so it's not too bad. I >> had figured to upcast everything to [u]intmax_t anyways, and what to >> cast from would just be [u]int32_t and [u]int64_t depending on >> reported size. Anything smaller should be copyout(9)'d as a 4 bit >> quantity. > > I think it shouldn't be converted in the kernel. Applications using > sysctl already have to deal with fields shorter than int in structs > returned by sysctl(). They don't need to deal with integers shorter > than int only since sysctl() doesn't support such integers yet. Short > integers could still be returned as essentially themselves by packing > them into a struct with nothing else. > >> But, there's two factors at play here. The first is sysctl(8) which >> asks for the size when reading and manages it. The second is >> sysctl(2) where we have code like SCTL_MASK32 so a long on a 64-bit >> kernel looks like a long to a 32-bit app. It would be simpler to >> expose this as an 8-byte quantity on 64-bit kernel, and sysctl(8) will >> deal with this just fine, but that would break some uses of sysctl(2). > > What are the uses? I guess they are not-so-hard-coding data types as > int and long, etc., and hard-coding them as int32_t and int64_t. Then > you get an error when the kernel returns an unexpected length, but > unexpected lengths are expected for 32-bit apps on 64-bit kernels. > > BTW, short writes are still horribly broken. Suppose there is a size > mismatch causing an application tries to write 32 bits to a 64-bit > kernel variable. Then no error is detected, and the kernel variable > ois left with garbage in its top bits. The error detection last worked > with 4.4BSD sysctl, and is still documented to work: > > % [EINVAL] A non-null newp is given and its specified length > in > % newlen is too large or too small. > > I think the case where newlen is too large still works. > > I guess the bug is potentially larger for 32 bit compat applications, > but it is rarely seen for those too since only system management > applications should be writing to kernel variables and there is no > reason to run such applications in 32 bit mode. At Isilon all of userland is still 32-bit only. The reason is that for a while we supported 32-bit and 64-bit kernels in the field, and in fact for various reasons on install the 32-bit kernel always came up first and a reboot after install was forced for 64-bit capable hardware. But we needed the same userspace to work on both kernels. >> However, I *think* it is safe to check the sysctl_req's oldidx/oldlen >> to see what size the user-space expects and cast a kernel long to int >> before SYSCTL_OUT. > > Maybe if the value fits. The application might be probing for a size > that works. Then it shouldn't care what size the value is returned in, > provided the value fits. Writing is only slightly more delicate. The > application must use a size in which the value fits. Then the kernel > can expand the size if necessary. It just has to be careful to fill > the top bits and not have sign extension bugs. sysctl_handle_i() can > probably handle both directions. Lower-level sysctl code should still > return an error if there is a size mismatch. Probing for a size that works should be passing in NULL for the old ptr to get a hint, and for scalars the sie doesn't change. It would be a somewhat malformed app that probes for the size anywhere below e.g. 512 bytes or 1 page. > If a value doesn't fit, then the application will just have to detect > the error and try a larger size (or fail if it doesn't support larger > size). An example might be a 32-bit app reading 64-bit kernel pointers. > These probably have the high bits set, so they won't fit in 32-bit sizes. > But the application can easily read them using 64-bit sizes. Integers > and pointers larger than 64 bits would be more interesting, since a > compat application might not have any integer data type larger enough > to represent them. > > So I changed my mind about converting in the kernel. Just try to convert > to whatever size the application is trying to use. Don't even force >= 32 > bit sizes on applications. The proposed code does attempt to convert to whatever the app uses, but it assumes the app is using at least a 32-bit quantity. :-) More on the other thread. 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: r217748 - head/sys/netinet/cc
For sbuf use for a sysctl you can use sbuf_init_for_sysctl() which
will, instead of growing, push the current data out using SYSCTL_OUT
to a wired user buffer. There's a few examples in the vm/ code. This
can sometimes significantly simplify the code since there's no need to
worry about held mutex/rwlock anymore.
Thanks,
matthew
On Sun, Jan 23, 2011 at 5:00 AM, Lawrence Stewart wrote:
> Author: lstewart
> Date: Sun Jan 23 13:00:25 2011
> New Revision: 217748
> URL: http://svn.freebsd.org/changeset/base/217748
>
> Log:
> An sbuf configured with SBUF_AUTOEXTEND will call malloc with M_WAITOK when a
> write to the buffer causes it to overflow. We therefore can't hold the CC
> list
> rwlock over a call to sbuf_printf() for an sbuf configured with
> SBUF_AUTOEXTEND.
>
> Switch to a fixed length sbuf which should be of sufficient size except in
> the
> very unlikely event that the sysctl is being processed as one or more new
> algorithms are loaded. If that happens, we accept the race and may fail the
> sysctl gracefully if there is insufficient room to print the names of all the
> algorithms.
>
> This should address a WITNESS warning and the potential panic that would
> occur
> if the sbuf call to malloc did sleep whilst holding the CC list rwlock.
>
> Sponsored by: FreeBSD Foundation
> Reported by: Nick Hibma
> Reviewed by: bz
> MFC after: 3 weeks
> X-MFC with: r215166
>
> Modified:
> head/sys/netinet/cc/cc.c
>
> Modified: head/sys/netinet/cc/cc.c
> ==
> --- head/sys/netinet/cc/cc.c Sun Jan 23 12:44:17 2011 (r217747)
> +++ head/sys/netinet/cc/cc.c Sun Jan 23 13:00:25 2011 (r217748)
> @@ -128,20 +128,37 @@ cc_list_available(SYSCTL_HANDLER_ARGS)
> {
> struct cc_algo *algo;
> struct sbuf *s;
> - int err, first;
> + int err, first, nalgos;
>
> - err = 0;
> + err = nalgos = 0;
> first = 1;
> - s = sbuf_new(NULL, NULL, TCP_CA_NAME_MAX, SBUF_AUTOEXTEND);
> +
> + CC_LIST_RLOCK();
> + STAILQ_FOREACH(algo, &cc_list, entries) {
> + nalgos++;
> + }
> + CC_LIST_RUNLOCK();
> +
> + s = sbuf_new(NULL, NULL, nalgos * TCP_CA_NAME_MAX, SBUF_FIXEDLEN);
>
> if (s == NULL)
> return (ENOMEM);
>
> + /*
> + * It is theoretically possible for the CC list to have grown in size
> + * since the call to sbuf_new() and therefore for the sbuf to be too
> + * small. If this were to happen (incredibly unlikely), the sbuf will
> + * reach an overflow condition, sbuf_printf() will return an error and
> + * the sysctl will fail gracefully.
> + */
> CC_LIST_RLOCK();
> STAILQ_FOREACH(algo, &cc_list, entries) {
> err = sbuf_printf(s, first ? "%s" : ", %s", algo->name);
> - if (err)
> + if (err) {
> + /* Sbuf overflow condition. */
> + err = EOVERFLOW;
> break;
> + }
> first = 0;
> }
> CC_LIST_RUNLOCK();
>
___
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 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 Wed, Jan 26, 2011 at 9:55 AM, Robert N. M. Watson 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 Wed, Jan 26, 2011 at 1:10 PM, Robert N. M. Watson 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 Wed, Jan 26, 2011 at 1:19 PM, Robert N. M. Watson 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"
Re: svn commit: r218195 - in head/sys: amd64/amd64 arm/arm i386/i386 ia64/ia64 kern mips/mips powerpc/powerpc sparc64/sparc64 sun4v/sun4v sys ufs/ffs
On Thu, Feb 3, 2011 at 4:50 AM, John Baldwin wrote: > On Thursday, February 03, 2011 2:47:20 am Juli Mallett wrote: >> On Wed, Feb 2, 2011 at 08:35, Matthew D Fleming wrote: >> > Author: mdf >> > Date: Wed Feb 2 16:35:10 2011 >> > New Revision: 218195 >> > URL: http://svn.freebsd.org/changeset/base/218195 >> > >> > Log: >> > Put the general logic for being a CPU hog into a new function >> > should_yield(). Use this in various places. Encapsulate the common >> > case of check-and-yield into a new function maybe_yield(). >> > >> > Change several checks for a magic number of iterations to use >> > should_yield() instead. >> >> First off, I admittedly don't know or care very much about this area, >> but this commit stood out to me and I had a few minor concerns. >> >> I'm slightly uncomfortable with the flat namespace here. It isn't >> obvious from the names that maybe_yield() and should_yield() relate >> only to uio_yield() and not other types of yielding (from DELAY() to >> cpu_idle() to sched_yield().) The other problematic element here is >> that "maybe_yield" and "should_yield" could quite reasonably be >> variables or functions in existing code in the kernel, and although we >> don't try to protect against changes that could cause such collisions, >> we shouldn't do them gratuitously, and there's even something that >> seems aesthetically off about these; they seem...informal, even >> Linuxy. I think names like uio_should_yield() and uio_maybe_yield() >> wouldn't have nearly as much of a problem, since the context of the >> question of "should" is isolated to uio operations rather than, say, >> whether the scheduler would *like* for us, as the running thread, to >> yield, or other considerations that may be more general. > > I mostly agree, but these checks are no longer specific to uio. Matt used > them to replace many ad-hoc checks using counters with hardcoded maximums in > places like softupdates, etc. > > I don't have any good suggestions for what else you would call these. I'm not > sure 'sched_amcpuhog() or sched_hoggingcpu()' are really better (and these are > not scheduler dependent, so sched_ would probably not be a good prefix). Bruce correctly points out that the code doesn't work like I expect with PREEMPTION, which most people will be running. I'm thinking of adding a new per-thread field to record the last ticks value that a voluntary mi_switch() was done, so that there's a standard way of checking if a thread is being a hog; this will work for both PREEMPTION and !PREEMPTION, and would be appropriate for the places that previously used a counter. (This would require uio_yield() to be SW_VOL, but I can't see why it's not a voluntary context switch anyways). I'm happy to rename the functions (perhaps just yield_foo() rather than foo_yield()?) and stop using uio_yield as the base name since it's not a uio function. I wanted to keep the uio_yield symbol to preserve the KBI/KPI. Any suggestions for names are welcome. 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: r218424 - in head/sys: dev/sio kern pc98/cbus sys ufs/ffs
On Tue, Feb 8, 2011 at 1:18 AM, Kostik Belousov wrote: > On Tue, Feb 08, 2011 at 12:16:36AM +, Matthew D Fleming wrote: >> Author: mdf >> Date: Tue Feb 8 00:16:36 2011 >> New Revision: 218424 >> URL: http://svn.freebsd.org/changeset/base/218424 >> >> Log: >> Based on discussions on the svn-src mailing list, rework r218195: >> >> - entirely eliminate some calls to uio_yeild() as being unnecessary, >> such as in a sysctl handler. > Are you sure ? Please see r185983. I eliminated the uio_yield inside two sysctl handlers, not in userland_sysctl() itself. 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: r218685 - head/sys/dev/acpica
On Mon, Feb 14, 2011 at 10:33 AM, John Baldwin wrote: > On Monday, February 14, 2011 12:20:20 pm Matthew D Fleming wrote: >> Author: mdf >> Date: Mon Feb 14 17:20:20 2011 >> New Revision: 218685 >> URL: http://svn.freebsd.org/changeset/base/218685 >> >> Log: >> Prevent reading from the ACPI_RESOURCE past its actual end. For >> paranoia limit to the size of the ACPI_RESOURCE as well. > > I think in practice that len would never be > sizeof(ACPI_RESOURCE). > > You could probably get by with using a KASSERT() instead: > > KASSERT(res->Length <= sizeof(ACPI_RESOURCE), "resource too large")); > bcopy(res, req->acpi_res, res->Length); Thanks. I wanted to be paranoid since the problem was sporadic. Anyone who can better test this code should feel free to modify it further. 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: r227473 - head/sbin/geom/class/multipath
On Sun, Nov 13, 2011 at 2:46 PM, Garrett Cooper wrote:
> On Sun, Nov 13, 2011 at 2:08 PM, Pawel Jakub Dawidek wrote:
>> On Sat, Nov 12, 2011 at 12:16:23PM -0800, Garrett Cooper wrote:
>>> On Sat, Nov 12, 2011 at 12:01 PM, Alexander Motin wrote:
>>> > Author: mav
>>> > Date: Sat Nov 12 20:01:30 2011
>>> > New Revision: 227473
>>> > URL: http://svn.freebsd.org/changeset/base/227473
>>> >
>>> > Log:
>>> > Fix build on some archs after r227464.
>>> >
>>> > Modified:
>>> > head/sbin/geom/class/multipath/geom_multipath.c
>>> >
>>> > Modified: head/sbin/geom/class/multipath/geom_multipath.c
>>> > ==
>>> > --- head/sbin/geom/class/multipath/geom_multipath.c Sat Nov 12
>>> > 19:55:48 2011 (r227472)
>>> > +++ head/sbin/geom/class/multipath/geom_multipath.c Sat Nov 12
>>> > 20:01:30 2011 (r227473)
>>> > @@ -133,7 +133,8 @@ mp_label(struct gctl_req *req)
>>> > uint8_t *sector, *rsector;
>>> > char *ptr;
>>> > uuid_t uuid;
>>> > - uint32_t secsize = 0, ssize, status;
>>> > + ssize_t secsize = 0, ssize;
>>> > + uint32_t status;
>>> > const char *name, *name2, *mpname;
>>> > int error, i, nargs, fd;
>>> >
>>> > @@ -161,8 +162,8 @@ mp_label(struct gctl_req *req)
>>> > disksize = msize;
>>> > } else {
>>> > if (secsize != ssize) {
>>> > - gctl_error(req, "%s sector size %u
>>> > different.",
>>> > - name, ssize);
>>> > + gctl_error(req, "%s sector size %ju
>>> > different.",
>>> > + name, (intmax_t)ssize);
>>>
>>> Shouldn't that be uintmax_t, not intmax_t ?
>>
>> No, ssize_t is signed. Although the best would be to use %zd for
>> ssize_t.
>
> Thanks... Missed the leading s.
... except that the cast and conversion specifier aren't in sync. The
cast is signed; the conversion specifier is unsigned. %zd is still
best, since it's the conversion specifier for the variable's type.
Next best is %jd and cast to intmax_t, since the signedness is
preserved.
But in the end it doesn't matter a lot since whatever is printed in
the error message, it's likely derivable from reading the code what
the actual value used was.
Cheers,
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: r227541 - head/sys/dev/usb/controller
On Tue, Nov 15, 2011 at 12:48 PM, Hans Petter Selasky
wrote:
> Author: hselasky
> Date: Tue Nov 15 20:48:57 2011
> New Revision: 227541
> URL: http://svn.freebsd.org/changeset/base/227541
>
> Log:
> Some brands of XHCI controllers needs more time to reset.
... and since there's no guarantee that hz is 1000 or has any
particular value, most of these seem a bit spurious.
Is there some reason these functions aren't asking for a delay in
terms of milli- or microseconds, and converting to hz internally? I
would expect a delay while waiting for hardware to have a wall-clock
time, not a time relative to hz, which has no predefined range.
Thanks,
matthew
> Reported by: Jan Henrik Sylvester
> MFC after: 1 week
>
> Modified:
> head/sys/dev/usb/controller/xhci.c
>
> Modified: head/sys/dev/usb/controller/xhci.c
> ==
> --- head/sys/dev/usb/controller/xhci.c Tue Nov 15 20:41:50 2011
> (r227540)
> +++ head/sys/dev/usb/controller/xhci.c Tue Nov 15 20:48:57 2011
> (r227541)
> @@ -292,7 +292,7 @@ xhci_start_controller(struct xhci_softc
> XWRITE4(sc, oper, XHCI_USBCMD, XHCI_CMD_HCRST);
>
> for (i = 0; i != 100; i++) {
> - usb_pause_mtx(NULL, hz / 1000);
> + usb_pause_mtx(NULL, hz / 100);
> temp = XREAD4(sc, oper, XHCI_USBCMD) &
> (XHCI_CMD_HCRST | XHCI_STS_CNR);
> if (!temp)
> @@ -453,7 +453,7 @@ xhci_start_controller(struct xhci_softc
> XHCI_CMD_INTE | XHCI_CMD_HSEE);
>
> for (i = 0; i != 100; i++) {
> - usb_pause_mtx(NULL, hz / 1000);
> + usb_pause_mtx(NULL, hz / 100);
> temp = XREAD4(sc, oper, XHCI_USBSTS) & XHCI_STS_HCH;
> if (!temp)
> break;
> @@ -487,7 +487,7 @@ xhci_halt_controller(struct xhci_softc *
> XWRITE4(sc, oper, XHCI_USBCMD, 0);
>
> for (i = 0; i != 100; i++) {
> - usb_pause_mtx(NULL, hz / 1000);
> + usb_pause_mtx(NULL, hz / 100);
> temp = XREAD4(sc, oper, XHCI_USBSTS) & XHCI_STS_HCH;
> if (temp)
> break;
>
___
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: r227541 - head/sys/dev/usb/controller
On Tue, Nov 15, 2011 at 1:02 PM, Hans Petter Selasky wrote: > For USB compliant operation, the USB stack requires hz to be greater or equal > to 250 hz, to put it like that. Mostly a requirement in USB gadget/device > mode. Really? That's news to me. Is that documented somewhere? I know we still use hz=100 internally, but we're on stable/7 still so not using the new USB stack yet. 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: r227541 - head/sys/dev/usb/controller
On Tue, Nov 15, 2011 at 1:20 PM, wrote: > On Tue, Nov 15, 2011 at 1:02 PM, Hans Petter Selasky wrote: >> For USB compliant operation, the USB stack requires hz to be greater or equal >> to 250 hz, to put it like that. Mostly a requirement in USB gadget/device >> mode. > > Really? That's news to me. Is that documented somewhere? I know we > still use hz=100 internally, but we're on stable/7 still so not using > the new USB stack yet. ... and I also just remembered that I have seen recommendations that, when FreeBSD is used as a virtual machine, hz should be set to 100 so that the virtual interrupt overhead is reduced. Those two recommendations are at odds with each other. 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: r227541 - head/sys/dev/usb/controller
On Tue, Nov 15, 2011 at 1:22 PM, Hans Petter Selasky wrote:
> On Tuesday 15 November 2011 22:20:18 m...@freebsd.org wrote:
>> On Tue, Nov 15, 2011 at 1:02 PM, Hans Petter Selasky
> wrote:
>> > For USB compliant operation, the USB stack requires hz to be greater or
>> > equal to 250 hz, to put it like that. Mostly a requirement in USB
>> > gadget/device mode.
>>
>> Really? That's news to me. Is that documented somewhere? I know we
>> still use hz=100 internally, but we're on stable/7 still so not using
>> the new USB stack yet.
>
> No it is not documented anywhere. This delay is mostly critical if you enable
> USB power saving features like suspend and resume. Then there are some
> software timers which should not derive too much.
>
> Most of the time the delays in USB are not critical. Transfer timers are in
> the seconds range and that works fine with hz=100.
>
> Where and how should I document such are requirement?
>
> Add something during system init?
>
> if (hz < 250)
> printf("USB: hz is too low (ignored)\n");
I'm not sure what functions we have for detecting the OS instance is
virtualized, but something like that would be useful if it's really
important. Perhaps:
"USB: hz value less than 250 may cause functional issues"
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: r227541 - head/sys/dev/usb/controller
On Tue, Nov 15, 2011 at 1:22 PM, Hans Petter Selasky wrote: > On Tuesday 15 November 2011 22:20:18 m...@freebsd.org wrote: >> On Tue, Nov 15, 2011 at 1:02 PM, Hans Petter Selasky > wrote: >> > For USB compliant operation, the USB stack requires hz to be greater or >> > equal to 250 hz, to put it like that. Mostly a requirement in USB >> > gadget/device mode. >> >> Really? That's news to me. Is that documented somewhere? I know we >> still use hz=100 internally, but we're on stable/7 still so not using >> the new USB stack yet. > > No it is not documented anywhere. This delay is mostly critical if you enable > USB power saving features like suspend and resume. Then there are some > software timers which should not derive too much. Actually, if the bug is only with the power_save mode, then perhaps it should only be warned when that mode is enabled. 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: r227588 - in head: share/man/man9 sys/kern sys/sys
On Wed, Nov 16, 2011 at 1:51 PM, Pawel Jakub Dawidek wrote: > Author: pjd > Date: Wed Nov 16 21:51:17 2011 > New Revision: 227588 > URL: http://svn.freebsd.org/changeset/base/227588 > > Log: > Constify arguments for locking KPIs where possible. > > This enables locking consumers to pass their own structures around as const > and > be able to assert locks embedded into those structures. Thank you! This is one of many (minor) diffs I have at $WORK. Cheers, 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: r227812 - head/lib/libc/string
On Mon, Nov 21, 2011 at 6:50 PM, Eitan Adler wrote: > Author: eadler (ports committer) > Date: Tue Nov 22 02:50:24 2011 > New Revision: 227812 > URL: http://svn.freebsd.org/changeset/base/227812 > > Log: > - fix some style(9) nits with my last commit > - add a comment explaining why I used '|' instead of '||' > > Submitted by: danfe@ > Approved by: emaste@ > > Modified: > head/lib/libc/string/strcasecmp.c > head/lib/libc/string/strncmp.c > > Modified: head/lib/libc/string/strcasecmp.c > == > --- head/lib/libc/string/strcasecmp.c Tue Nov 22 02:27:59 2011 > (r227811) > +++ head/lib/libc/string/strcasecmp.c Tue Nov 22 02:50:24 2011 > (r227812) > @@ -49,7 +49,7 @@ strcasecmp_l(const char *s1, const char > *us1 = (const u_char *)s1, > *us2 = (const u_char *)s2; > if (s1 == s2) > - return (0); > + return (0); > > FIX_LOCALE(locale); > > @@ -73,8 +73,9 @@ strncasecmp_l(const char *s1, const char > *us1 = (const u_char *)s1, > *us2 = (const u_char *)s2; > > - if (( s1 == s2) | (n == 0)) > - return (0); > + /* use a bitwise or to avoid an additional branch instruction */ > + if ((s1 == s2) | (n == 0)) > + return (0); I guess I'm a little confused. Do we really have profiling information at this level that suggests the overhead of the branch is significant? I thought most hardware had pretty good branch-prediction, particularly with speculative execution. Wouldn't something like __predict_false() have more value for performance, or is all this just guess-work? I would much rather have the code say what it means unless there's real, measurable performance differences from doing otherwise. 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: r250986 - head/sys/dev/usb
On Sat, May 25, 2013 at 10:09 AM, Hans Petter Selasky
wrote:
> Author: hselasky
> Date: Sat May 25 17:09:58 2013
> New Revision: 250986
> URL: http://svnweb.freebsd.org/changeset/base/250986
>
> Log:
> Fix some statical clang analyzer warnings.
>
> Modified:
> head/sys/dev/usb/usb_device.c
> head/sys/dev/usb/usb_hub.c
> head/sys/dev/usb/usb_msctest.c
>
> Modified: head/sys/dev/usb/usb_device.c
>
> ==
> --- head/sys/dev/usb/usb_device.c Sat May 25 16:58:12 2013
> (r250985)
> +++ head/sys/dev/usb/usb_device.c Sat May 25 17:09:58 2013
> (r250986)
> @@ -799,9 +799,6 @@ usb_config_parse(struct usb_device *udev
> /* find maximum number of endpoints */
> if (ep_max < temp)
> ep_max = temp;
> -
> - /* optimalisation */
> - id = (struct usb_interface_descriptor *)ed;
> }
> }
>
>
> Modified: head/sys/dev/usb/usb_hub.c
>
> ==
> --- head/sys/dev/usb/usb_hub.c Sat May 25 16:58:12 2013(r250985)
> +++ head/sys/dev/usb/usb_hub.c Sat May 25 17:09:58 2013(r250986)
> @@ -341,7 +341,6 @@ uhub_reattach_port(struct uhub_softc *sc
>
> DPRINTF("reattaching port %d\n", portno);
>
> - err = 0;
> timeout = 0;
> udev = sc->sc_udev;
> child = usb_bus_port_get_device(udev->bus,
>
> Modified: head/sys/dev/usb/usb_msctest.c
>
> ==
> --- head/sys/dev/usb/usb_msctest.c Sat May 25 16:58:12 2013
> (r250985)
> +++ head/sys/dev/usb/usb_msctest.c Sat May 25 17:09:58 2013
> (r250986)
> @@ -802,7 +802,6 @@ usb_msc_eject(struct usb_device *udev, u
> if (sc == NULL)
> return (USB_ERR_INVAL);
>
> - err = 0;
> switch (method) {
> case MSC_EJECT_STOPUNIT:
> err = bbb_command_start(sc, DIR_IN, 0, NULL, 0,
> @@ -844,6 +843,7 @@ usb_msc_eject(struct usb_device *udev, u
> break;
> default:
> DPRINTF("Unknown eject method (%d)\n", method);
> + err = 0;
> break;
> }
> DPRINTF("Eject CD command status: %s\n", usbd_errstr(err));
>
I don't know about the top one, but the bottom two are the safer way to
code, and should not have been changed. Unless we feel guaranteed the
compiler can detect all uninitialized use and will break the build, an
early initialization to a sane value is absolutely the right thing to do,
even if it will be overwritten. If the compiler feels sure the
initialization isn't needed, it does not have to emit the code. But any
coding change after the (missing) initialization can create a bug now
(well, it depends on how the code is structured, but from the three lines
of context svn diff provides it's not clear a bug couldn't easily be
introduced).
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: r252032 - head/sys/amd64/include
[snipping everything about counter64, atomic ops, cycles, etc.] I wonder if the idea explained in this paper: http://static.usenix.org/event/usenix03/tech/freenix03/full_papers/mcgarry/mcgarry_html/ Which seems to be used in FreeBSD for some ARM atomics: http://svnweb.freebsd.org/base/head/sys/arm/include/atomic.h?view=annotate , look for ARM_RAS_START would be more efficient. To summarize: one marks a section of code such that if a thread is interrupted during the code it restarts at the beginning instead of where it was interrupted. This has been used to implement atomic increment on some hardware without the necessary instructions. Here it could be used to implement atomic increment on the per-cpu counter without the overhead of an atomic instruction. It's multiple stores to mark the section of code doing the increment, but they're all per-cpu or per thread. That may be cheaper than an atomic increment, at least on 32-bit platforms that are doing an atomic 64-bit increment. I haven't benchmarked this (ENOTIME, plus I'm on vacation right now), but using restartable sections requires three stores (add an item to a linked list, 64-bit increment for the counter, remove an item from a linked list). Some of the penalty is payed in the context switch code, which has to check if the instruction pointer is in one of these critical sections. I haven't checked to see if this code is enabled on all architectures or just ARM. But if context switches are less frequent than counter increments in the networking code, it's still a win for most uses. 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: r252376 - head/lib/libutil
On Sat, Jun 29, 2013 at 9:36 AM, Tim Kientzle wrote: > > On Jun 29, 2013, at 9:19 AM, Konstantin Belousov wrote: > > > On Sat, Jun 29, 2013 at 03:52:49PM +, Tim Kientzle wrote: > >> Author: kientzle > >> Date: Sat Jun 29 15:52:48 2013 > >> New Revision: 252376 > >> URL: http://svnweb.freebsd.org/changeset/base/252376 > >> > >> Log: > >> Fix -Wunsequenced warning > > What is this ? From the name of the warning, it sounds as if the problem > > is in the lack of sequence point between two modifications of the same > > variable in the expression ? > > > > But, there function' argument evaluation and function call are separated > > by seq point, AFAIR. Could you, please, clarify ? > > I think you're right about that, though I'd have to > look at the spec to be sure. > > Not sure why clang would report this as a -Wunsequenced > warning. The implied store here is certainly redundant, though. > It may be like other warnings (-Wmissing-field-initializers, I'm looking at you) that warn about currently correct, but potentially problematic behavior. In particular, if any of the functions is re-implemented as a macro, the sequence point goes away, and this code is broken without the code's author having made any changes. So it seems like a reasonable warning. 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: r235797 - head/contrib/gcc
On Tue, May 22, 2012 at 1:05 PM, Bruce Evans wrote: > On Tue, 22 May 2012, David E. O'Brien wrote: > >> Log: >> Do not incorrectly warn when printing a quad_t using "%qd" on 64-bit >> platforms. > > > I think I like this, since it is technically correct, and will find a > different set of type mismatches. We run with the following at Isilon, which is somewhat bogus because it allows a bit of sloppiness in types, but is also terribly convenient since it means no casting on printf arguments is needed: --- contrib/gcc/c-format.c 2012-05-22 14:08:23.538266746 -0700 +++ /data/sb/head/src/contrib/gcc/c-format.c2012-05-16 12:59:40.937016702 -0700 @@ -2298,10 +2570,20 @@ check_format_types (format_wanted_type * equivalent but the above test won't consider them equivalent. */ if (wanted_type == char_type_node && (!pedantic || i < 2) && char_type_flag) continue; + + /* Isilon: FreeBSD defines int64_t (and others) as one type (e.g. long +long) on i386 and another type (e.g. long) on amd64. This prevents +the use of a common format specifier. Treat equal sized integer types +as equivalent. */ + if (TREE_CODE (wanted_type) == INTEGER_TYPE + && TREE_CODE (cur_type) == INTEGER_TYPE + && int_size_in_bytes (wanted_type) == int_size_in_bytes (cur_type)) +continue; + /* Now we have a type mismatch. */ format_type_warning (types->name, format_start, format_length, wanted_type, types->pointer_count, types->wanted_type_name, orig_cur_type, arg_num); } If there's no objections, I (or David or anyone else) can commit to the FreeBSD repository. Cheers, 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: r236380 - head/sys/vm
On Fri, Jun 1, 2012 at 2:14 AM, Bruce Evans wrote: >>> +SYSCTL_OID(_vm, OID_AUTO, swap_free, >>> CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE, >>> + NULL, 0, sysctl_vm_swap_free, "Q", >>> + "Blocks of free swap storage."); > > > Bug 9 is a style bug. I didn't even know that the raw SYSCTL_OID() could > be misused like this. The normal SYSCTL_PROC() is identical with > SYSCTL_OID() except it checks that the access flags are not 0. Few or no > SYSCTL_FOO()s have no access flags, and this is not one. It has rather > excessive access flags (I think CTLFLAG_MPSAFE is unnecessary. I wanted to correct this one point. CTLFLAG_MPSAFE is helpful, because its use prevents kern_sysctl from taking Giant before calling the sysctl handler. It's probably nearing the case, now, that any sysctl *without* CTLFLAG_MPSAFE is incorrect, except perhaps for a few that set/get text strings that don't want to roll their own serialization. Cheers, 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: r240549 - head/sys/arm/tegra
On Sun, Sep 16, 2012 at 2:53 AM, Alexey Dokuchaev wrote: > On Sun, Sep 16, 2012 at 10:37:56AM +0100, Chris Rees wrote: >> On 16 September 2012 10:28, Alexey Dokuchaev wrote: >> > I thought preferred and more style(9) compliant way to code [empty >> > endless loop] is: >> > >> > for (;;) >> > continue; >> >> Actually: >> >> for (;;) >> ; > > Explicit "continue" is supposed to tell reviewer that original author did > not make a typo, but indeed knew what he was doing. Lonely semicolon is too > ambiguous in this case. The semicolon being on its own line is not ambiguous, and is good style everywhere. A semicolon on the same line as a while or for is a red flag that it may be unintentional. Cheers, 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: r241373 - head/lib/libc/stdlib
On Tue, Oct 9, 2012 at 10:16 AM, David Chisnall wrote: > On 9 Oct 2012, at 17:33, Andrey Chernov wrote: > >> Do you check assembler output for _both_ cases? >> In my testing clang and gcc xor's 'junk' properly in case it have >> 'volatile' keyword (as in srandomdev()) and elide it without 'volatile'. >> IMHO this change should be backed out for srandomdev() and adding >> 'volatile' for sranddev() instead. > > In it's original form, it is very dangerous - the whole expression reduces to > undefined and so the LLVM IR for the call is: > > call void @srand(i32 undef) > > The back end is then free to use any value for the call argument, including > any register value or 0. Since the value is passed in a register, it will > probably just use whatever the last value there is, which may or may not be > anything sensible. On MIPS, for example, this is most likely to be &tv, and > so is 100% deterministic. > > Adding the volatile means that we are doing an XOR with a value left on the > stack. If this is early on in the application, then it is most likely to be > 0. If it's later on, then there may be a value here, but it's still not very > likely to be something particularly unpredictable. > The original behavior can be recovered by using inline assembly to fetch the value from a register into a local C variable; this would at least not rely on undefined behavior. But I agree it's of dubious value anyways. ___ 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: r241471 - head/share/man/man9
On Thu, Oct 11, 2012 at 9:55 PM, Gleb Smirnoff wrote:
> On Fri, Oct 12, 2012 at 01:31:03AM +, Kevin Lo wrote:
> K> Author: kevlo
> K> Date: Fri Oct 12 01:31:02 2012
> K> New Revision: 241471
> K> URL: http://svn.freebsd.org/changeset/base/241471
> K>
> K> Log:
> K> Since the moduledata structure member priv is a void pointer, using
> K> NULL instead of 0 when dealing with pointers.
> K>
> K> Modified:
> K> head/share/man/man9/module.9
> K>
> K> Modified: head/share/man/man9/module.9
> K>
> ==
> K> --- head/share/man/man9/module.9 Thu Oct 11 23:41:18 2012
> (r241470)
> K> +++ head/share/man/man9/module.9 Fri Oct 12 01:31:02 2012
> (r241471)
> K> @@ -99,7 +99,7 @@ static int foo_handler(module_t mod, int
> K> static moduledata_t mod_data= {
> K> "foo",
> K> foo_handler,
> K> -0
> K> +NULL
> K> };
> K>
> K> MODULE_VERSION(foo, 1);
>
> I think we should provide C99 sparse initializers for structures in
> all manpages in section 9, as well as use only such initializers in any new
> code added to tree.
For man pages and .c files, that'd be fine. But since it's still
possible to build C++ kernel modules, header files can't do this since
named initializers don't have the same syntax in C++ (unless they
fixed this in C++11?)
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: r243134 - head/sys/sys
On Thu, Nov 15, 2012 at 10:25 PM, Konstantin Belousov wrote: > Author: kib > Date: Fri Nov 16 06:25:20 2012 > New Revision: 243134 > URL: http://svnweb.freebsd.org/changeset/base/243134 > > Log: > Alphabetically reorder the forward-declarations of the structures. > Add the declaration for enum idtype, to be used later. Forward declarations of enums isn't an ISO C feature, but a gcc extension. While the kernel uses many gcc extensions, it hides most under a #define so unsupported compilers can continue along. This unsupported feature can't be hidden. Does the forward declaration prevent another warning? Thanks, matthew > Reported and reviewed by: bde > MFC after:28 days > > Modified: > head/sys/sys/syscallsubr.h > > Modified: head/sys/sys/syscallsubr.h > == > --- head/sys/sys/syscallsubr.h Fri Nov 16 06:22:14 2012(r243133) > +++ head/sys/sys/syscallsubr.h Fri Nov 16 06:25:20 2012(r243134) > @@ -35,25 +35,26 @@ > #include > > struct file; > +enum idtype; > struct itimerval; > struct image_args; > struct jail; > +struct kevent; > +struct kevent_copyops; > +struct kld_file_stat; > +struct ksiginfo; > struct mbuf; > struct msghdr; > struct msqid_ds; > +struct ogetdirentries_args; > struct rlimit; > struct rusage; > -struct __wrusage; > union semun; > +struct sendfile_args; > struct sockaddr; > struct stat; > -struct kevent; > -struct kevent_copyops; > -struct kld_file_stat; > -struct ksiginfo; > -struct sendfile_args; > struct thr_param; > -struct ogetdirentries_args; > +struct __wrusage; > > intkern___getcwd(struct thread *td, u_char *buf, enum uio_seg bufseg, > u_int buflen); ___ 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: r244112 - head/sys/kern
On Thu, Dec 13, 2012 at 1:42 AM, Andriy Gapon wrote: > on 13/12/2012 09:16 Adrian Chadd said the following: >> Hi, >> >> I think the fundamental problem here is we have some pretty different >> ideas of what KASSERT should be, versus what it actually is in various >> parts of the code. > > Oh, and another part of the problem is that the discussion is opinion based. > But it didn't have to be. > > Compare this: > We think that feature F is a very good idea, we think that it will be used by > many > people and it will provide a lot of benefits. So here you are - the code is > in > the tree. > > To this: > We have been using feature F, it has proved to be a very good idea as it > provided > these benefits and spared us from these problems. So here you are - the code > is > in the tree. > > If I have a differing opinion in the first case I usually state it (and can be > pulled into an argument about it). If I have a different opinion in the > second > case, I try to adjust my opinion to the stated reality. > >> Since we're lost in semantics, we're not going to get any further on >> this discussion just for now, so let's take a break and think about >> other things for now. Tools, not policy. A non-panic-ing KASSERT is a tool, not enabled by default. You don't need to use it. Someone does, so why can't we provide the tool? 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: r238502 - in head/sys: kern vm
On Sun, Jul 15, 2012 at 1:29 PM, Matthew D Fleming wrote: > Author: mdf > Date: Sun Jul 15 20:29:48 2012 > New Revision: 238502 > URL: http://svn.freebsd.org/changeset/base/238502 > > Log: > Fix a bug with memguard(9) on 32-bit architectures without a > VM_KMEM_MAX_SIZE. > > The code was not taking into account the size of the kernel_map, which > the kmem_map is allocated from, so it could produce a sub-map size too > large to fit. The simplest solution is to ignore VM_KMEM_MAX entirely > and base the memguard map's size off the kernel_map's size, since this > is always relevant and always smaller. > > Found by: Justin Hibbits MFC after: 3 days 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: r240067 - head/sys/kern
On Mon, Sep 3, 2012 at 1:52 AM, Aleksandr Rybalko wrote:
> Author: ray
> Date: Mon Sep 3 08:52:05 2012
> New Revision: 240067
> URL: http://svn.freebsd.org/changeset/base/240067
>
> Log:
> Add kern.hintmode sysctl variable to show current state of hints:
> 0 - loader hints in environment only;
> 1 - static hints only
> 2 - fallback mode (Dynamic KENV with fallback to kernel environment)
> Add kern.hintmode write handler, accept only value 2. That will switch
> static KENV to dynamic. So it will be possible to change device hints.
>
> Approved by: adrian (mentor)
>
> Modified:
> head/sys/kern/subr_hints.c
>
> Modified: head/sys/kern/subr_hints.c
> ==
> --- head/sys/kern/subr_hints.c Mon Sep 3 07:18:24 2012(r240066)
> +++ head/sys/kern/subr_hints.c Mon Sep 3 08:52:05 2012(r240067)
> @@ -29,8 +29,10 @@ __FBSDID("$FreeBSD$");
>
> #include
> #include
> +#include
> #include
> #include
> +#include
Putting on my style-nazi hat. sysctl comes before systm alphabetically.
> #include
>
> /*
> @@ -42,6 +44,81 @@ static int use_kenv;
> static char *hintp;
>
> /*
> + * Define kern.hintmode sysctl, which only accept value 2, that cause to
> + * switch from Static KENV mode to Dynamic KENV. So systems that have hints
> + * compiled into kernel will be able to see/modify KENV (and hints too).
> + */
> +
> +static int
> +sysctl_hintmode(SYSCTL_HANDLER_ARGS)
> +{
> + int error, i, from_kenv, value, eqidx;
> + const char *cp;
> + char *line, *eq;
These are not sorted properly; pointers come before scalars, and
within the group they should be sorted alphabetically; e.g. "eqidx"
comes before all the other ints.
> +
> + from_kenv = 0;
> + cp = kern_envp;
> + value = hintmode;
> +
> + /* Fetch candidate for new hintmode value */
> + error = sysctl_handle_int(oidp, &value, 0, req);
> + if (error || !req->newptr)
This may be copying existing code, but style(9) explicitly forbids
using '!' except on boolean expressions. Since req->newptr is a
pointer, an explicit comparison to NULL should be made. This error is
repeated later.
Thanks,
matthew
> + return (error);
> +
> + if (value != 2)
> + /* Only accept swithing to hintmode 2 */
> + return (EINVAL);
> +
> + /* Migrate from static to dynamic hints */
> + switch (hintmode) {
> + case 0:
> + if (dynamic_kenv)
> + /* Already here */
> + hintmode = value; /* XXX: Need we switch or not ? */
> + return (0);
> + from_kenv = 1;
> + cp = kern_envp;
> + break;
> + case 1:
> + cp = static_hints;
> + break;
> + case 2:
> + /* Nothing to do, hintmode already 2 */
> + return (0);
> + }
> +
> + while (cp) {
> + i = strlen(cp);
> + if (i == 0)
> + break;
> + if (from_kenv) {
> + if (strncmp(cp, "hint.", 5) != 0)
> + /* kenv can have not only hints */
> + continue;
> + }
> + eq = strchr(cp, '=');
> + if (!eq)
> + /* Bad hint value */
> + continue;
> + eqidx = eq - cp;
> +
> + line = malloc(i+1, M_TEMP, M_WAITOK);
> + strcpy(line, cp);
> + line[eqidx] = '\0';
> + setenv(line, line + eqidx + 1);
> + free(line, M_TEMP);
> + cp += i + 1;
> + }
> +
> + hintmode = value;
> + use_kenv = 1;
> + return (0);
> +}
> +
> +SYSCTL_PROC(_kern, OID_AUTO, hintmode, CTLTYPE_INT|CTLFLAG_RW,
> +&hintmode, 0, sysctl_hintmode, "I", "Get/set current hintmode");
> +
> +/*
> * Evil wildcarding resource string lookup.
> * This walks the supplied env string table and returns a match.
> * The start point can be remembered for incremental searches.
___
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: r231220 - head/sys/sys
On Wed, Feb 8, 2012 at 10:36 AM, Konstantin Belousov wrote:
> Author: kib
> Date: Wed Feb 8 18:36:07 2012
> New Revision: 231220
> URL: http://svn.freebsd.org/changeset/base/231220
>
> Log:
> Trim 8 unused bytes from struct vnode on 64-bit architectures.
Doesn't this change the KBI? So should __FreeBSD_version be bumped?
Thanks,
matthew
> Modified:
> head/sys/sys/vnode.h
>
> Modified: head/sys/sys/vnode.h
> ==
> --- head/sys/sys/vnode.h Wed Feb 8 18:22:10 2012 (r231219)
> +++ head/sys/sys/vnode.h Wed Feb 8 18:36:07 2012 (r231220)
> @@ -149,8 +149,8 @@ struct vnode {
> struct lock *v_vnlock; /* u pointer to vnode lock */
> int v_holdcnt; /* i prevents recycling. */
> int v_usecount; /* i ref count of users */
> - u_long v_iflag; /* i vnode flags (see below)
> */
> - u_long v_vflag; /* v vnode flags */
> + u_int v_iflag; /* i vnode flags (see below)
> */
> + u_int v_vflag; /* v vnode flags */
> int v_writecount; /* v ref count of writers */
>
> /*
___
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: r253636 - head/sys/vm
On Thu, Jul 25, 2013 at 4:43 AM, David Chisnall wrote: > However(), memset is to be preferred in this idiom because the compiler > provides better diagnostics in the case of error: > > bzero.c:9:22: warning: 'memset' call operates on objects of type 'struct > foo' > while the size is based on a different type 'struct foo *' > [-Wsizeof-pointer-memaccess] > memset(f, 0, sizeof(f)); >~^ > bzero.c:9:22: note: did you mean to dereference the argument to 'sizeof' > (and > multiply it by the number of elements)? > memset(f, 0, sizeof(f)); > ^ > > The same line with bzero(f, sizeof(f)) generates no error. > Isn't that a compiler bug? memset(p, 0, n) is the same as bzero(p, n). Why would the compiler warn on one and not the other? Does clang have a similar bias for memcpy versus bcopy? 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: r247014 - head/lib/libc/stdlib
On Tue, Feb 19, 2013 at 3:57 PM, Giorgos Keramidas wrote:
> Author: keramida (doc committer)
> Date: Tue Feb 19 23:57:39 2013
> New Revision: 247014
> URL: http://svnweb.freebsd.org/changeset/base/247014
>
> Log:
> Add a sample program that shows how a custom comparison function and
> qsort(3) can work together to sort an array of integers.
>
> PR: docs/176197
> Submitted by: Fernando, fapesteguia at opensistemas.com
> Approved by:gjb (mentor)
> MFC after: 1 week
>
> Modified:
> head/lib/libc/stdlib/qsort.3
>
> Modified: head/lib/libc/stdlib/qsort.3
> ==
> --- head/lib/libc/stdlib/qsort.3Tue Feb 19 23:46:51 2013
> (r247013)
> +++ head/lib/libc/stdlib/qsort.3Tue Feb 19 23:57:39 2013
> (r247014)
> @@ -32,7 +32,7 @@
> .\" @(#)qsort.38.1 (Berkeley) 6/4/93
> .\" $FreeBSD$
> .\"
> -.Dd September 30, 2003
> +.Dd February 20, 2013
> .Dt QSORT 3
> .Os
> .Sh NAME
> @@ -211,6 +211,52 @@ Previous versions of
> did not permit the comparison routine itself to call
> .Fn qsort 3 .
> This is no longer true.
> +.Sh EXAMPLES
> +A sample program that sorts an array of
> +.Vt int
> +values in place using
> +.Fn qsort ,
> +and then prints the sorted array to standard output is:
> +.Bd -literal
> +#include
> +#include
> +#include
> +
> +/*
> + * Custom comparison function that can compare 'int' values through pointers
> + * passed by qsort(3).
> + */
> +static int
> +int_compare(const void *p1, const void *p2)
> +{
> +int *left = (int *)p1;
> +int *right = (int *)p2;
These should be declared const int *. And the cast shouldn't be
needed in C, since void * can be assigned to any other pointer type.
Cheers,
matthew
> +
> +if (*left < *right)
> +return (-1);
> +else if (*left > *right)
> +return (1);
> +else
> +return (0);
> +}
> +
> +/*
> + * Sort an array of 'int' values and print it to standard output.
> + */
> +int
> +main(void)
> +{
> + int int_array[] = { 4, 5, 9, 3, 0, 1, 7, 2, 8, 6 };
> + const int array_size = sizeof(int_array) / sizeof(int_array[0]);
> + int k;
> +
> + qsort(&int_array, array_size, sizeof(int), int_compare);
> + for (k = 0; k < array_size; k++)
> +printf(" %d", int_array[k]);
> +printf("\\n");
> +exit(EXIT_SUCCESS);
> +}
> +.Ed
> .Sh ERRORS
> The
> .Fn heapsort
___
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: r248649 - in head: share/man/man9 sys/kern sys/sys
On Sat, Mar 23, 2013 at 8:11 AM, Will Andrews wrote:
> Author: will
> Date: Sat Mar 23 15:11:53 2013
> New Revision: 248649
> URL: http://svnweb.freebsd.org/changeset/base/248649
>
> Log:
> Extend taskqueue(9) to enable per-taskqueue callbacks.
>
> The scope of these callbacks is primarily to support actions that affect the
> taskqueue's thread environments. They are entirely optional, and
> consequently are introduced as a new API: taskqueue_set_callback().
>
> This interface allows the caller to specify that a taskqueue requires a
> callback and optional context pointer for a given callback type.
>
> The callback types included in this commit can be used to register a
> constructor and destructor for thread-local storage using osd(9). This
> allows a particular taskqueue to define that its threads require a specific
> type of TLS, without the need for a specially-orchestrated task-based
> mechanism for startup and shutdown in order to accomplish it.
>
> Two callback types are supported at this point:
>
> - TASKQUEUE_CALLBACK_TYPE_INIT, called by every thread when it starts, prior
> to processing any tasks.
> - TASKQUEUE_CALLBACK_TYPE_SHUTDOWN, called by every thread when it exits,
> after it has processed its last task but before the taskqueue is
> reclaimed.
>
> While I'm here:
>
> - Add two new macros, TQ_ASSERT_LOCKED and TQ_ASSERT_UNLOCKED, and use them
> in appropriate locations.
> - Fix taskqueue.9 to mention taskqueue_start_threads(), which is a required
> interface for all consumers of taskqueue(9).
>
> Reviewed by: kib (all), eadler (taskqueue.9), brd (taskqueue.9)
> Approved by: ken (mentor)
> Sponsored by: Spectra Logic
> MFC after:1 month
>
> Modified:
> head/share/man/man9/taskqueue.9
> head/sys/kern/subr_taskqueue.c
> head/sys/sys/taskqueue.h
>
> Modified: head/share/man/man9/taskqueue.9
> ==
> --- head/share/man/man9/taskqueue.9 Sat Mar 23 14:52:31 2013
> (r248648)
> +++ head/share/man/man9/taskqueue.9 Sat Mar 23 15:11:53 2013
> (r248649)
> @@ -53,12 +53,23 @@ struct task {
> void*ta_context;/* argument for handler */
> };
>
> +enum taskqueue_callback_type {
> + TASKQUEUE_CALLBACK_TYPE_INIT,
> + TASKQUEUE_CALLBACK_TYPE_SHUTDOWN,
> +};
> +
> +typedef void (*taskqueue_callback_fn)(void *context);
> +
> struct timeout_task;
> .Ed
> .Ft struct taskqueue *
> .Fn taskqueue_create "const char *name" "int mflags" "taskqueue_enqueue_fn
> enqueue" "void *context"
> .Ft struct taskqueue *
> .Fn taskqueue_create_fast "const char *name" "int mflags"
> "taskqueue_enqueue_fn enqueue" "void *context"
> +.Ft int
> +.Fn taskqueue_start_threads "struct taskqueue **tqp" "int count" "int pri"
> "const char *name" "..."
> +.Ft void
> +.Fn taskqueue_set_callback "struct taskqueue *queue" "enum
> taskqueue_callback_type cb_type" "taskqueue_callback_fn callback" "void
> *context"
> .Ft void
> .Fn taskqueue_free "struct taskqueue *queue"
> .Ft int
> @@ -127,6 +138,23 @@ should be used to free the memory used b
> Any tasks that are on the queue will be executed at this time after
> which the thread servicing the queue will be signaled that it should exit.
> .Pp
> +Once a taskqueue has been created, its threads should be started using
> +.Fn taskqueue_start_threads .
> +Callbacks may optionally be registered using
> +.Fn taskqueue_set_callback .
> +Currently, callbacks may be registered for the following purposes:
> +.Bl -tag -width TASKQUEUE_CALLBACK_TYPE_SHUTDOWN
> +.It Dv TASKQUEUE_CALLBACK_TYPE_INIT
> +This callback is called by every thread in the taskqueue, before it executes
> +any tasks.
> +This callback must be set before the taskqueue's threads are started.
> +.It Dv TASKQUEUE_CALLBACK_TYPE_SHUTDOWN
> +This callback is called by every thread in the taskqueue, after it executes
> +its last task.
> +This callback will always be called before the taskqueue structure is
> +reclaimed.
> +.El
> +.Pp
> To add a task to the list of tasks queued on a taskqueue, call
> .Fn taskqueue_enqueue
> with pointers to the queue and task.
>
> Modified: head/sys/kern/subr_taskqueue.c
> ==
> --- head/sys/kern/subr_taskqueue.c Sat Mar 23 14:52:31 2013
> (r248648)
> +++ head/sys/kern/subr_taskqueue.c Sat Mar 23 15:11:53 2013
> (r248649)
> @@ -63,6 +63,8 @@ struct taskqueue {
> int tq_spin;
> int tq_flags;
> int tq_callouts;
> + taskqueue_callback_fn tq_callbacks[TASKQUEUE_NUM_CALLBACKS];
> + void*tq_cb_contexts[TASKQUEUE_NUM_CALLBACKS];
> };
>
> #defineTQ_FLAGS_ACTIVE (1 << 0)
> @@ -78,6 +80,7 @@ struct taskqueue {
> else
Re: svn commit: r248649 - in head: share/man/man9 sys/kern sys/sys
On Sat, Mar 23, 2013 at 9:45 AM, Will Andrews wrote:
> I agree about the name length, it is a bit obnoxious. However, it is also
> descriptive. TQCB strikes me as perhaps a bit too far in the other
> direction. How about TQ_CALLBACK_? Is there an existing (pseudo)
> convention for callback names?
I'm not sure FreeBSD has anything standard, but at $WORK we use CB or
cb frequently as an abbreviation for callback. TASKQUEUE -> TQ still
removes 7 letters, which is a start. :-)
Thanks,
matthew
> On Sat, Mar 23, 2013 at 10:20 AM, wrote:
>>
>> On Sat, Mar 23, 2013 at 8:11 AM, Will Andrews wrote:
>> > Author: will
>> > Date: Sat Mar 23 15:11:53 2013
>> > New Revision: 248649
>> > URL: http://svnweb.freebsd.org/changeset/base/248649
>> >
>> > Log:
>> > Extend taskqueue(9) to enable per-taskqueue callbacks.
>> >
>> > The scope of these callbacks is primarily to support actions that
>> > affect the
>> > taskqueue's thread environments. They are entirely optional, and
>> > consequently are introduced as a new API: taskqueue_set_callback().
>> >
>> > This interface allows the caller to specify that a taskqueue requires
>> > a
>> > callback and optional context pointer for a given callback type.
>> >
>> > The callback types included in this commit can be used to register a
>> > constructor and destructor for thread-local storage using osd(9).
>> > This
>> > allows a particular taskqueue to define that its threads require a
>> > specific
>> > type of TLS, without the need for a specially-orchestrated task-based
>> > mechanism for startup and shutdown in order to accomplish it.
>> >
>> > Two callback types are supported at this point:
>> >
>> > - TASKQUEUE_CALLBACK_TYPE_INIT, called by every thread when it starts,
>> > prior
>> > to processing any tasks.
>> > - TASKQUEUE_CALLBACK_TYPE_SHUTDOWN, called by every thread when it
>> > exits,
>> > after it has processed its last task but before the taskqueue is
>> > reclaimed.
>> >
>> > While I'm here:
>> >
>> > - Add two new macros, TQ_ASSERT_LOCKED and TQ_ASSERT_UNLOCKED, and use
>> > them
>> > in appropriate locations.
>> > - Fix taskqueue.9 to mention taskqueue_start_threads(), which is a
>> > required
>> > interface for all consumers of taskqueue(9).
>> >
>> > Reviewed by: kib (all), eadler (taskqueue.9), brd (taskqueue.9)
>> > Approved by: ken (mentor)
>> > Sponsored by: Spectra Logic
>> > MFC after:1 month
>> >
>> > Modified:
>> > head/share/man/man9/taskqueue.9
>> > head/sys/kern/subr_taskqueue.c
>> > head/sys/sys/taskqueue.h
>> >
>> > Modified: head/share/man/man9/taskqueue.9
>> >
>> > ==
>> > --- head/share/man/man9/taskqueue.9 Sat Mar 23 14:52:31 2013
>> > (r248648)
>> > +++ head/share/man/man9/taskqueue.9 Sat Mar 23 15:11:53 2013
>> > (r248649)
>> > @@ -53,12 +53,23 @@ struct task {
>> > void*ta_context;/* argument for handler
>> > */
>> > };
>> >
>> > +enum taskqueue_callback_type {
>> > + TASKQUEUE_CALLBACK_TYPE_INIT,
>> > + TASKQUEUE_CALLBACK_TYPE_SHUTDOWN,
>> > +};
>> > +
>> > +typedef void (*taskqueue_callback_fn)(void *context);
>> > +
>> > struct timeout_task;
>> > .Ed
>> > .Ft struct taskqueue *
>> > .Fn taskqueue_create "const char *name" "int mflags"
>> > "taskqueue_enqueue_fn enqueue" "void *context"
>> > .Ft struct taskqueue *
>> > .Fn taskqueue_create_fast "const char *name" "int mflags"
>> > "taskqueue_enqueue_fn enqueue" "void *context"
>> > +.Ft int
>> > +.Fn taskqueue_start_threads "struct taskqueue **tqp" "int count" "int
>> > pri" "const char *name" "..."
>> > +.Ft void
>> > +.Fn taskqueue_set_callback "struct taskqueue *queue" "enum
>> > taskqueue_callback_type cb_type" "taskqueue_callback_fn callback" "void
>> > *context"
>> > .Ft void
>> > .Fn taskqueue_free "struct taskqueue *queue"
>> > .Ft int
>> > @@ -127,6 +138,23 @@ should be used to free the memory used b
>> > Any tasks that are on the queue will be executed at this time after
>> > which the thread servicing the queue will be signaled that it should
>> > exit.
>> > .Pp
>> > +Once a taskqueue has been created, its threads should be started using
>> > +.Fn taskqueue_start_threads .
>> > +Callbacks may optionally be registered using
>> > +.Fn taskqueue_set_callback .
>> > +Currently, callbacks may be registered for the following purposes:
>> > +.Bl -tag -width TASKQUEUE_CALLBACK_TYPE_SHUTDOWN
>> > +.It Dv TASKQUEUE_CALLBACK_TYPE_INIT
>> > +This callback is called by every thread in the taskqueue, before it
>> > executes
>> > +any tasks.
>> > +This callback must be set before the taskqueue's threads are started.
>> > +.It Dv TASKQUEUE_CALLBACK_TYPE_SHUTDOWN
>> > +This callback is called by every thread in the taskqueue, after it
>> > executes
>> > +its last task.
>> > +This callback will always be called before the taskqueue structure is
>> > +reclaimed.
>> > +.El
>>
Re: svn commit: r249105 - in head/sys/cam: ata scsi
On Fri, Apr 5, 2013 at 8:21 AM, Bruce Evans wrote: > This method works well in userland too. Instead of assert() or abort(), > use an null dereference, or more portably, a signal Digressing quite a bit, doesn't abort() send a signal already, i.e. SIGABRT? And doesn't __assert() call abort()? Cheers, 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"
