svn commit: r244202 - head/sys/netpfil/pf
Author: glebius Date: Fri Dec 14 08:02:35 2012 New Revision: 244202 URL: http://svnweb.freebsd.org/changeset/base/244202 Log: Fix VIMAGE build broken in r244185. Submitted by: Nikolai Lifanov Modified: head/sys/netpfil/pf/if_pfsync.c Modified: head/sys/netpfil/pf/if_pfsync.c == --- head/sys/netpfil/pf/if_pfsync.c Fri Dec 14 07:48:03 2012 (r244201) +++ head/sys/netpfil/pf/if_pfsync.c Fri Dec 14 08:02:35 2012 (r244202) @@ -615,7 +615,7 @@ pfsync_input(struct mbuf *m, __unused in len = ntohs(ph->len) + offset; if (m->m_pkthdr.len < len) { - pfsyncstats.pfsyncs_badlen++; + V_pfsyncstats.pfsyncs_badlen++; goto done; } ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244209 - head/sys/sys
Author: ae Date: Fri Dec 14 12:37:35 2012 New Revision: 244209 URL: http://svnweb.freebsd.org/changeset/base/244209 Log: Add an #include guard to the sys/fnv_hash.h. MFC after:3 days Modified: head/sys/sys/fnv_hash.h Modified: head/sys/sys/fnv_hash.h == --- head/sys/sys/fnv_hash.h Fri Dec 14 11:38:15 2012(r244208) +++ head/sys/sys/fnv_hash.h Fri Dec 14 12:37:35 2012(r244209) @@ -7,6 +7,8 @@ * * $FreeBSD$ */ +#ifndef _SYS_FNV_HASH_H_ +#define_SYS_FNV_HASH_H_ typedef u_int32_t Fnv32_t; typedef u_int64_t Fnv64_t; @@ -66,3 +68,4 @@ fnv_64_str(const char *str, Fnv64_t hval } return hval; } +#endif /* _SYS_FNV_HASH_H_ */ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244210 - head/sys/netpfil/pf
Author: glebius Date: Fri Dec 14 13:01:16 2012 New Revision: 244210 URL: http://svnweb.freebsd.org/changeset/base/244210 Log: Fix error in r235991. No-sleep version of IFNET_RLOCK() should be used here, since we may hold the main pf rulesets rwlock. Reported by: Fleuriot Damien Modified: head/sys/netpfil/pf/pf_if.c Modified: head/sys/netpfil/pf/pf_if.c == --- head/sys/netpfil/pf/pf_if.c Fri Dec 14 12:37:35 2012(r244209) +++ head/sys/netpfil/pf/pf_if.c Fri Dec 14 13:01:16 2012(r244210) @@ -478,10 +478,10 @@ pfi_table_update(struct pfr_ktable *kt, if (kif->pfik_ifp != NULL) pfi_instance_add(kif->pfik_ifp, net, flags); else if (kif->pfik_group != NULL) { - IFNET_RLOCK(); + IFNET_RLOCK_NOSLEEP(); TAILQ_FOREACH(ifgm, &kif->pfik_group->ifg_members, ifgm_next) pfi_instance_add(ifgm->ifgm_ifp, net, flags); - IFNET_RUNLOCK(); + IFNET_RUNLOCK_NOSLEEP(); } if ((e = pfr_set_addrs(&kt->pfrkt_t, V_pfi_buffer, V_pfi_buffer_cnt, &size2, ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244215 - head/sbin/savecore
Author: pjd Date: Fri Dec 14 15:01:23 2012 New Revision: 244215 URL: http://svnweb.freebsd.org/changeset/base/244215 Log: Whitespace cleanups. Modified: head/sbin/savecore/savecore.c Modified: head/sbin/savecore/savecore.c == --- head/sbin/savecore/savecore.c Fri Dec 14 14:59:35 2012 (r244214) +++ head/sbin/savecore/savecore.c Fri Dec 14 15:01:23 2012 (r244215) @@ -194,7 +194,7 @@ check_space(const char *savedir, off_t d syslog(LOG_ERR, "%s: %m", savedir); exit(1); } - spacefree = ((off_t) fsbuf.f_bavail * fsbuf.f_bsize) / 1024; + spacefree = ((off_t) fsbuf.f_bavail * fsbuf.f_bsize) / 1024; totfree = ((off_t) fsbuf.f_bfree * fsbuf.f_bsize) / 1024; (void)snprintf(path, sizeof(path), "%s/minfree", savedir); @@ -209,7 +209,7 @@ check_space(const char *savedir, off_t d } needed = dumpsize / 1024 + 2; /* 2 for info file */ - if (((minfree > 0) ? spacefree : totfree) - needed < minfree) { + if (((minfree > 0) ? spacefree : totfree) - needed < minfree) { syslog(LOG_WARNING, "no dump, not enough free space on device (%lld available, need %lld)", (long long)(minfree > 0 ? spacefree : totfree), @@ -262,7 +262,7 @@ DoRegularFile(int fd, off_t dumpsize, ch if (he >= hs + BLOCKSIZE) break; } - + /* back down to a block boundary */ he &= BLOCKMASK; @@ -433,7 +433,7 @@ DoFile(const char *savedir, const char * syslog(LOG_ERR, "unknown version (%d) in last dump header on %s", dtoh32(kdhl.version), device); - + status = STATUS_BAD; if (force == 0) goto closefd; @@ -444,7 +444,7 @@ DoFile(const char *savedir, const char * syslog(LOG_ERR, "unknown version (%d) in last dump header on %s", dtoh32(kdhl.version), device); - + status = STATUS_BAD; if (force == 0) goto closefd; @@ -472,7 +472,7 @@ DoFile(const char *savedir, const char * syslog(LOG_ERR, "unknown version (%d) in last dump header on %s", dtoh32(kdhl.version), device); - + status = STATUS_BAD; if (force == 0) goto closefd; ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244216 - head/sbin/savecore
Author: pjd Date: Fri Dec 14 15:03:12 2012 New Revision: 244216 URL: http://svnweb.freebsd.org/changeset/base/244216 Log: If we are not going to clear the dump (we are either just checking if the dump exists or we want to keep it), open device read-only. Obtained from:WHEEL Systems Modified: head/sbin/savecore/savecore.c Modified: head/sbin/savecore/savecore.c == --- head/sbin/savecore/savecore.c Fri Dec 14 15:01:23 2012 (r244215) +++ head/sbin/savecore/savecore.c Fri Dec 14 15:03:12 2012 (r244216) @@ -394,7 +394,7 @@ DoFile(const char *savedir, const char * if (verbose) printf("checking for kernel dump on device %s\n", device); - fd = open(device, O_RDWR); + fd = open(device, (checkfor || keep) ? O_RDONLY : O_RDWR); if (fd < 0) { syslog(LOG_ERR, "%s: %m", device); return; @@ -612,7 +612,7 @@ DoFile(const char *savedir, const char * printf("dump saved\n"); nuke: - if (clear || !keep) { + if (!keep) { if (verbose) printf("clearing dump header\n"); memcpy(kdhl.magic, KERNELDUMPMAGIC_CLEARED, sizeof kdhl.magic); ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244217 - head/sbin/savecore
Author: pjd Date: Fri Dec 14 15:04:39 2012 New Revision: 244217 URL: http://svnweb.freebsd.org/changeset/base/244217 Log: The clear option (-c) is not compatible with keep (-k) and compress (-z) options. Obtained from:WHEEL Systems Modified: head/sbin/savecore/savecore.c Modified: head/sbin/savecore/savecore.c == --- head/sbin/savecore/savecore.c Fri Dec 14 15:03:12 2012 (r244216) +++ head/sbin/savecore/savecore.c Fri Dec 14 15:04:39 2012 (r244217) @@ -681,6 +681,8 @@ main(int argc, char **argv) } if (checkfor && (clear || force || keep)) usage(); + if (clear && (compress || keep)) + usage(); argc -= optind; argv += optind; if (argc >= 1) { ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244218 - in head: etc/rc.d sbin/savecore
Author: pjd Date: Fri Dec 14 15:12:08 2012 New Revision: 244218 URL: http://svnweb.freebsd.org/changeset/base/244218 Log: - When checking if a dump exists on the given device there is no need to provide dump directory. Eliminate this redundant argument. This changes the usage, but the only risk here is that a warning will be printed about directory given as device. - Update usage of -C option. - When clearing dump header from the given device there is also no need to provide dump directory, although additional arguments for -c were not documented. - Document that -v can be used with -c and that list of devices can be given. Obtained from:WHEEL Systems Modified: head/etc/rc.d/savecore head/sbin/savecore/savecore.8 head/sbin/savecore/savecore.c Modified: head/etc/rc.d/savecore == --- head/etc/rc.d/savecore Fri Dec 14 15:04:39 2012(r244217) +++ head/etc/rc.d/savecore Fri Dec 14 15:12:08 2012(r244218) @@ -62,7 +62,7 @@ savecore_start() ;; esac - if savecore -C "${dumpdir}" "${dev}" >/dev/null; then + if savecore -C "${dev}" >/dev/null; then savecore ${savecore_flags} ${dumpdir} ${dumpdev} if checkyesno crashinfo_enable; then ${crashinfo_program} -d ${dumpdir} Modified: head/sbin/savecore/savecore.8 == --- head/sbin/savecore/savecore.8 Fri Dec 14 15:04:39 2012 (r244217) +++ head/sbin/savecore/savecore.8 Fri Dec 14 15:12:08 2012 (r244218) @@ -28,7 +28,7 @@ .\" From: @(#)savecore.8 8.1 (Berkeley) 6/5/93 .\" $FreeBSD$ .\" -.Dd December 24, 2008 +.Dd December 14, 2012 .Dt SAVECORE 8 .Os .Sh NAME @@ -37,10 +37,12 @@ .Sh SYNOPSIS .Nm .Fl c +.Op Fl v +.Op Ar device ... .Nm .Fl C .Op Fl v -.Op Ar directory device +.Op Ar device ... .Nm .Op Fl fkvz .Op Ar directory Op Ar device ... Modified: head/sbin/savecore/savecore.c == --- head/sbin/savecore/savecore.c Fri Dec 14 15:04:39 2012 (r244217) +++ head/sbin/savecore/savecore.c Fri Dec 14 15:12:08 2012 (r244218) @@ -636,8 +636,8 @@ static void usage(void) { fprintf(stderr, "%s\n%s\n%s\n", - "usage: savecore -c", - " savecore -C [-v] [directory device]", + "usage: savecore -c [-v] [device ...]", + " savecore -C [-v] [device ...]", " savecore [-fkvz] [directory [device ...]]"); exit (1); } @@ -685,7 +685,7 @@ main(int argc, char **argv) usage(); argc -= optind; argv += optind; - if (argc >= 1) { + if (argc >= 1 && !checkfor && !clear) { error = chdir(argv[0]); if (error) { syslog(LOG_ERR, "chdir(%s): %m", argv[0]); ___ 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 Thursday, December 13, 2012 4:02:15 am Gleb Smirnoff wrote: > On Wed, Dec 12, 2012 at 04:53:48PM -0800, Alfred Perlstein wrote: > A> The problem again is that not all the KASSERTS are inviolable, if you > A> want to do a project to split them, then please do, it would really be > A> helpful, as for now, they are a mis-mash of death/warnings and there are > A> at least three vendors who approve of this as well as 3 long term > A> committers that approved my change (not including Adrian). > > Can you show examples of not inviolable KASSERTs? There are none. They are all assertions for a reason. However, in my experience at several large consumers of FreeBSD, no one wants to run with INVARIANTS in production. Not because we don't want panics (believe me, Yahoo! gets plenty of panics even with INVARIANTS disabled), but because the performance overhead, and redefining INVARIANTS into printf doesn't address that at all. Also, in regards to "if you think an a condition should be inviolable, make it a panic". I _did_ do that in WITNESS and you just reverted them! In all the cases of things like mismatching slock -> xlock you are about to corrupt WITNESS' internal state (leading to false positives or missed warnings) as well as the state of the locks themselves resulting in either hangs or random data corruption. Also, if you don't have a console wired up on all your machines (which not everyone does these days) a hang is _far_ worse than a crashdump, as when the machine hangs, you have to power cycle it, and you won't find the messages in /var/log/messages. With a straight-up panic if someone wants to run with INVARIANTS enabled they would instead have a nice crashdump that could be examined after the machine comes back up that points to the offending location. So in general, I will never use this and find it doesn't add any benefit whatsoever. OTOH, if it is not invasive (e.g. your original commit), then I think it might be ok if there are some folks who might actually find it useful. That said, I think direct use of kassert_panic() such as your changes to WITNESS is wrong. If you are going to change explicit panics, make them into a KASSERT() instead of changing a panic into a kassert_panic(). -- 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: r244154 - head/bin/ps
On Thursday, December 13, 2012 6:12:44 am Pawel Jakub Dawidek wrote: > On Wed, Dec 12, 2012 at 11:06:52PM +0200, Konstantin Belousov wrote: > > On Wed, Dec 12, 2012 at 03:45:04PM +, Pawel Jakub Dawidek wrote: > > > Author: pjd > > > Date: Wed Dec 12 15:45:03 2012 > > > New Revision: 244154 > > > URL: http://svnweb.freebsd.org/changeset/base/244154 > > > > > > Log: > > > Use kern.max_pid sysctl to obtain maximum PID number instead of using > > > local > > > define. > > It is pid_max, not max_pid. > > > > But the change is wrong. The kern.pid_max only limits newly allocated pids, > > it does not magically moves existing pids, which are out of range, to the > > limited region. See the corresponding commit log for the description. > > It was added to make it easier to run FreeBSD 1.x binaries on the modern > > kernels. > > I saw CTLFLAG_TUN on the sysctl and assumed it is read-only... > How about defining BSD_PID_MAX in sys/proc.h, which would be visible by > userland as well and setting PID_MAX to BSD_PID_MAX? > > This would also help bsnmpd. > > http://people.freebsd.org/~pjd/patches/PID_MAX.patch This doesn't help your actual use case though where you want to boot a kernel with a different PID_MAX. I would much rather our tools learn such constants from the kernel via sysctl than have them compiled in. So, I would add a new sysctl which exports the true PID_MAX constant (and is read-only and never changes) and use that in ps, etc. -- 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: r244112 - head/sys/kern
on 13/12/2012 19:01 m...@freebsd.org said the following: > 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? So if some code is a tool and it is disabled by default and someone believes that they will use it, then the code is acceptable to include into the tree no questions asked? -- Andriy Gapon ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244224 - head/share/mk
Author: emaste Date: Fri Dec 14 18:30:01 2012 New Revision: 244224 URL: http://svnweb.freebsd.org/changeset/base/244224 Log: Minor refactoring prior to .symbols file changes - Combine .if x and .if !x using .else - Separate out beforelinking dependency - Add comments to clarify .if nesting Sponsored by: ADARA Networks Modified: head/share/mk/bsd.lib.mk head/share/mk/bsd.prog.mk Modified: head/share/mk/bsd.lib.mk == --- head/share/mk/bsd.lib.mkFri Dec 14 16:28:10 2012(r244223) +++ head/share/mk/bsd.lib.mkFri Dec 14 18:30:01 2012(r244224) @@ -39,9 +39,7 @@ CFLAGS+= ${DEBUG_FLAGS} .if ${MK_CTF} != "no" && ${DEBUG_FLAGS:M-g} != "" CTFFLAGS+= -g .endif -.endif - -.if !defined(DEBUG_FLAGS) +.else STRIP?=-s .endif @@ -173,10 +171,9 @@ SOLINKOPTS+= -Wl,--fatal-warnings -Wl,-- .endif .if target(beforelinking) -${SHLIB_NAME}: ${SOBJS} beforelinking -.else -${SHLIB_NAME}: ${SOBJS} +${SHLIB_NAME}: beforelinking .endif +${SHLIB_NAME}: ${SOBJS} @${ECHO} building shared library ${SHLIB_NAME} @rm -f ${.TARGET} ${SHLIB_LINK} .if defined(SHLIB_LINK) Modified: head/share/mk/bsd.prog.mk == --- head/share/mk/bsd.prog.mk Fri Dec 14 16:28:10 2012(r244223) +++ head/share/mk/bsd.prog.mk Fri Dec 14 18:30:01 2012(r244224) @@ -46,10 +46,9 @@ PROG=${PROG_CXX} OBJS+= ${SRCS:N*.h:R:S/$/.o/g} .if target(beforelinking) -${PROG}: ${OBJS} beforelinking -.else -${PROG}: ${OBJS} +${PROG}: beforelinking .endif +${PROG}: ${OBJS} .if defined(PROG_CXX) ${CXX} ${CXXFLAGS} ${LDFLAGS} -o ${.TARGET} ${OBJS} ${LDADD} .else @@ -76,10 +75,9 @@ SRCS=${PROG}.c OBJS= ${PROG}.o .if target(beforelinking) -${PROG}: ${OBJS} beforelinking -.else -${PROG}: ${OBJS} +${PROG}: beforelinking .endif +${PROG}: ${OBJS} .if defined(PROG_CXX) ${CXX} ${CXXFLAGS} ${LDFLAGS} -o ${.TARGET} ${OBJS} ${LDADD} .else @@ -90,7 +88,7 @@ ${PROG}: ${OBJS} .endif .endif -.endif +.endif # !defined(SRCS) .if${MK_MAN} != "no" && !defined(MAN) && \ !defined(MAN1) && !defined(MAN2) && !defined(MAN3) && \ @@ -100,7 +98,7 @@ ${PROG}: ${OBJS} MAN= ${PROG}.1 MAN1= ${MAN} .endif -.endif +.endif # defined(PROG) all: objwarn ${PROG} ${SCRIPTS} .if ${MK_MAN} != "no" ___ 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: r244154 - head/bin/ps
On Fri, Dec 14, 2012 at 11:52:15AM -0500, John Baldwin wrote: > On Thursday, December 13, 2012 6:12:44 am Pawel Jakub Dawidek wrote: > > On Wed, Dec 12, 2012 at 11:06:52PM +0200, Konstantin Belousov wrote: > > > On Wed, Dec 12, 2012 at 03:45:04PM +, Pawel Jakub Dawidek wrote: > > > > Author: pjd > > > > Date: Wed Dec 12 15:45:03 2012 > > > > New Revision: 244154 > > > > URL: http://svnweb.freebsd.org/changeset/base/244154 > > > > > > > > Log: > > > > Use kern.max_pid sysctl to obtain maximum PID number instead of using > > > > local > > > > define. > > > It is pid_max, not max_pid. > > > > > > But the change is wrong. The kern.pid_max only limits newly allocated > > > pids, > > > it does not magically moves existing pids, which are out of range, to the > > > limited region. See the corresponding commit log for the description. > > > It was added to make it easier to run FreeBSD 1.x binaries on the modern > > > kernels. > > > > I saw CTLFLAG_TUN on the sysctl and assumed it is read-only... > > How about defining BSD_PID_MAX in sys/proc.h, which would be visible by > > userland as well and setting PID_MAX to BSD_PID_MAX? > > > > This would also help bsnmpd. > > > > http://people.freebsd.org/~pjd/patches/PID_MAX.patch > > This doesn't help your actual use case though where you want to boot a kernel > with a different PID_MAX. I would much rather our tools learn such constants > from the kernel via sysctl than have them compiled in. So, I would add a new > sysctl which exports the true PID_MAX constant (and is read-only and never > changes) and use that in ps, etc. In that case I'd prefer to make existing kern.pid_max sysctl read-only and make it loader tunable. I don't expect there are many users of this sysctl... -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpQfA26S7SNa.pgp Description: PGP signature
svn commit: r244226 - head/sys/kern
Author: rmacklem Date: Fri Dec 14 21:49:06 2012 New Revision: 244226 URL: http://svnweb.freebsd.org/changeset/base/244226 Log: The group list for a non-default export entry (a host/subnet one) was being copied from the wrong place. This patch fixes that. This could cause access failures for mapped users, when the group permissions were needed. PR: 147998 Submitted by: Christopher Key (cjk32 at cam.ac.uk) MFC after:2 weeks Modified: head/sys/kern/vfs_export.c Modified: head/sys/kern/vfs_export.c == --- head/sys/kern/vfs_export.c Fri Dec 14 21:22:23 2012(r244225) +++ head/sys/kern/vfs_export.c Fri Dec 14 21:49:06 2012(r244226) @@ -208,7 +208,7 @@ vfs_hang_addrlist(struct mount *mp, stru np->netc_anon = crget(); np->netc_anon->cr_uid = argp->ex_anon.cr_uid; crsetgroups(np->netc_anon, argp->ex_anon.cr_ngroups, - np->netc_anon->cr_groups); + argp->ex_anon.cr_groups); np->netc_anon->cr_prison = &prison0; prison_hold(np->netc_anon->cr_prison); np->netc_numsecflavors = argp->ex_numsecflavors; ___ 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: r244188 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
On Thu, Dec 13, 2012 at 05:39:08PM +, Steven Hartland wrote: > Author: smh > Date: Thu Dec 13 17:39:07 2012 > New Revision: 244188 > URL: http://svnweb.freebsd.org/changeset/base/244188 > > Log: > Added vfs.zfs.vdev.trim_on_init sysctl which allows full vdev trim on > initialisation to be enabled (1) / disabled (0) defaults to enabled. > > This is useful for devices which have a slow trim speed and are either > new or have otherwise already been wiped e.g. secure erase. > > PR: kern/173116 > Submitted by: Steven Hartland > Approved by:pjd (mentor) > > Modified: > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_label.c > > Modified: head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_label.c > == > --- head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_label.c Thu Dec > 13 17:06:38 2012(r244187) > +++ head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/vdev_label.c Thu Dec > 13 17:39:07 2012(r244188) > @@ -148,6 +148,11 @@ > #include > #include > > +static boolean_t vdev_trim_on_init = B_TRUE; > +SYSCTL_DECL(_vfs_zfs_vdev); > +SYSCTL_INT(_vfs_zfs_vdev, OID_AUTO, trim_on_init, CTLFLAG_RW, > +&vdev_trim_on_init, 0, "Enable/disable full vdev trim on > initialisation"); I'd also make it loader tunable. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpnNSjsjlwVE.pgp Description: PGP signature
Re: svn commit: r244193 - head/sys/x86/include
On 12/13/12 21:49, Bruce Evans wrote: On Thu, 13 Dec 2012, Jim Harris wrote: Log: Add bus_space_read_8 and bus_space_write_8 for amd64. Rather than trying to KASSERT for callers that invoke this on IO tags, either do nothing (for write_8) or return ~0 (for read_8). read_8 returns a uint64_t, so it cannot return the signed integer ~0. It actually returns this signed integer converted to uint64_t. On amd64, this is the 32 bit value 0x. The 64-bit value 0x should be returned. I double checked and 0x is being returned when compiled by both clang and gcc. Using KASSERT here just makes bus.h too messy from both polluting bus.h with systm.h (for any number of drivers that include bus.h without first including systm.h) or ports that use bus.h directly (i.e. libpciaccess) as reported by zeising@. Also don't try to implement all of the other bus_space functions for 8 byte access since realistically only these two are needed for some devices that expose 64-bit memory-mapped registers. Good. Put the amd64-specific functions here rather than sys/amd64/include/bus.h so that we can keep this header unified for x86, as requested by mdf@ and tijl@. Not so good. I don't really like any of the unified headers. Me neither, but it sounds like there is a good reason for it, which I'll admit I don't fully understand. Sounds like it has something to do with cross-compiling. Modified: head/sys/x86/include/bus.h == --- head/sys/x86/include/bus.hThu Dec 13 21:39:59 2012 (r244192) +++ head/sys/x86/include/bus.hThu Dec 13 21:40:11 2012 (r244193) @@ -130,6 +130,7 @@ #define BUS_SPACE_MAXADDR0x #endif This file spells the F in hex constants in upper case. In the above definition and in previous ones, it is careful to spell out the constants and not depend on sign extension. So it is also a style bug to use ~0. Are you saying it is a style bug to not match the style used above, regardless of whether that style is right or wrong, or are you saying (~0) is always a style bug? Style bug visible in the above: space instead of tab after #define. This style bugs is in all #define's near here, including the new one. Type error in #define's just before the above: the above BUS_SPACE_MAXADDR is for 32 bits. For amd64 and i386-PAE, BUS_SPACE_MAXADDR is of course 64 bits, but the ifdef tangle for it is not tangled enough to be correct: BUS_SPACE_MAXADDR is 0xULL, on both, but bus_addr_t is only unsigned long long on i386-PAE. I think this should be a separate patch though, since it is unrelated to this change. +#define BUS_SPACE_INVALID_DATA(~0) #define BUS_SPACE_UNRESTRICTED(~0) /* @@ -221,6 +222,12 @@ static __inline u_int32_t bus_space_read bus_space_handle_t handle, bus_size_t offset); +#ifdef __amd64__ +static __inline uint64_t bus_space_read_8(bus_space_tag_t tag, + bus_space_handle_t handle, + bus_size_t offset); +#endif + This is style-bug for bug compatible with the existing forward declarations. Forward declarations of inline functions are nonsense. They are from NetBSD, for K&R support. But FreeBSD never supported K&R in bus-space headers, and the forward declarations never even compiled with K&R, since they never used __P(()). Almost 1/3 of the x86 bus.h consists of these negatively useful forward declarations. Some of them are almost as large as the full functions, since they are misformatted worse, with parameters starting at about column 40 instead of about column 20, so so that many lines are needed just for the parameters (to line them up in perfectly non-KNF gnu style). Same with this - separate patch ... @@ -251,8 +258,16 @@ bus_space_read_4(bus_space_tag_t tag, bu return (*(volatile u_int32_t *)(handle + offset)); } -#if 0/* Cause a link error for bus_space_read_8 */ -#definebus_space_read_8(t, h, o)!!! bus_space_read_8 unimplemented !!! +#ifdef __amd64__ +static __inline uint64_t +bus_space_read_8(bus_space_tag_t tag, bus_space_handle_t handle, + bus_size_t offset) +{ + +if (tag == X86_BUS_SPACE_IO) /* No 8 byte IO space access on x86 */ The comment is not indented, and should probably not be placed to the right of the code. This file mostly doesn't place comments there, and when it does it doesn't capitalize them. One that does also spells IO as i/o. All the #if 0 lines also start with an end of line comment, and they are all capitalized. By "not indented" are you saying that all end of line comments must be preceded by a tab? +return (BUS_SPACE_INVALID_DATA); +return (*(volatile uint64_t *)(handle + offset)); +} #endif Bruce ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn
svn commit: r244236 - head/share/mk
Author: emaste Date: Sat Dec 15 00:03:35 2012 New Revision: 244236 URL: http://svnweb.freebsd.org/changeset/base/244236 Log: Put shared library debug info into separate .symbols file Sponsored by: ADARA Networks Modified: head/share/mk/bsd.lib.mk head/share/mk/sys.mk Modified: head/share/mk/bsd.lib.mk == --- head/share/mk/bsd.lib.mkFri Dec 14 23:13:06 2012(r244235) +++ head/share/mk/bsd.lib.mkSat Dec 15 00:03:35 2012(r244236) @@ -173,11 +173,15 @@ SOLINKOPTS+= -Wl,--fatal-warnings -Wl,-- .if target(beforelinking) ${SHLIB_NAME}: beforelinking .endif +.if defined(DEBUG_FLAGS) +${SHLIB_NAME}.debug: ${SOBJS} +.else ${SHLIB_NAME}: ${SOBJS} +.endif @${ECHO} building shared library ${SHLIB_NAME} - @rm -f ${.TARGET} ${SHLIB_LINK} + @rm -f ${SHLIB_NAME} ${SHLIB_LINK} .if defined(SHLIB_LINK) - @ln -fs ${.TARGET} ${SHLIB_LINK} + @ln -fs ${SHLIB_NAME} ${SHLIB_LINK} .endif .if !defined(NM) @${CC} ${LDFLAGS} ${SSP_CFLAGS} ${SOLINKOPTS} \ @@ -191,6 +195,15 @@ ${SHLIB_NAME}: ${SOBJS} .if ${MK_CTF} != "no" ${CTFMERGE} ${CTFFLAGS} -o ${.TARGET} ${SOBJS} .endif + +.if defined(DEBUG_FLAGS) +${SHLIB_NAME}: ${SHLIB_NAME}.debug ${SHLIB_NAME}.symbols + ${OBJCOPY} --strip-debug --add-gnu-debuglink=${SHLIB_NAME}.symbols \ + ${SHLIB_NAME}.debug ${.TARGET} + +${SHLIB_NAME}.symbols: ${SHLIB_NAME}.debug + ${OBJCOPY} --only-keep-debug ${SHLIB_NAME}.debug ${.TARGET} +.endif .endif .if defined(INSTALL_PIC_ARCHIVE) && defined(LIB) && !empty(LIB) && ${MK_TOOLCHAIN} != "no" @@ -267,6 +280,11 @@ _libinstall: ${INSTALL} ${STRIP} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \ ${_INSTALLFLAGS} ${_SHLINSTALLFLAGS} \ ${SHLIB_NAME} ${DESTDIR}${SHLIBDIR} +.if defined(DEBUG_FLAGS) + ${INSTALL} -o ${LIBOWN} -g ${LIBGRP} -m ${LIBMODE} \ + ${_INSTALLFLAGS} ${_SHLINSTALLFLAGS} \ + ${SHLIB_NAME}.symbols ${DESTDIR}${SHLIBDIR} +.endif .if defined(SHLIB_LINK) # ${_SHLIBDIRPREFIX} and ${_LDSCRIPTROOT} are both needed when cross-building # and when building 32 bits library shims. ${_SHLIBDIRPREFIX} is the directory Modified: head/share/mk/sys.mk == --- head/share/mk/sys.mkFri Dec 14 23:13:06 2012(r244235) +++ head/share/mk/sys.mkSat Dec 15 00:03:35 2012(r244236) @@ -134,6 +134,8 @@ NM ?= nm OBJC ?= cc OBJCFLAGS ?= ${OBJCINCLUDES} ${CFLAGS} -Wno-import +OBJCOPY?= objcopy + PC ?= pc PFLAGS ?= ___ 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: r244236 - head/share/mk
On Fri, Dec 14, 2012 at 4:03 PM, Ed Maste wrote: > Author: emaste > Date: Sat Dec 15 00:03:35 2012 > New Revision: 244236 > URL: http://svnweb.freebsd.org/changeset/base/244236 ... > Modified: head/share/mk/sys.mk > == > --- head/share/mk/sys.mkFri Dec 14 23:13:06 2012(r244235) > +++ head/share/mk/sys.mkSat Dec 15 00:03:35 2012(r244236) > @@ -134,6 +134,8 @@ NM ?= nm > OBJC ?= cc > OBJCFLAGS ?= ${OBJCINCLUDES} ${CFLAGS} -Wno-import > > +OBJCOPY?= objcopy > + > PC ?= pc > PFLAGS ?= This shouldn't be defined when !defined(%POSIX) is true. The .endif should be pulled down to just above SHELL?= according to what I'm reading in http://pubs.opengroup.org/onlinepubs/009695399/utilities/make.html . Other spots in the tree where OBJCOPY?= is noted can be probably start to be reaped (thinking about sys/boot/, etc in particular). I have a PR open for some work Warner started with an initial patch that I've kind of been keeping up to date in git, but it needs to be updated now per this change (and also because I didn't notice that these variables weren't supposed to be defined when POSIX mode was on). Thanks! -Garrett ___ 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: r244188 - head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs
- Original Message - From: "Pawel Jakub Dawidek" On Thu, Dec 13, 2012 at 05:39:08PM +, Steven Hartland wrote: Author: smh Date: Thu Dec 13 17:39:07 2012 New Revision: 244188 URL: http://svnweb.freebsd.org/changeset/base/244188 Log: Added vfs.zfs.vdev.trim_on_init sysctl which allows full vdev trim on initialisation to be enabled (1) / disabled (0) defaults to enabled. This is useful for devices which have a slow trim speed and are either new or have otherwise already been wiped e.g. secure erase. ... +static boolean_t vdev_trim_on_init = B_TRUE; +SYSCTL_DECL(_vfs_zfs_vdev);> +SYSCTL_INT(_vfs_zfs_vdev, OID_AUTO, trim_on_init, CTLFLAG_RW, +&vdev_trim_on_init, 0, "Enable/disable full vdev trim on initialisation"); I'd also make it loader tunable. Will look into that, worth doing the other SYSCTL only ones at the same time? Regards Steve ___ 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: r244154 - head/bin/ps
On Fri, Dec 14, 2012 at 10:42:47PM +0100, Pawel Jakub Dawidek wrote: > On Fri, Dec 14, 2012 at 11:52:15AM -0500, John Baldwin wrote: > > On Thursday, December 13, 2012 6:12:44 am Pawel Jakub Dawidek wrote: > > > On Wed, Dec 12, 2012 at 11:06:52PM +0200, Konstantin Belousov wrote: > > > > On Wed, Dec 12, 2012 at 03:45:04PM +, Pawel Jakub Dawidek wrote: > > > > > Author: pjd > > > > > Date: Wed Dec 12 15:45:03 2012 > > > > > New Revision: 244154 > > > > > URL: http://svnweb.freebsd.org/changeset/base/244154 > > > > > > > > > > Log: > > > > > Use kern.max_pid sysctl to obtain maximum PID number instead of > > > > > using local > > > > > define. > > > > It is pid_max, not max_pid. > > > > > > > > But the change is wrong. The kern.pid_max only limits newly allocated > > > > pids, > > > > it does not magically moves existing pids, which are out of range, to > > > > the > > > > limited region. See the corresponding commit log for the description. > > > > It was added to make it easier to run FreeBSD 1.x binaries on the modern > > > > kernels. > > > > > > I saw CTLFLAG_TUN on the sysctl and assumed it is read-only... > > > How about defining BSD_PID_MAX in sys/proc.h, which would be visible by > > > userland as well and setting PID_MAX to BSD_PID_MAX? > > > > > > This would also help bsnmpd. > > > > > > http://people.freebsd.org/~pjd/patches/PID_MAX.patch > > > > This doesn't help your actual use case though where you want to boot a > > kernel > > with a different PID_MAX. I would much rather our tools learn such > > constants > > from the kernel via sysctl than have them compiled in. So, I would add a > > new > > sysctl which exports the true PID_MAX constant (and is read-only and never > > changes) and use that in ps, etc. > > In that case I'd prefer to make existing kern.pid_max sysctl read-only > and make it loader tunable. I don't expect there are many users of this > sysctl... > No, I described you the purpose of the sysctl. Requiring reboot just for running the old binaries is not useful. Please do not break it. pgpF4mkFwGjjP.pgp Description: PGP signature
svn commit: r244237 - head/sys/kern
Author: kib Date: Sat Dec 15 02:02:11 2012 New Revision: 244237 URL: http://svnweb.freebsd.org/changeset/base/244237 Log: Remove a special case for XEN, which is erronous and makes vfork(2) behaviour to differ from the documented, only on XEN. If there are any issues with XEN pmap left, they should be fixed in pmap. MFC after:2 weeks Modified: head/sys/kern/kern_fork.c Modified: head/sys/kern/kern_fork.c == --- head/sys/kern/kern_fork.c Sat Dec 15 00:03:35 2012(r244236) +++ head/sys/kern/kern_fork.c Sat Dec 15 02:02:11 2012(r244237) @@ -150,11 +150,7 @@ sys_vfork(struct thread *td, struct vfor int error, flags; struct proc *p2; -#ifdef XEN - flags = RFFDG | RFPROC; /* validate that this is still an issue */ -#else flags = RFFDG | RFPROC | RFPPWAIT | RFMEM; -#endif error = fork1(td, flags, 0, &p2, NULL, 0); if (error == 0) { td->td_retval[0] = p2->p_pid; ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244238 - head/sys/sys
Author: kib Date: Sat Dec 15 02:03:06 2012 New Revision: 244238 URL: http://svnweb.freebsd.org/changeset/base/244238 Log: Line up the continuation backslashes. Sponsored by: The FreeBSD Foundation MFC after:3 days Modified: head/sys/sys/mount.h Modified: head/sys/sys/mount.h == --- head/sys/sys/mount.hSat Dec 15 02:02:11 2012(r244237) +++ head/sys/sys/mount.hSat Dec 15 02:03:06 2012(r244238) @@ -199,8 +199,8 @@ struct vnode *__mnt_vnode_next_all(struc struct vnode *__mnt_vnode_first_all(struct vnode **mvp, struct mount *mp); void __mnt_vnode_markerfree_all(struct vnode **mvp, struct mount *mp); -#define MNT_VNODE_FOREACH_ALL(vp, mp, mvp) \ - for (vp = __mnt_vnode_first_all(&(mvp), (mp)); \ +#define MNT_VNODE_FOREACH_ALL(vp, mp, mvp) \ + for (vp = __mnt_vnode_first_all(&(mvp), (mp)); \ (vp) != NULL; vp = __mnt_vnode_next_all(&(mvp), (mp))) #define MNT_VNODE_FOREACH_ALL_ABORT(mp, mvp) \ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244239 - head/sys/ufs/ufs
Author: kib Date: Sat Dec 15 02:03:59 2012 New Revision: 244239 URL: http://svnweb.freebsd.org/changeset/base/244239 Log: Fix a typo, resulting in the NULL pointer dereference. Reported and tested by: pho Sponsored by: The FreeBSD Foundation MFC after:3 days Modified: head/sys/ufs/ufs/ufs_quota.c Modified: head/sys/ufs/ufs/ufs_quota.c == --- head/sys/ufs/ufs/ufs_quota.cSat Dec 15 02:03:06 2012 (r244238) +++ head/sys/ufs/ufs/ufs_quota.cSat Dec 15 02:03:59 2012 (r244239) @@ -1044,7 +1044,7 @@ again: error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, td); if (error) { if (error == ENOENT) { - MNT_VNODE_FOREACH_ALL_ABORT(mp, mvp); + MNT_VNODE_FOREACH_ACTIVE_ABORT(mp, mvp); goto again; } continue; ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r244240 - in head/sys: kern sys
Author: kib Date: Sat Dec 15 02:04:46 2012 New Revision: 244240 URL: http://svnweb.freebsd.org/changeset/base/244240 Log: When mnt_vnode_next_active iterator cannot lock the next vnode and yields, specify the user priority for the yield. Otherwise, a higher-priority (kernel) thread could fall into the priority-inversion with the thread owning the mutex lock. On single-processor machines or UP kernels, do not loop adaptively when the next vnode cannot be locked, instead yield unconditionally. Restructure the iteration initializer and the iterator to remove code duplication. Put the code to fetch and lock a vnode next to the current marker, into the mnt_vnode_next_active() function, and use it instead of repeating the loop. Reported by: hrs, rmacklem Tested by:pho Sponsored by: The FreeBSD Foundation MFC after:3 days Modified: head/sys/kern/vfs_subr.c head/sys/sys/mount.h Modified: head/sys/kern/vfs_subr.c == --- head/sys/kern/vfs_subr.cSat Dec 15 02:03:59 2012(r244239) +++ head/sys/kern/vfs_subr.cSat Dec 15 02:04:46 2012(r244240) @@ -69,6 +69,7 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include #include #include #include @@ -4710,32 +4711,54 @@ __mnt_vnode_markerfree_all(struct vnode * These are helper functions for filesystems to traverse their * active vnodes. See MNT_VNODE_FOREACH_ACTIVE() in sys/mount.h */ -struct vnode * -__mnt_vnode_next_active(struct vnode **mvp, struct mount *mp) +static void +mnt_vnode_markerfree_active(struct vnode **mvp, struct mount *mp) +{ + + KASSERT((*mvp)->v_mount == mp, ("marker vnode mount list mismatch")); + + MNT_ILOCK(mp); + MNT_REL(mp); + MNT_IUNLOCK(mp); + free(*mvp, M_VNODE_MARKER); + *mvp = NULL; +} + +#ifdef SMP +#defineALWAYS_YIELD(mp_ncpus == 1) +#else +#defineALWAYS_YIELD1 +#endif + +static struct vnode * +mnt_vnode_next_active(struct vnode **mvp, struct mount *mp) { struct vnode *vp, *nvp; - if (should_yield()) - kern_yield(PRI_UNCHANGED); - mtx_lock(&vnode_free_list_mtx); -restart: + mtx_assert(&vnode_free_list_mtx, MA_OWNED); KASSERT((*mvp)->v_mount == mp, ("marker vnode mount list mismatch")); +restart: vp = TAILQ_NEXT(*mvp, v_actfreelist); + TAILQ_REMOVE(&mp->mnt_activevnodelist, *mvp, v_actfreelist); while (vp != NULL) { if (vp->v_type == VMARKER) { vp = TAILQ_NEXT(vp, v_actfreelist); continue; } if (!VI_TRYLOCK(vp)) { - if (should_yield()) { + if (ALWAYS_YIELD || should_yield()) { + TAILQ_INSERT_BEFORE(vp, *mvp, v_actfreelist); mtx_unlock(&vnode_free_list_mtx); - kern_yield(PRI_UNCHANGED); + kern_yield(PRI_USER); mtx_lock(&vnode_free_list_mtx); + goto restart; } - goto restart; + continue; } - if (vp->v_mount == mp && vp->v_type != VMARKER && - (vp->v_iflag & VI_DOOMED) == 0) + KASSERT(vp->v_type != VMARKER, ("locked marker %p", vp)); + KASSERT(vp->v_mount == mp || vp->v_mount == NULL, + ("alien vnode on the active list %p %p", vp, mp)); + if (vp->v_mount == mp && (vp->v_iflag & VI_DOOMED) == 0) break; nvp = TAILQ_NEXT(vp, v_actfreelist); VI_UNLOCK(vp); @@ -4745,22 +4768,31 @@ restart: /* Check if we are done */ if (vp == NULL) { mtx_unlock(&vnode_free_list_mtx); - __mnt_vnode_markerfree_active(mvp, mp); - mtx_assert(MNT_MTX(mp), MA_NOTOWNED); + mnt_vnode_markerfree_active(mvp, mp); return (NULL); } - TAILQ_REMOVE(&mp->mnt_activevnodelist, *mvp, v_actfreelist); TAILQ_INSERT_AFTER(&mp->mnt_activevnodelist, vp, *mvp, v_actfreelist); mtx_unlock(&vnode_free_list_mtx); ASSERT_VI_LOCKED(vp, "active iter"); KASSERT((vp->v_iflag & VI_ACTIVE) != 0, ("Non-active vp %p", vp)); return (vp); } +#undef ALWAYS_YIELD + +struct vnode * +__mnt_vnode_next_active(struct vnode **mvp, struct mount *mp) +{ + + if (should_yield()) + kern_yield(PRI_UNCHANGED); + mtx_lock(&vnode_free_list_mtx); + return (mnt_vnode_next_active(mvp, mp)); +} struct vnode * __mnt_vnode_first_active(struct vnode **mvp, struct mount *mp) { - struct vnode *vp, *nvp; + struct vnode *vp; *mvp = malloc(sizeof(struct vnode), M_VNODE_MARKER, M_WAIT
Re: svn commit: r244112 - head/sys/kern
On Fri, 14 Dec 2012, John Baldwin wrote: On Thursday, December 13, 2012 4:02:15 am Gleb Smirnoff wrote: On Wed, Dec 12, 2012 at 04:53:48PM -0800, Alfred Perlstein wrote: A> The problem again is that not all the KASSERTS are inviolable, if you A> want to do a project to split them, then please do, it would really be A> helpful, as for now, they are a mis-mash of death/warnings and there are A> at least three vendors who approve of this as well as 3 long term A> committers that approved my change (not including Adrian). Can you show examples of not inviolable KASSERTs? There are none. They are all assertions for a reason. However, in my experience at several large consumers of FreeBSD, no one wants to run with INVARIANTS in production. Not because we don't want panics (believe me, Yahoo! gets plenty of panics even with INVARIANTS disabled), but because the performance overhead, and redefining INVARIANTS into printf doesn't address that at all. In the past, FYI, the two major INVARIANTS hits were un-inlining of locking, and UMA using additional global locking to perform heavier-weight sanity checking. For me, at least, the former wasn't such a problem, but the latter was extremely measurable. I have occasionally wondered if we should have another option name for heavier-duty sanity checking, as is the case with WITNESS, SOCKBUF_DEBUG, etc, so that INVARIANTS overhead can be made tolerable for more real-world installations. As you observe, our invariants tests are generally things where you catch critical problems in much more debuggable states, rather than sources of additional fragility, and it would be great if we could leave more in so that bug reports were able to contain more information. Robert ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r244112 - head/sys/kern
On 12/14/12 4:12 PM, Robert Watson wrote: On Fri, 14 Dec 2012, John Baldwin wrote: On Thursday, December 13, 2012 4:02:15 am Gleb Smirnoff wrote: On Wed, Dec 12, 2012 at 04:53:48PM -0800, Alfred Perlstein wrote: A> The problem again is that not all the KASSERTS are inviolable, if you A> want to do a project to split them, then please do, it would really be A> helpful, as for now, they are a mis-mash of death/warnings and there are A> at least three vendors who approve of this as well as 3 long term A> committers that approved my change (not including Adrian). Can you show examples of not inviolable KASSERTs? There are none. They are all assertions for a reason. However, in my experience at several large consumers of FreeBSD, no one wants to run with INVARIANTS in production. Not because we don't want panics (believe me, Yahoo! gets plenty of panics even with INVARIANTS disabled), but because the performance overhead, and redefining INVARIANTS into printf doesn't address that at all. In the past, FYI, the two major INVARIANTS hits were un-inlining of locking, and UMA using additional global locking to perform heavier-weight sanity checking. For me, at least, the former wasn't such a problem, but the latter was extremely measurable. I have occasionally wondered if we should have another option name for heavier-duty sanity checking, as is the case with WITNESS, SOCKBUF_DEBUG, etc, so that INVARIANTS overhead can be made tolerable for more real-world installations. As you observe, our invariants tests are generally things where you catch critical problems in much more debuggable states, rather than sources of additional fragility, and it would be great if we could leave more in so that bug reports were able to contain more information. Robert Yes. The KTR system offers an interesting reference for a model that allows us to make a compile time decision about which areas to trace. There used to be a DIAGNOSTIC option for the more heavy checks, this is either removed or just not used these days. Again, using a mask like KTR might be a win if we bring back the equivalent of heavy weight DIAGNOSTIC option. Often I've been guilty of putting KASSERT(ptr != NULL) checks into the code too. Those are really just to make it less painful when hitting that bug, so instead of having to do a lot of homework to figure out the fault address and line number, I can just get a pretty printed message under INVARIANTS. Maybe those "pretty null checks" need to go under another option, perhaps DIAGNOSTIC, or another .. ??PRETTY_PANIC. Anyhow, I've always been a huge fan of FreeBSD due the additional debugging checks it offered. I was the one that suggested splassert() back in the day when we would continually find issues with spl calls. Additionally I found doing filesystem work a ton easier with DEBUG_VFS_LOCKS under FreeBSD than under Darwin which at the time did not have such a feature. One of my great joys in developing FreeBSD is the flexibility and power it offers us as developers by giving us a huge library of debugging tools. -Alfred ___ 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: r244105 - in head/sys: kern sys
On 12 Dec 2012, at 07:42, John Baldwin wrote: > On Monday, December 10, 2012 8:23:51 pm Alfred Perlstein wrote: >> Author: alfred >> Date: Tue Dec 11 01:23:50 2012 >> New Revision: 244105 >> URL: http://svnweb.freebsd.org/changeset/base/244105 >> >> Log: >> Switch the hardwired WITNESS panics to kassert_panic. >> >> This is an ongoing effort to provide runtime debug information >> useful in the field that does not panic existing installations. >> >> This gives us the flexibility needed when shipping images to a >> potentially large audience with WITNESS enabled without worrying >> about formerly non-fatal LORs hurting a release. >> >> Sponsored by: iXsystems > > Witness doesn't panic on LORs. These are all bigger violations for things > like doing sx_sunlock() on a exclusively locked sx lock. That is not safe > and > is merely going to result in data corruption and other unpleasantness. This > sounds like a very bad idea. Did you talk about this anywhere (I have not > caught up on various lists yet, so apologies if this has been discussed.) Eww. This is indeed pretty bad. I would like to see this commit reverted. Regards, -- Rui Paulo ___ 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: r244154 - head/bin/ps
On Sat, 15 Dec 2012, Konstantin Belousov wrote: On Fri, Dec 14, 2012 at 10:42:47PM +0100, Pawel Jakub Dawidek wrote: On Fri, Dec 14, 2012 at 11:52:15AM -0500, John Baldwin wrote: On Thursday, December 13, 2012 6:12:44 am Pawel Jakub Dawidek wrote: On Wed, Dec 12, 2012 at 11:06:52PM +0200, Konstantin Belousov wrote: On Wed, Dec 12, 2012 at 03:45:04PM +, Pawel Jakub Dawidek wrote: Author: pjd Date: Wed Dec 12 15:45:03 2012 New Revision: 244154 URL: http://svnweb.freebsd.org/changeset/base/244154 Log: Use kern.max_pid sysctl to obtain maximum PID number instead of using local define. It is pid_max, not max_pid. But the change is wrong. The kern.pid_max only limits newly allocated pids, it does not magically moves existing pids, which are out of range, to the limited region. See the corresponding commit log for the description. It was added to make it easier to run FreeBSD 1.x binaries on the modern kernels. I saw CTLFLAG_TUN on the sysctl and assumed it is read-only... How about defining BSD_PID_MAX in sys/proc.h, which would be visible by userland as well and setting PID_MAX to BSD_PID_MAX? This would also help bsnmpd. http://people.freebsd.org/~pjd/patches/PID_MAX.patch This doesn't help your actual use case though where you want to boot a kernel with a different PID_MAX. I would much rather our tools learn such constants from the kernel via sysctl than have them compiled in. So, I would add a new sysctl which exports the true PID_MAX constant (and is read-only and never changes) and use that in ps, etc. In that case I'd prefer to make existing kern.pid_max sysctl read-only and make it loader tunable. I don't expect there are many users of this sysctl... No, I described you the purpose of the sysctl. Requiring reboot just for running the old binaries is not useful. Please do not break it. ps doesn't make any useful use of either its BSD_PID_MAX or the kernel's PID_MAX anyway. Its only use is to break cases where these BSD_PID_MAX is smaller than the largest active pid. There are some useful uses of a limit: - to "optimize" the parsing function by not supporting pids larger than necessary - for formatting pids. The 9 limit is useful for restricting the width of pid fields. With a dynamic limit, you need complications for dynamic widths. With a larger fixed limit, you need complications for dyamic widths anyway, since after expanding the pid fields there is no longer enough space for fixed widths in other fields. BTW, someone broke the formatting for ruptime(1) in -current. It now uses a fixed and _far too large_ width for the host name, so that most lines have length precisely 80 and get double-spaced by auto=linefeed on 80-column terminals. "ps l" still uses < 80 columns, with 2 5-digit fields for pids. Wide fields still eat into the field width for the COMMAND field. Interestingly, someone improved the formatting for the UID field -- it now seems to be dynamic and usually takes 3-4 columns, where it used to have the fixed width 5. ps of course doesn't use the limit for these useful uses: % #define BSD_PID_MAX 9 /* Copy of PID_MAX from sys/proc.h. */ % static int % addelem_pid(struct listinfo *inf, const char *elem) % { % char *endp; % long tempid; % % if (*elem == '\0') { % warnx("Invalid (zero-length) process id"); % optfatal = 1; % return (0); /* Do not add this value. */ % } % % errno = 0; % tempid = strtol(elem, &endp, 10); Here ps assumes that the limit is < LONG_MAX. A reasonable assumption. % if (*endp != '\0' || tempid < 0 || elem == endp) { % warnx("Invalid %s: %s", inf->lname, elem); % errno = ERANGE; % } else if (errno != 0 || tempid > BSD_PID_MAX) { % warnx("%s too large: %s", inf->lname, elem); % errno = ERANGE; % } All ps does with BSD_PID_MAX is to give an up-front error for values that are so large that they can't match any pids. It would be harmless for it to simply accept all representable values and let them not match any actual pid later. Example uses: - ps -p 0,745,756: all 3 pids are accepted - ps -p -1:-1 is rejected because it gives tempid < -1 - ps -p 9: 9 is accepted because it is at the limit - ps -p 99:99 is rejected because it is above the limit % if (errno == ERANGE) { % optfatal = 1; % return (0); % } % if (inf->count >= inf->maxcount) % expand_list(inf); % inf->l.pids[(inf->count)++] = tempid; l.pids[N] has type pid_t. We assume that the long tempid is representable as a pid. This is a valid assumption due to our earlier checks. 9 is smaller than 2**31, so it fits in pid_t because pid_t happens to be int32_t. This would not necessarily be true of the limit were dynamic. The kern.pid_max sysctl actuall
Re: svn commit: r244112 - head/sys/kern
On 12/13/12 6:37 AM, Andriy Gapon wrote: on 12/12/2012 19:06 Adrian Chadd said the following: kassert()s are already optional. Ie, you can choose to not compile them in. So the __dead2() code path bit for doing KASSERT() -> kassert_panic() at compile time isn't a problem. The problem is where you do panic() -> kassert_panic() (eg in the Witness code) which is what Alfred discovered shortly after doing up his initial patch. Anything which is a KASSERT() can and should be treated as a run-time warning just as much as a run-time "crash here so I can figure out what broke." Having the warning in a production box is going to be helpful for developers. I have a quite different view on purpose and costs of KASSERTs. Specifically referring to r243980 I do not think that "non-fatal asserts" should really exist (or do exist). I wish all this muddying of KASSERT meaning would get reverted. These quite sensitive changes were rushed in, IMO. having worked in a place where asserts were by default NON fatal, I can say that it is a REALLY BAD IDEA. it never did anything but cost us time and effort.. one reason it was there was the lack of a debugger in Linux at the time but all ports had top pay the price... (new linux does have one at last) On 12 December 2012 07:46, John Baldwin wrote: On Tuesday, December 11, 2012 2:08:14 am Alfred Perlstein wrote: Author: alfred Date: Tue Dec 11 07:08:14 2012 New Revision: 244112 URL: http://svnweb.freebsd.org/changeset/base/244112 Log: Cleanup more of the kassert_panic. fix compile warnings on !amd64 and NULL derefs that would happen if kassert_panic() would return. This is one reason why having kassert not panic is such a bad idea. There are tons of places where the compiler knows that panic() is __dead2, and there is no cleanup code to handle what happens when an invariant is violated. This is not safe to run in the field unless your customers do not care about their data. If you are interested in doing regression tests, I am using a very different approach for some locking regression tests I am working on in p4 that allow you to use a wrapper around setjmp/longjmp to "catch" panics somewhat like exception handling in C++/Java (though much cruder). However, evne that is only intended for testing, not for production cases where production data is at stake. -- 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"