Re: svn commit: r243627 - head/sys/kern
On 28.11.2012 00:59, Robert N. M. Watson wrote: On 27 Nov 2012, at 23:29, Andre Oppermann wrote: Andre.. this breaks incoming connections. TCP is immediately reset and never even gets to the listener process. You need to back out of fix this urgently please. I just found out and fixed it. Sorry for the breakage. I'd like to see a much more thorough use of "Reviewed by:" in socket and TCP-related commits -- this is very sensitive code, and a second pair of eyes is always valuable. Post-commit review is not a substitute. Looking back over similar changes in the socket code over the last two years, I see that almost all have reviewers, so I think it would be reasonable to consider it mandatory for these subsystems at this point. The good news is that we have lots of people with expertise in it. Good to see you becoming more active again. :-) And yes, you have a point there. Yes -- this is only about three weeks old, however; for the prior six-twelve months, I've been fairly non-existent in FreeBSD-land due to outside obligations :-). Just saw that I did indeed send you a review request three weeks ago. ;-) At the end of a rather long email though. Yes, indeed -- no patch was attached, and it followed quite a long e-mail on your plans to rewrite the TCP stack. I'm afraid that went onto the "read this later as time permits" pile as I was at a conference, rather than the fast-path "oh, quickly review this patch" pile. However, simply committing the patch rather than trying a bit harder to find a reviewer isn't the right answer either. To maximise the likelihood of a review, construct an e-mail with a subject line like "Review request: (patch description)", attach the patch, and include a proposed commit message. Yes, and I didn't really expect you to answer (at least quickly) during your FreeBSD hiatus. So it was seeking review by chance. Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from the fact that it was mea culpa in any case though. For prior review of kern_socket* and netinet/tcp_* related changes it has been on and off by various committers over the past year. If we do have a policy of prior review required then it should be made official and codified in MAINTAINERS and universally applied to all. -- Andre ___ 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: r243627 - head/sys/kern
On Wed, 28 Nov 2012, Andre Oppermann wrote: Yes, and I didn't really expect you to answer (at least quickly) during your FreeBSD hiatus. So it was seeking review by chance. Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from the fact that it was mea culpa in any case though. The rapid fix was, of course, extremely appreciated :-). For prior review of kern_socket* and netinet/tcp_* related changes it has been on and off by various committers over the past year. If we do have a policy of prior review required then it should be made official and codified in MAINTAINERS and universally applied to all. I tend to be of the view that 'maintainers' is a bad idea, and that we should just make a regular practice of seeking review for this sort of thing, especially as our community grows (and, let us be honest, complexity also grows -- your observations about decades of accumulated complexity in the TCP stack are not amiss). I'll try to take a look at this change in detail over the weekend -- listen/accept locking is a bit of a sore point; in the original design, I didn't have a separate accept lock, but ended up being forced to introduce it to solve races along these lines. In the past we've also relied on the pcbinfo lock in the protocol providing significant synchronisation during new connection events, and as we reduce the influence of that lock, finding more structured solutions is necessary. 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"
svn commit: r243649 - head/contrib/sendmail/src
Author: ume
Date: Wed Nov 28 11:47:47 2012
New Revision: 243649
URL: http://svnweb.freebsd.org/changeset/base/243649
Log:
cyrus-sasl 2.1.26 was released. In this version, the type of callback
functions was changed from "unsigned long" to "size_t".
Reviewed by: gshapiro
MFC after:3 days
Modified:
head/contrib/sendmail/src/sasl.c
Modified: head/contrib/sendmail/src/sasl.c
==
--- head/contrib/sendmail/src/sasl.cWed Nov 28 07:12:08 2012
(r243648)
+++ head/contrib/sendmail/src/sasl.cWed Nov 28 11:47:47 2012
(r243649)
@@ -24,9 +24,15 @@ SM_RCSID("@(#)$Id: sasl.c,v 8.22 2006/08
** using unsigned long: for portability, it should be size_t.
*/
-void *sm_sasl_malloc __P((unsigned long));
-static void *sm_sasl_calloc __P((unsigned long, unsigned long));
-static void *sm_sasl_realloc __P((void *, unsigned long));
+#if defined(SASL_VERSION_FULL) && SASL_VERSION_FULL >= 0x02011a
+#define SM_SASL_SIZE_T size_t
+#else /* defined(SASL_VERSION_FULL) && SASL_VERSION_FULL >= 0x02011a */
+#define SM_SASL_SIZE_T unsigned long
+#endif /* defined(SASL_VERSION_FULL) && SASL_VERSION_FULL >= 0x02011a */
+
+void *sm_sasl_malloc __P((SM_SASL_SIZE_T));
+static void *sm_sasl_calloc __P((SM_SASL_SIZE_T, SM_SASL_SIZE_T));
+static void *sm_sasl_realloc __P((void *, SM_SASL_SIZE_T));
void sm_sasl_free __P((void *));
/*
@@ -50,7 +56,7 @@ void sm_sasl_free __P((void *));
void *
sm_sasl_malloc(size)
- unsigned long size;
+ SM_SASL_SIZE_T size;
{
return sm_malloc((size_t) size);
}
@@ -71,8 +77,8 @@ sm_sasl_malloc(size)
static void *
sm_sasl_calloc(nelem, elemsize)
- unsigned long nelem;
- unsigned long elemsize;
+ SM_SASL_SIZE_T nelem;
+ SM_SASL_SIZE_T elemsize;
{
size_t size;
void *p;
@@ -99,7 +105,7 @@ sm_sasl_calloc(nelem, elemsize)
static void *
sm_sasl_realloc(o, size)
void *o;
- unsigned long size;
+ SM_SASL_SIZE_T size;
{
return sm_realloc(o, (size_t) 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: r243644 - head/usr.sbin/nfsd
On Wed, Nov 28, 2012 at 02:23:59AM +, Alfred Perlstein wrote:
> Author: alfred
> Date: Wed Nov 28 02:23:59 2012
> New Revision: 243644
> URL: http://svnweb.freebsd.org/changeset/base/243644
> Log:
> Fix typo.
> Pointed out by: marck
> Modified:
> head/usr.sbin/nfsd/nfsd.c
> Modified: head/usr.sbin/nfsd/nfsd.c
> ==
> --- head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:03:53 2012(r243643)
> +++ head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:23:59 2012(r243644)
> @@ -431,7 +431,7 @@ main(int argc, char **argv)
> nfsdcnt = DEFNFSDCNT;
> }
> if (nfsdcnt > MAXNFSDCNT) {
> - warnx("nfsd counta too high %d; reset to %d", nfsdcnt,
> + warnx("nfsd count too high %d; reset to %d", nfsdcnt,
> DEFNFSDCNT);
> nfsdcnt = MAXNFSDCNT;
> }
This says it reset the count to 4, while actually resetting it to 256.
--
Jilles Tjoelker
___
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: r243645 - head/usr.sbin/nfsd
On Wed, Nov 28, 2012 at 02:47:32AM +, Alfred Perlstein wrote:
> Author: alfred
> Date: Wed Nov 28 02:47:31 2012
> New Revision: 243645
> URL: http://svnweb.freebsd.org/changeset/base/243645
>
> Log:
> Don't allow minthreads > maxthreads.
>
> Suggested by: rmacklem
>
> Modified:
> head/usr.sbin/nfsd/nfsd.c
>
> Modified: head/usr.sbin/nfsd/nfsd.c
> ==
> --- head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:23:59 2012(r243644)
> +++ head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:47:31 2012(r243645)
> @@ -224,6 +224,10 @@ main(int argc, char **argv)
> udpflag = 1;
> argv += optind;
> argc -= optind;
> + if (minthreads_set && maxthreads_set && minthreads > maxthreads)
> + errx(EX_USAGE,
> + "error: minthreads(%d) can't be greater than "
> + "maxthreads(%d)", minthreads, maxthreads);
>
> /*
>* XXX
Should not this be also checked in the kernel? Looks like nfssvc_nfsd is
trustful:
[..]
if (args) {
nfsrv_pool->sp_minthreads = args->minthreads;
nfsrv_pool->sp_maxthreads = args->maxthreads;
} else {
nfsrv_pool->sp_minthreads = 4;
nfsrv_pool->sp_maxthreads = 4;
}
[..]
--
Mateusz Guzik
___
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: r243652 - head/sys/fs/ext2fs
Author: pfg
Date: Wed Nov 28 15:48:32 2012
New Revision: 243652
URL: http://svnweb.freebsd.org/changeset/base/243652
Log:
Update some definitions or make them match NetBSD's headers.
Bring several definitions required for newer ext4 features.
Rename EXT2F_COMPAT_HTREE to EXT2F_COMPAT_DIRHASHINDEX since it
is not being used yet and the new name is more compatible with
NetBSD and Linux.
This change is purely cosmetic and has no effect on the real
code.
Obtained from:NetBSD
MFC after:3 days
Modified:
head/sys/fs/ext2fs/ext2_dinode.h
head/sys/fs/ext2fs/ext2fs.h
Modified: head/sys/fs/ext2fs/ext2_dinode.h
==
--- head/sys/fs/ext2fs/ext2_dinode.hWed Nov 28 13:34:44 2012
(r243651)
+++ head/sys/fs/ext2fs/ext2_dinode.hWed Nov 28 15:48:32 2012
(r243652)
@@ -60,8 +60,15 @@
#define EXT2_APPEND0x0020 /* writes to file may only append */
#define EXT2_NODUMP0x0040 /* do not dump file */
#define EXT2_NOATIME 0x0080 /* do not update atime */
-
-#define EXT2_HTREE 0x1000 /* HTree-indexed directory */
+#define EXT2_INDEX 0x1000 /* hash-indexed directory */
+#define EXT2_IMAGIC0x2000 /* AFS directory */
+#define EXT2_JOURNAL_DATA 0x4000 /* file data should be
journaled */
+#define EXT2_NOTAIL0x8000 /* file tail should not be merged */
+#define EXT2_DIRSYNC 0x0001 /* dirsync behaviour */
+#define EXT2_TOPDIR0x0002 /* Top of directory hierarchies*/
+#define EXT2_HUGE_FILE 0x0004 /* Set to each huge file */
+#define EXT2_EXTENTS 0x0008 /* Inode uses extents */
+#define EXT2_EOFBLOCKS 0x0040 /* Blocks allocated beyond EOF */
/*
* Definitions for nanosecond timestamps.
@@ -95,9 +102,8 @@ struct ext2fs_dinode {
uint32_te2di_facl; /* 104: file ACL (not implemented) */
uint32_te2di_dacl; /* 108: dir ACL (not implemented) */
uint32_te2di_faddr; /* 112: fragment address */
- uint8_t e2di_nfrag; /* 116: fragment number */
- uint8_t e2di_fsize; /* 117: fragment size */
- uint16_te2di_linux_reserved2; /* 118 */
+ uint16_te2di_nblock_high; /* 116: Blocks count bits 47:32 */
+ uint16_te2di_facl_high; /* 118: file ACL bits 47:32 */
uint16_te2di_uid_high; /* 120: Owner UID top 16 bits */
uint16_te2di_gid_high; /* 122: Owner GID top 16 bits */
uint32_te2di_linux_reserved3; /* 124 */
Modified: head/sys/fs/ext2fs/ext2fs.h
==
--- head/sys/fs/ext2fs/ext2fs.h Wed Nov 28 13:34:44 2012(r243651)
+++ head/sys/fs/ext2fs/ext2fs.h Wed Nov 28 15:48:32 2012(r243652)
@@ -210,15 +210,23 @@ struct m_ext2fs {
#define EXT2F_COMPAT_PREALLOC 0x0001
#define EXT2F_COMPAT_HASJOURNAL0x0004
#define EXT2F_COMPAT_RESIZE0x0010
-#define EXT2F_COMPAT_HTREE 0x0020
+#define EXT2F_COMPAT_DIRHASHINDEX 0x0020
#define EXT2F_ROCOMPAT_SPARSESUPER 0x0001
#define EXT2F_ROCOMPAT_LARGEFILE 0x0002
#define EXT2F_ROCOMPAT_BTREE_DIR 0x0004
+#define EXT4F_ROCOMPAT_HUGE_FILE 0x0008
+#define EXT4F_ROCOMPAT_GDT_CSUM0x0010
+#define EXT4F_ROCOMPAT_DIR_NLINK 0x0020
#define EXT4F_ROCOMPAT_EXTRA_ISIZE 0x0040
#define EXT2F_INCOMPAT_COMP0x0001
#define EXT2F_INCOMPAT_FTYPE 0x0002
+#define EXT4F_INCOMPAT_META_BG 0x0010
+#define EXT4F_INCOMPAT_EXTENTS 0x0040
+#define EXT4F_INCOMPAT_64BIT 0x0080
+#define EXT4F_INCOMPAT_MMP 0x0100
+#define EXT4F_INCOMPAT_FLEX_BG 0x0200
/*
* Features supported in this implementation
___
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: r243631 - in head/sys: kern sys
I'm pretty sure that the "realmem" calculation is going to overflow on
i386/PAE, where the number of bytes of physical memory is greater than
the type long can represent.
On 11/27/2012 15:19, Andre Oppermann wrote:
> Author: andre
> Date: Tue Nov 27 21:19:58 2012
> New Revision: 243631
> URL: http://svnweb.freebsd.org/changeset/base/243631
>
> Log:
> Base the mbuf related limits on the available physical memory or
> kernel memory, whichever is lower. The overall mbuf related memory
> limit must be set so that mbufs (and clusters of various sizes)
> can't exhaust physical RAM or KVM.
>
> The limit is set to half of the physical RAM or KVM (whichever is
> lower) as the baseline. In any normal scenario we want to leave
> at least half of the physmem/kvm for other kernel functions and
> userspace to prevent it from swapping too easily. Via a tunable
> kern.maxmbufmem the limit can be upped to at most 3/4 of physmem/kvm.
>
> At the same time divorce maxfiles from maxusers and set maxfiles to
> physpages / 8 with a floor based on maxusers. This way busy servers
> can make use of the significantly increased mbuf limits with a much
> larger number of open sockets.
>
> Tidy up ordering in init_param2() and check up on some users of
> those values calculated here.
>
> Out of the overall mbuf memory limit 2K clusters and 4K (page size)
> clusters to get 1/4 each because these are the most heavily used mbuf
> sizes. 2K clusters are used for MTU 1500 ethernet inbound packets.
> 4K clusters are used whenever possible for sends on sockets and thus
> outbound packets. The larger cluster sizes of 9K and 16K are limited
> to 1/6 of the overall mbuf memory limit. When jumbo MTU's are used
> these large clusters will end up only on the inbound path. They are
> not used on outbound, there it's still 4K. Yes, that will stay that
> way because otherwise we run into lots of complications in the
> stack. And it really isn't a problem, so don't make a scene.
>
> Normal mbufs (256B) weren't limited at all previously. This was
> problematic as there are certain places in the kernel that on
> allocation failure of clusters try to piece together their packet
> from smaller mbufs.
>
> The mbuf limit is the number of all other mbuf sizes together plus
> some more to allow for standalone mbufs (ACK for example) and to
> send off a copy of a cluster. Unfortunately there isn't a way to
> set an overall limit for all mbuf memory together as UMA doesn't
> support such a limiting.
>
> NB: Every cluster also has an mbuf associated with it.
>
> Two examples on the revised mbuf sizing limits:
>
> 1GB KVM:
>512MB limit for mbufs
>419,430 mbufs
> 65,536 2K mbuf clusters
> 32,768 4K mbuf clusters
> 9,709 9K mbuf clusters
> 5,461 16K mbuf clusters
>
> 16GB RAM:
>8GB limit for mbufs
>33,554,432 mbufs
> 1,048,576 2K mbuf clusters
> 524,288 4K mbuf clusters
> 155,344 9K mbuf clusters
>87,381 16K mbuf clusters
>
> These defaults should be sufficient for even the most demanding
> network loads.
>
> MFC after: 1 month
>
> Modified:
> head/sys/kern/kern_mbuf.c
> head/sys/kern/subr_param.c
> head/sys/kern/uipc_socket.c
> head/sys/sys/eventhandler.h
> head/sys/sys/mbuf.h
>
> Modified: head/sys/kern/kern_mbuf.c
> ==
> --- head/sys/kern/kern_mbuf.c Tue Nov 27 20:22:36 2012(r243630)
> +++ head/sys/kern/kern_mbuf.c Tue Nov 27 21:19:58 2012(r243631)
> @@ -96,6 +96,7 @@ __FBSDID("$FreeBSD$");
> *
> */
>
> +int nmbufs; /* limits number of mbufs */
> int nmbclusters; /* limits number of mbuf clusters */
> int nmbjumbop; /* limits number of page size jumbo
> clusters */
> int nmbjumbo9; /* limits number of 9k jumbo clusters */
> @@ -147,9 +148,11 @@ sysctl_nmbclusters(SYSCTL_HANDLER_ARGS)
> newnmbclusters = nmbclusters;
> error = sysctl_handle_int(oidp, &newnmbclusters, 0, req);
> if (error == 0 && req->newptr) {
> - if (newnmbclusters > nmbclusters) {
> + if (newnmbclusters > nmbclusters &&
> + nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 + nmbjumbo16)
> {
> nmbclusters = newnmbclusters;
> uma_zone_set_max(zone_clust, nmbclusters);
> + nmbclusters = uma_zone_get_max(zone_clust);
> EVENTHANDLER_INVOKE(nmbclusters_change);
> } else
> error = EINVAL;
> @@ -168,9 +171,11 @@ sysctl_nmbjumbop(SYSCTL_HANDLER_ARGS)
> newnmbjumbop = nmbjumbop;
> error = sysctl_handle_int(oidp, &newnmbjumbop, 0, req);
> if (error == 0 && req->newptr) {
> - if (newnmbjumbop> nmbjumbop) {
> + if (newnmbjum
Re: svn commit: r243627 - head/sys/kern
On 11/28/12 12:01 AM, Andre Oppermann wrote: On 28.11.2012 00:59, Robert N. M. Watson wrote: On 27 Nov 2012, at 23:29, Andre Oppermann wrote: Andre.. this breaks incoming connections. TCP is immediately reset and never even gets to the listener process. You need to back out of fix this urgently please. I just found out and fixed it. Sorry for the breakage. I'd like to see a much more thorough use of "Reviewed by:" in socket and TCP-related commits -- this is very sensitive code, and a second pair of eyes is always valuable. Post-commit review is not a substitute. Looking back over similar changes in the socket code over the last two years, I see that almost all have reviewers, so I think it would be reasonable to consider it mandatory for these subsystems at this point. The good news is that we have lots of people with expertise in it. Good to see you becoming more active again. :-) And yes, you have a point there. Yes -- this is only about three weeks old, however; for the prior six-twelve months, I've been fairly non-existent in FreeBSD-land due to outside obligations :-). Just saw that I did indeed send you a review request three weeks ago. ;-) At the end of a rather long email though. Yes, indeed -- no patch was attached, and it followed quite a long e-mail on your plans to rewrite the TCP stack. I'm afraid that went onto the "read this later as time permits" pile as I was at a conference, rather than the fast-path "oh, quickly review this patch" pile. However, simply committing the patch rather than trying a bit harder to find a reviewer isn't the right answer either. To maximise the likelihood of a review, construct an e-mail with a subject line like "Review request: (patch description)", attach the patch, and include a proposed commit message. Yes, and I didn't really expect you to answer (at least quickly) during your FreeBSD hiatus. So it was seeking review by chance. Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from the fact that it was mea culpa in any case though. For prior review of kern_socket* and netinet/tcp_* related changes it has been on and off by various committers over the past year. If we do have a policy of prior review required then it should be made official and codified in MAINTAINERS and universally applied to all. Personally I don't think we need any more anchors attached to people's feet when developing FreeBSD. Mistakes will happen, they will happen in head. Slowing down the process to eliminate mistakes only works to slow down change and give a false sense of "fixing stability" when in fact the only thing "stable" is the slowness of submitting code. -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: r243627 - head/sys/kern
On Wed, 28 Nov 2012, Alfred Perlstein wrote: Yes, and I didn't really expect you to answer (at least quickly) during your FreeBSD hiatus. So it was seeking review by chance. Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from the fact that it was mea culpa in any case though. For prior review of kern_socket* and netinet/tcp_* related changes it has been on and off by various committers over the past year. If we do have a policy of prior review required then it should be made official and codified in MAINTAINERS and universally applied to all. Personally I don't think we need any more anchors attached to people's feet when developing FreeBSD. Mistakes will happen, they will happen in head. Slowing down the process to eliminate mistakes only works to slow down change and give a false sense of "fixing stability" when in fact the only thing "stable" is the slowness of submitting code. Yes, the purpose of review in this case would not be to catch (too many) dumb mistakes -- that's what testing is for -- but rather, to introduce architectural review and catch quite subtle mistakes. It was interesting doing the development cycle for the pcbgroup work a year or so ago -- having Bjoern do line-by-line reviews of changes generated not only lots of bug fixes, but also lots of high-level feedback on the design that improved it markedly. "MAINTAINERS" is a concept I'd like to see die, but the idea of seeking pre-commit review for changes, especially in sensitive code, is something to encourage. One reason review is so critical in this area is that there are lots of subtle implications to do with timing, object life cycles, inter-layer locking, etc, in the socket and TCP code -- things that any one author may not see the big picture on. We've been working to improve comments, but the problem there, of course, is that you end up with a book of design comments, and code review actually brings many of those concerns to light in a more structured and practical way. 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: r243627 - head/sys/kern
Alfred, On Wed, Nov 28, 2012 at 09:39:15AM -0800, Alfred Perlstein wrote: A> Personally I don't think we need any more anchors attached to people's A> feet when developing FreeBSD. A> A> Mistakes will happen, they will happen in head. Slowing down the A> process to eliminate mistakes only works to slow down change and give a A> false sense of "fixing stability" when in fact the only thing "stable" A> is the slowness of submitting code. This will eventually lead back to the situation when no one runs head, because it is unusable. -- Totus tuus, Glebius. ___ 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: r243627 - head/sys/kern
On 28 Nov 2012, at 17:51, Gleb Smirnoff wrote: > On Wed, Nov 28, 2012 at 09:39:15AM -0800, Alfred Perlstein wrote: > A> Personally I don't think we need any more anchors attached to people's > A> feet when developing FreeBSD. > A> > A> Mistakes will happen, they will happen in head. Slowing down the > A> process to eliminate mistakes only works to slow down change and give a > A> false sense of "fixing stability" when in fact the only thing "stable" > A> is the slowness of submitting code. > > This will eventually lead back to the situation when no one runs head, > because it is unusable. Also, based on past experience: I'm much happier reviewing shaky code before it goes into the tree than trying to debug it in situ and having to back it out. If our advice to many companies is that they should start developing products against head, we can't let the quality of the head get back to the way it was in the 5.x timeframe. Several factors have led to our having a nearly-production quality development head over the last few years -- one is much heavier use of branched development for features (first Perforce, and more recently, Subversion, git, etc branches); the other is much heavier use of code review, especially for critical parts of the system. Device driver authors have a lot more leeway, but for core parts of the design, seeking review during development of a feature, and then before merging it upstream, should be an expectation for all but the most trivial of changes. It's a two-way street, of course: if you review other people's code, they will review yours, so as more people use review, the pool of potential reviewers goes up as well. 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"
svn commit: r243659 - head/sys/vm
Author: alc
Date: Wed Nov 28 18:29:34 2012
New Revision: 243659
URL: http://svnweb.freebsd.org/changeset/base/243659
Log:
Add support for the (relatively) new object type OBJT_MGTDEVICE to
vm_object_set_memattr(). Also, add a "safety belt" so that
vm_object_set_memattr() doesn't silently modify undefined object types.
Reviewed by: kib
MFC after:10 days
Modified:
head/sys/vm/vm_object.c
Modified: head/sys/vm/vm_object.c
==
--- head/sys/vm/vm_object.c Wed Nov 28 18:15:38 2012(r243658)
+++ head/sys/vm/vm_object.c Wed Nov 28 18:29:34 2012(r243659)
@@ -301,6 +301,7 @@ vm_object_set_memattr(vm_object_t object
switch (object->type) {
case OBJT_DEFAULT:
case OBJT_DEVICE:
+ case OBJT_MGTDEVICE:
case OBJT_PHYS:
case OBJT_SG:
case OBJT_SWAP:
@@ -310,6 +311,9 @@ vm_object_set_memattr(vm_object_t object
break;
case OBJT_DEAD:
return (KERN_INVALID_ARGUMENT);
+ default:
+ panic("vm_object_set_memattr: object %p is of undefined type",
+ object);
}
object->memattr = memattr;
return (KERN_SUCCESS);
___
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: r243660 - head/sys/boot/forth
Author: dteske Date: Wed Nov 28 18:35:46 2012 New Revision: 243660 URL: http://svnweb.freebsd.org/changeset/base/243660 Log: Discussed at-length on -arch. Make the following interface changes to my beastie boot menu: + Move boot options to a submenu + Add a new "Boot Single" menu item + Make "Boot" item and new "Boot Single" item reverse when boot_single is set + Add new "Load Defaults" item (in new "Boot Options" submenu) for overridding loader.conf(5) provided values with system defaults. Reviewed by: adrian (co-mentor) Approved by: adrian (co-mentor) Modified: head/sys/boot/forth/menu-commands.4th head/sys/boot/forth/menu.rc Modified: head/sys/boot/forth/menu-commands.4th == --- head/sys/boot/forth/menu-commands.4th Wed Nov 28 18:29:34 2012 (r243659) +++ head/sys/boot/forth/menu-commands.4th Wed Nov 28 18:35:46 2012 (r243660) @@ -32,6 +32,51 @@ variable kernel_state variable root_state \ +\ Boot +\ + +: init_boot ( N -- N ) + dup + s" boot_single" getenv -1 <> if + drop ( n n c-addr -- n n ) \ unused + toggle_menuitem ( n n -- n n ) + s" set menu_keycode[N]=115" \ base command to execute + else + s" set menu_keycode[N]=98" \ base command to execute + then + 17 +c! \ replace 'N' with ASCII numeral + evaluate +; + +\ +\ Alternate Boot +\ + +: init_altboot ( N -- N ) + dup + s" boot_single" getenv -1 <> if + drop ( n c-addr -- n ) \ unused + toggle_menuitem ( n -- n ) + s" set menu_keycode[N]=109" \ base command to execute + else + s" set menu_keycode[N]=115" \ base command to execute + then + 17 +c! \ replace 'N' with ASCII numeral + evaluate +; + +: altboot ( -- ) + s" boot_single" 2dup getenv -1 <> if + drop ( c-addr/u c-addr -- c-addr/u ) \ unused + unsetenv ( c-addr/u -- ) + else + 2drop ( c-addr/u -- ) \ unused + s" set boot_single=YES" evaluate + then + 0 boot ( state -- ) +; + +\ \ ACPI \ @@ -287,3 +332,15 @@ variable root_state menu-redraw TRUE \ Loop menu again ; + +\ +\ Defaults +\ + +: set_default_boot_options ( N -- N TRUE ) + acpi_enable + safemode_disable + singleuser_disable + verbose_disable + 2 goto_menu +; Modified: head/sys/boot/forth/menu.rc == --- head/sys/boot/forth/menu.rc Wed Nov 28 18:29:34 2012(r243659) +++ head/sys/boot/forth/menu.rc Wed Nov 28 18:35:46 2012(r243660) @@ -18,57 +18,98 @@ menu-init \ initialize the menu area \ Initialize main menu constructs (see `menu.4th') \ NOTE: To use the `ansi' variants, add `loader_color=1' to loader.conf(5) -set menu_caption[1]="Boot [Enter]" -set menu_command[1]="boot" -set ansi_caption[1]="[1mB[37moot [1m[Enter][37m" -set menu_keycode[1]="98" - -set menu_caption[2]="[Esc]ape to loader prompt" -set menu_command[2]="goto_prompt" -set menu_keycode[2]="27" -set ansi_caption[2]="[1mEsc[37mape to loader prompt" +\ +\ MAIN MENU +\ + +set menuset_name1="main" + +set mainmenu_init[1]="init_boot" +set mainmenu_caption[1]="Boot Multi User [Enter]" +set maintoggled_text[1]="Boot [S]ingle User [Enter]" +set mainmenu_command[1]="boot" +set mainansi_caption[1]="[1mB[37moot Multi User [1m[Enter][37m" +set maintoggled_ansi[1]="Boot [1mS[37mingle User [1m[Enter][37m" +\ keycode set by init_boot + +set mainmenu_init[2]="init_altboot" +set mainmenu_caption[2]="Boot [S]ingle User" +set maintoggled_text[2]="Boot [M]ulti User" +set mainmenu_command[2]="altboot" +set mainansi_caption[2]="Boot [1mS[37mingle User" +set maintoggled_ansi[2]="Boot [1mM[37multi User" +\ keycode set by init_altboot + +set mainmenu_caption[3]="[Esc]ape to loader prompt" +set mainmenu_command[3]="goto_prompt" +set mainmenu_keycode[3]=27 +set mainansi_caption[3]="[1mEsc[37mape to loader prompt" \ Enable built-in "Reboot" trailing menuitem \ NOTE: appears before menu_options if configured \ -set menu_reboot +set mainmenu_reboot \ Enable "Options:" separator. When set to a numerical value (1-8), a visual \ separator is inserted before that menuitem number. \ -set menu_options=4 +set mainmenu_options=5 + +set mainmenu_caption[5]="Configure Boot [O]ptions..." +set mainmenu_command[5]="2 goto_menu" +set mainmenu_keycode[5]=111 +set mainansi_caption[5]="Configure Boot [1mO[37mptions..." + +\ +\ BOOT OPTIONS MENU +\ + +set menuset_name2="options" -set menu_caption[4]="[A]CPI Support off" -set toggled_text[4]="[A]CPI Support On" -set menu_command[4]="toggle_acpi" -set menu_keycode[4]="97" -set menu_acpi=4 -set ansi_caption[4]="[1mA[37mCPI Support [34;1mOff[37m" -set toggled_ansi[4]="[1mA[37mCPI
svn commit: r243661 - head/etc/devd
Author: hselasky
Date: Wed Nov 28 18:37:20 2012
New Revision: 243661
URL: http://svnweb.freebsd.org/changeset/base/243661
Log:
Regenerate usb.conf
MFC after:1 week
Modified:
head/etc/devd/usb.conf
Modified: head/etc/devd/usb.conf
==
--- head/etc/devd/usb.conf Wed Nov 28 18:35:46 2012(r243660)
+++ head/etc/devd/usb.conf Wed Nov 28 18:37:20 2012(r243661)
@@ -52,6 +52,17 @@ nomatch 32 {
nomatch 32 {
match "bus" "uhub[0-9]+";
match "mode" "host";
+ match "vendor" "0x05ac";
+ match "product" "0x12a8";
+ match "intclass" "0xff";
+ match "intsubclass" "0xfd";
+ match "intprotocol" "0x01";
+ action "kldload -n if_ipheth";
+};
+
+nomatch 32 {
+ match "bus" "uhub[0-9]+";
+ match "mode" "host";
match "vendor" "0x0104";
match "product" "0x00be";
action "kldload -n uipaq";
@@ -3069,7 +3080,7 @@ nomatch 32 {
match "bus" "uhub[0-9]+";
match "mode" "host";
match "vendor" "0x12d1";
- match "product"
"(0x1001|0x1003|0x1004|0x1401|0x1402|0x1403|0x1404|0x1405|0x1406|0x1407|0x1408|0x1409|0x140a|0x140b|0x140c|0x140d|0x140e|0x140f|0x1410|0x1411|0x1412|0x1413|0x1414|0x1415|0x1416|0x1417|0x1418|0x1419|0x141a|0x141b|0x141c|0x141d|0x141e|0x141f|0x1420|0x1421|0x1422|0x1423|0x1424|0x1425|0x1426|0x1427|0x1428|0x1429|0x142a|0x142b|0x142c|0x142d|0x142e|0x142f|0x1430|0x1431|0x1432|0x1433|0x1434|0x1435|0x1436|0x1437|0x1438|0x1439|0x143a|0x143b|0x143c|0x143d|0x143e|0x143f|0x1446|0x1465|0x14ac|0x14fe|0x1505|0x1506|0x1520|0x1803|0x1c05|0x1c0b)";
+ match "product"
"(0x1001|0x1003|0x1004|0x1401|0x1402|0x1403|0x1404|0x1405|0x1406|0x1407|0x1408|0x1409|0x140a|0x140b|0x140c|0x140d|0x140e|0x140f|0x1410|0x1411|0x1412|0x1413|0x1414|0x1415|0x1416|0x1417|0x1418|0x1419|0x141a|0x141b|0x141c|0x141d|0x141e|0x141f|0x1420|0x1421|0x1422|0x1423|0x1424|0x1425|0x1426|0x1427|0x1428|0x1429|0x142a|0x142b|0x142c|0x142d|0x142e|0x142f|0x1430|0x1431|0x1432|0x1433|0x1434|0x1435|0x1436|0x1437|0x1438|0x1439|0x143a|0x143b|0x143c|0x143d|0x143e|0x143f|0x1446|0x1464|0x1465|0x14ac|0x14c9|0x14d1|0x14fe|0x1505|0x1506|0x1520|0x1521|0x1803|0x1c05|0x1c0b)";
action "kldload -n u3g";
};
@@ -4600,5 +4611,5 @@ nomatch 32 {
action "kldload -n umass";
};
-# 2274 USB entries processed
+# 2279 USB entries processed
___
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: r243627 - head/sys/kern
Do you think we need another TRB? It could be used to oust undesirable committers if needed. Sent from my iPhone On Nov 28, 2012, at 10:25 AM, "Robert N. M. Watson" wrote: > > On 28 Nov 2012, at 17:51, Gleb Smirnoff wrote: > >> On Wed, Nov 28, 2012 at 09:39:15AM -0800, Alfred Perlstein wrote: >> A> Personally I don't think we need any more anchors attached to people's >> A> feet when developing FreeBSD. >> A> >> A> Mistakes will happen, they will happen in head. Slowing down the >> A> process to eliminate mistakes only works to slow down change and give a >> A> false sense of "fixing stability" when in fact the only thing "stable" >> A> is the slowness of submitting code. >> >> This will eventually lead back to the situation when no one runs head, >> because it is unusable. > > Also, based on past experience: I'm much happier reviewing shaky code before > it goes into the tree than trying to debug it in situ and having to back it > out. If our advice to many companies is that they should start developing > products against head, we can't let the quality of the head get back to the > way it was in the 5.x timeframe. Several factors have led to our having a > nearly-production quality development head over the last few years -- one is > much heavier use of branched development for features (first Perforce, and > more recently, Subversion, git, etc branches); the other is much heavier use > of code review, especially for critical parts of the system. Device driver > authors have a lot more leeway, but for core parts of the design, seeking > review during development of a feature, and then before merging it upstream, > should be an expectation for all but the most trivial of changes. It's a > two-way street, of course: if you review other people's code, they will > review your s, so as more people use review, the pool of potential reviewers goes up as well. > > 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: r243627 - head/sys/kern
On 28 Nov 2012, at 19:32, Alfred Perlstein wrote: > Do you think we need another TRB? > > It could be used to oust undesirable committers if needed. Are we seriously having a discussion in which the merits of favouring pre-commit code review for the things like TCP stack are in doubt? I'm not saying we need to seek universal consensus on all changes, rather, that I would strongly prefer that people committing to this code seek review (and clearly indicate it in commit messages) before rather than after sticking things in the tree. This stuff is incredibly subtle, and debugging problems in the field is vastly harder than catching them early in the cycle. I certainly wouldn't commit any non-trivial change to the code in question without asking someone to go through it line-by-line, and I'd prefer if others took the same view, as I often end up chasing the bugs later, in the field, where it is hardest to do so. Robert > > Sent from my iPhone > > On Nov 28, 2012, at 10:25 AM, "Robert N. M. Watson" > wrote: > >> >> On 28 Nov 2012, at 17:51, Gleb Smirnoff wrote: >> >>> On Wed, Nov 28, 2012 at 09:39:15AM -0800, Alfred Perlstein wrote: >>> A> Personally I don't think we need any more anchors attached to people's >>> A> feet when developing FreeBSD. >>> A> >>> A> Mistakes will happen, they will happen in head. Slowing down the >>> A> process to eliminate mistakes only works to slow down change and give a >>> A> false sense of "fixing stability" when in fact the only thing "stable" >>> A> is the slowness of submitting code. >>> >>> This will eventually lead back to the situation when no one runs head, >>> because it is unusable. >> >> Also, based on past experience: I'm much happier reviewing shaky code before >> it goes into the tree than trying to debug it in situ and having to back it >> out. If our advice to many companies is that they should start developing >> products against head, we can't let the quality of the head get back to the >> way it was in the 5.x timeframe. Several factors have led to our having a >> nearly-production quality development head over the last few years -- one is >> much heavier use of branched development for features (first Perforce, and >> more recently, Subversion, git, etc branches); the other is much heavier use >> of code review, especially for critical parts of the system. Device driver >> authors have a lot more leeway, but for core parts of the design, seeking >> review during development of a feature, and then before merging it upstream, >> should be an expectation for all but the most trivial of changes. It's a >> two-way street, of course: if you review other people's code, they will >> review you rs, so as more people use review, the pool of potential reviewers goes up as well. >> >> 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: r243631 - in head/sys: kern sys
On Wed, 28 Nov 2012, Alan Cox wrote:
I'm pretty sure that the "realmem" calculation is going to overflow on
i386/PAE, where the number of bytes of physical memory is greater than
the type long can represent.
It overflows on i386 even without PAE, where the number of bytes of
physical memory is greater than the type long can represent (2GB).
This is the usual case for new hardware.
I think it changes the defaults for machines with small amounts of
memory (say 32MB) in dangerous ways.
Reserving half of kva for mbufs is network-centric. I reserve more
than half of kva for the buffer cache in some configurations
(VM_BCACHE_SIZE_MAX defaults too 200 MB, but I use 512 MB), and in the
old scheme where the default for mbufs was under-sized, this may even
have fitted without separate tuning for mbufs.
The code has lots of style bugs.
On 11/27/2012 15:19, Andre Oppermann wrote:
...
NB: Every cluster also has an mbuf associated with it.
Two examples on the revised mbuf sizing limits:
1GB KVM:
512MB limit for mbufs
419,430 mbufs
65,536 2K mbuf clusters
32,768 4K mbuf clusters
9,709 9K mbuf clusters
5,461 16K mbuf clusters
16GB RAM:
8GB limit for mbufs
33,554,432 mbufs
1,048,576 2K mbuf clusters
524,288 4K mbuf clusters
155,344 9K mbuf clusters
87,381 16K mbuf clusters
These defaults should be sufficient for even the most demanding
network loads.
I noticed in earlier mail that there were no examples with small amounts
of physical memory. This case mixes unusually with the relatively large
amount of kva. PAE gives the opposite extreme.
Modified: head/sys/kern/kern_mbuf.c
==
--- head/sys/kern/kern_mbuf.c Tue Nov 27 20:22:36 2012(r243630)
+++ head/sys/kern/kern_mbuf.c Tue Nov 27 21:19:58 2012(r243631)
@@ -147,9 +148,11 @@ sysctl_nmbclusters(SYSCTL_HANDLER_ARGS)
newnmbclusters = nmbclusters;
error = sysctl_handle_int(oidp, &newnmbclusters, 0, req);
if (error == 0 && req->newptr) {
- if (newnmbclusters > nmbclusters) {
+ if (newnmbclusters > nmbclusters &&
+ nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 + nmbjumbo16)
{
Style bug: long line. This bogotifies early splitting in the previous line.
nmbclusters = newnmbclusters;
uma_zone_set_max(zone_clust, nmbclusters);
+ nmbclusters = uma_zone_get_max(zone_clust);
EVENTHANDLER_INVOKE(nmbclusters_change);
} else
error = EINVAL;
@@ -168,9 +171,11 @@ sysctl_nmbjumbop(SYSCTL_HANDLER_ARGS)
newnmbjumbop = nmbjumbop;
error = sysctl_handle_int(oidp, &newnmbjumbop, 0, req);
if (error == 0 && req->newptr) {
- if (newnmbjumbop> nmbjumbop) {
+ if (newnmbjumbop > nmbjumbop &&
+ nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 + nmbjumbo16)
{
Style bug, as above.
nmbjumbop = newnmbjumbop;
uma_zone_set_max(zone_jumbop, nmbjumbop);
+ nmbjumbop = uma_zone_get_max(zone_jumbop);
} else
error = EINVAL;
}
@@ -189,9 +194,11 @@ sysctl_nmbjumbo9(SYSCTL_HANDLER_ARGS)
newnmbjumbo9 = nmbjumbo9;
error = sysctl_handle_int(oidp, &newnmbjumbo9, 0, req);
if (error == 0 && req->newptr) {
- if (newnmbjumbo9> nmbjumbo9) {
+ if (newnmbjumbo9 > nmbjumbo9&&
Style bug, not as above (no space before '&&').
+ nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 + nmbjumbo16)
{
Style bug, as above.
nmbjumbo9 = newnmbjumbo9;
uma_zone_set_max(zone_jumbo9, nmbjumbo9);
+ nmbjumbo9 = uma_zone_get_max(zone_jumbo9);
} else
error = EINVAL;
}
@@ -209,9 +216,11 @@ sysctl_nmbjumbo16(SYSCTL_HANDLER_ARGS)
newnmbjumbo16 = nmbjumbo16;
error = sysctl_handle_int(oidp, &newnmbjumbo16, 0, req);
if (error == 0 && req->newptr) {
- if (newnmbjumbo16> nmbjumbo16) {
+ if (newnmbjumbo16 > nmbjumbo16 &&
+ nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 + nmbjumbo16)
{
Style bug, as above.
nmbjumbo16 = newnmbjumbo16;
uma_zone_set_max(zone_jumbo16, nmbjumbo16);
+ nmbjumbo16 = uma_zone_get_max(zone_jumbo16);
} else
error = EINVAL;
}
The code surrounding these style bugs uses the technique of nested elses
to make the indentation march to the right, when it could simply return
after an error and after the usual case where the sysctl only does a
read. For one of the sysctls, the old code is:
@ stati
svn commit: r243662 - head/share/misc
Author: pgj (ports committer) Date: Wed Nov 28 22:04:18 2012 New Revision: 243662 URL: http://svnweb.freebsd.org/changeset/base/243662 Log: - Update organization.dot to reflect that attilio resigned from core Approved by: core (implicit) Modified: head/share/misc/organization.dot Modified: head/share/misc/organization.dot == --- head/share/misc/organization.dotWed Nov 28 18:37:20 2012 (r243661) +++ head/share/misc/organization.dotWed Nov 28 22:04:18 2012 (r243662) @@ -25,7 +25,7 @@ _misc [label="Miscellaneous Hats"] # Development teams go here alphabetically sorted -core [label="Core Team\nc...@freebsd.org\ntabthorpe, gavin, jhb, kib,\ntheraven, attilio, hrs,\npeter, miwi"] +core [label="Core Team\nc...@freebsd.org\ntabthorpe, gavin, jhb, kib,\ntheraven, hrs, peter, miwi"] coresecretary [label="Core Team Secretary\ncore-secret...@freebsd.org\npgj"] doccommitters [label="Doc/www Committers\ndoc-committ...@freebsd.org"] doceng [label="Documentation Engineering Team\ndoc...@freebsd.org\ngjb, blackend,\ngabor, hrs"] ___ 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: r243631 - in head/sys: kern sys
Can you please post some example figures for machines with 16, 32, 64 and 128MB?
As Bruce has just replied, there's concern about this tuning being
very "lots of RAM" centric, which certainly isn't the case.
I'd hate to see this swing from one extreme (not enough mbufs) to the
other extreme ..
Adrian
On 27 November 2012 13:19, Andre Oppermann wrote:
> Author: andre
> Date: Tue Nov 27 21:19:58 2012
> New Revision: 243631
> URL: http://svnweb.freebsd.org/changeset/base/243631
>
> Log:
> Base the mbuf related limits on the available physical memory or
> kernel memory, whichever is lower. The overall mbuf related memory
> limit must be set so that mbufs (and clusters of various sizes)
> can't exhaust physical RAM or KVM.
>
> The limit is set to half of the physical RAM or KVM (whichever is
> lower) as the baseline. In any normal scenario we want to leave
> at least half of the physmem/kvm for other kernel functions and
> userspace to prevent it from swapping too easily. Via a tunable
> kern.maxmbufmem the limit can be upped to at most 3/4 of physmem/kvm.
>
> At the same time divorce maxfiles from maxusers and set maxfiles to
> physpages / 8 with a floor based on maxusers. This way busy servers
> can make use of the significantly increased mbuf limits with a much
> larger number of open sockets.
>
> Tidy up ordering in init_param2() and check up on some users of
> those values calculated here.
>
> Out of the overall mbuf memory limit 2K clusters and 4K (page size)
> clusters to get 1/4 each because these are the most heavily used mbuf
> sizes. 2K clusters are used for MTU 1500 ethernet inbound packets.
> 4K clusters are used whenever possible for sends on sockets and thus
> outbound packets. The larger cluster sizes of 9K and 16K are limited
> to 1/6 of the overall mbuf memory limit. When jumbo MTU's are used
> these large clusters will end up only on the inbound path. They are
> not used on outbound, there it's still 4K. Yes, that will stay that
> way because otherwise we run into lots of complications in the
> stack. And it really isn't a problem, so don't make a scene.
>
> Normal mbufs (256B) weren't limited at all previously. This was
> problematic as there are certain places in the kernel that on
> allocation failure of clusters try to piece together their packet
> from smaller mbufs.
>
> The mbuf limit is the number of all other mbuf sizes together plus
> some more to allow for standalone mbufs (ACK for example) and to
> send off a copy of a cluster. Unfortunately there isn't a way to
> set an overall limit for all mbuf memory together as UMA doesn't
> support such a limiting.
>
> NB: Every cluster also has an mbuf associated with it.
>
> Two examples on the revised mbuf sizing limits:
>
> 1GB KVM:
>512MB limit for mbufs
>419,430 mbufs
> 65,536 2K mbuf clusters
> 32,768 4K mbuf clusters
> 9,709 9K mbuf clusters
> 5,461 16K mbuf clusters
>
> 16GB RAM:
>8GB limit for mbufs
>33,554,432 mbufs
> 1,048,576 2K mbuf clusters
> 524,288 4K mbuf clusters
> 155,344 9K mbuf clusters
>87,381 16K mbuf clusters
>
> These defaults should be sufficient for even the most demanding
> network loads.
>
> MFC after:1 month
>
> Modified:
> head/sys/kern/kern_mbuf.c
> head/sys/kern/subr_param.c
> head/sys/kern/uipc_socket.c
> head/sys/sys/eventhandler.h
> head/sys/sys/mbuf.h
>
> Modified: head/sys/kern/kern_mbuf.c
> ==
> --- head/sys/kern/kern_mbuf.c Tue Nov 27 20:22:36 2012(r243630)
> +++ head/sys/kern/kern_mbuf.c Tue Nov 27 21:19:58 2012(r243631)
> @@ -96,6 +96,7 @@ __FBSDID("$FreeBSD$");
> *
> */
>
> +int nmbufs;/* limits number of mbufs */
> int nmbclusters; /* limits number of mbuf clusters */
> int nmbjumbop; /* limits number of page size jumbo clusters
> */
> int nmbjumbo9; /* limits number of 9k jumbo clusters */
> @@ -147,9 +148,11 @@ sysctl_nmbclusters(SYSCTL_HANDLER_ARGS)
> newnmbclusters = nmbclusters;
> error = sysctl_handle_int(oidp, &newnmbclusters, 0, req);
> if (error == 0 && req->newptr) {
> - if (newnmbclusters > nmbclusters) {
> + if (newnmbclusters > nmbclusters &&
> + nmbufs >= nmbclusters + nmbjumbop + nmbjumbo9 +
> nmbjumbo16) {
> nmbclusters = newnmbclusters;
> uma_zone_set_max(zone_clust, nmbclusters);
> + nmbclusters = uma_zone_get_max(zone_clust);
> EVENTHANDLER_INVOKE(nmbclusters_change);
> } else
> error = EINVAL;
> @@ -168,9 +171,11 @@ sysctl_nmbjumbop(SYSCTL_HANDLER_ARGS)
> newnmbjumbop = nmbjumbop;
> error = sysctl_handle_int(oidp, &newnmbjumbo
Re: svn commit: r243631 - in head/sys: kern sys
On 28.11.2012 18:37, Alan Cox wrote: I'm pretty sure that the "realmem" calculation is going to overflow on i386/PAE, where the number of bytes of physical memory is greater than the type long can represent. Right. long == int on i386/PAE, not LP64. Is uint64_t the correct type to use to catch that? -- Andre ___ 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: r243631 - in head/sys: kern sys
On Wed, 28 Nov 2012, Andre Oppermann wrote: On 28.11.2012 18:37, Alan Cox wrote: I'm pretty sure that the "realmem" calculation is going to overflow on i386/PAE, where the number of bytes of physical memory is greater than the type long can represent. Right. long == int on i386/PAE, not LP64. Is uint64_t the correct type to use to catch that? No. 2**63-1 bytes of physical memory might be enough for anyone, but more than that is is useful for virtual memory, and there is no need to ensure that P128 will be broken when it exists. I would just use sizes in pages for everything so that 32-bit u_ints are enough. Although this may break before P128 exists. Otherwise, uintmax_t should be used. Sloppy code can also depend on uintmax_t being "infinitely" large, so that multiplications by small scale factors don't overflow (use the more natural 'foo = bar * 3 / 4;' instead of 'foo = bar / 4 * 3;', but this can still overflow if bar is user input (say a tunable) that is not already limited enough. vm_paddr_t could be used for physical memory sizes, but might be too small or too large for virtual memory sizes, so using it would often give the same bloat as using uintmax_t, and more complications than using either u_int or uintmax_t for everything. 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: r243631 - in head/sys: kern sys
On 28.11.2012 22:45, Bruce Evans wrote:
On Wed, 28 Nov 2012, Alan Cox wrote:
I'm pretty sure that the "realmem" calculation is going to overflow on
i386/PAE, where the number of bytes of physical memory is greater than
the type long can represent.
It overflows on i386 even without PAE, where the number of bytes of
physical memory is greater than the type long can represent (2GB). This is the
usual case for new
hardware.
Please have a look at the attached patch. Is quad_t the appropriate
type to use? If not, which is the right one?
I think it changes the defaults for machines with small amounts of
memory (say 32MB) in dangerous ways.
Reserving half of kva for mbufs is network-centric. I reserve more
than half of kva for the buffer cache in some configurations
(VM_BCACHE_SIZE_MAX defaults too 200 MB, but I use 512 MB), and in the
old scheme where the default for mbufs was under-sized, this may even
have fitted without separate tuning for mbufs.
Please note that NO *reservations* are made for mbufs. It's only *limits*
that are enforced. For example stand-alone mbufs were not limited at all
previously and could completely exhaust all available kernel memory.
The code has lots of style bugs.
Thank you for your detailed input. I'll handle that after the "long"
overflow issue is solved.
--
Andre
Index: kern/subr_param.c
===
--- kern/subr_param.c (revision 243631)
+++ kern/subr_param.c (working copy)
@@ -271,7 +271,7 @@
void
init_param2(long physpages)
{
- long realmem;
+ quad_t realmem;
/* Base parameters */
maxusers = MAXUSERS;
@@ -332,10 +332,10 @@
* available kernel memory (physical or kmem).
* At most it can be 3/4 of available kernel memory.
*/
- realmem = lmin(physpages * PAGE_SIZE,
+ realmem = qmin(physpages * PAGE_SIZE,
VM_MAX_KERNEL_ADDRESS - VM_MIN_KERNEL_ADDRESS);
maxmbufmem = realmem / 2;
- TUNABLE_LONG_FETCH("kern.maxmbufmem", &maxmbufmem);
+ TUNABLE_QUAD_FETCH("kern.maxmbufmem", &maxmbufmem);
if (maxmbufmem > (realmem / 4) * 3)
maxmbufmem = (realmem / 4) * 3;
Index: sys/mbuf.h
===
--- sys/mbuf.h (revision 243631)
+++ sys/mbuf.h (working copy)
@@ -395,7 +395,7 @@
*
* The rest of it is defined in kern/kern_mbuf.c
*/
-extern longmaxmbufmem;
+extern quad_t maxmbufmem;
extern uma_zone_t zone_mbuf;
extern uma_zone_t zone_clust;
extern uma_zone_t zone_pack;
___
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: r243631 - in head/sys: kern sys
On 29.11.2012 00:14, Bruce Evans wrote: On Wed, 28 Nov 2012, Andre Oppermann wrote: On 28.11.2012 18:37, Alan Cox wrote: I'm pretty sure that the "realmem" calculation is going to overflow on i386/PAE, where the number of bytes of physical memory is greater than the type long can represent. Right. long == int on i386/PAE, not LP64. Is uint64_t the correct type to use to catch that? No. 2**63-1 bytes of physical memory might be enough for anyone, but more than that is is useful for virtual memory, and there is no need to ensure that P128 will be broken when it exists. I would just use sizes in pages for everything so that 32-bit u_ints are enough. Although this may break before P128 exists. Pages is not optimal as it would complicate the translations to the different mbuf sizes which are not necessarily integer multiple of pages. Otherwise, uintmax_t should be used. Sloppy code can also depend on uintmax_t being "infinitely" large, so that multiplications by small scale factors don't overflow (use the more natural 'foo = bar * 3 / 4;' instead of 'foo = bar / 4 * 3;', but this can still overflow if bar is user input (say a tunable) that is not already limited enough. In the other patch I already sent to you I've used quad_t because we already have an existing TUNABLE_QUAD_FETCH macro for it. It also more natural for people to specify a memory related number in raw bytes rather than pages (which are 8k on IA64 IIRC). vm_paddr_t could be used for physical memory sizes, but might be too small or too large for virtual memory sizes, so using it would often give the same bloat as using uintmax_t, and more complications than using either u_int or uintmax_t for everything. -- Andre ___ 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: r243631 - in head/sys: kern sys
On Thu, 29 Nov 2012, Andre Oppermann wrote:
On 28.11.2012 22:45, Bruce Evans wrote:
On Wed, 28 Nov 2012, Alan Cox wrote:
I'm pretty sure that the "realmem" calculation is going to overflow on
i386/PAE, where the number of bytes of physical memory is greater than
the type long can represent.
It overflows on i386 even without PAE, where the number of bytes of
physical memory is greater than the type long can represent (2GB). This is
the usual case for new
hardware.
Please have a look at the attached patch. Is quad_t the appropriate
type to use? If not, which is the right one?
quad_t is an old BSD type and shouldn't be used in any code newer than
C99. Using a signed type risks sign extension bugs (but using an unsigned
type may risk more).
Reserving half of kva for mbufs is network-centric. I reserve more
than half of kva for the buffer cache in some configurations
(VM_BCACHE_SIZE_MAX defaults too 200 MB, but I use 512 MB), and in the
old scheme where the default for mbufs was under-sized, this may even
have fitted without separate tuning for mbufs.
Please note that NO *reservations* are made for mbufs. It's only *limits*
that are enforced. For example stand-alone mbufs were not limited at all
previously and could completely exhaust all available kernel memory.
I think there is no real difference. If the limits are reached then
virtual memory may still be overcommitted.
Index: kern/subr_param.c
===
--- kern/subr_param.c (revision 243631)
+++ kern/subr_param.c (working copy)
@@ -271,7 +271,7 @@
void
init_param2(long physpages)
{
- long realmem;
+ quad_t realmem;
/* Base parameters */
maxusers = MAXUSERS;
@@ -332,10 +332,10 @@
* available kernel memory (physical or kmem).
* At most it can be 3/4 of available kernel memory.
*/
- realmem = lmin(physpages * PAGE_SIZE,
+ realmem = qmin(physpages * PAGE_SIZE,
VM_MAX_KERNEL_ADDRESS - VM_MIN_KERNEL_ADDRESS);
Oops, libkern hasn't caught up with C99 yet, so it doesn't have uimmin()
or uimmax() for uintmax_t. Its only new min/max functions since 1993 are
for off_t. These are mistakes, since off_t is not a basic type like all
the others and it is much less important than intmax_t. Using these
functions is too hard, but 4.4BSD intentionally left out the unsafe macros
MIN() and MAX() in the kernel. Someone broke this, so we now have MIN()
and MAX() and random choices of which is used.
Older subr_param code mostly doesn't use min/max functions, and you
could copy this and just use compare-and-assign for uintmax_t here.
maxmbufmem = realmem / 2;
- TUNABLE_LONG_FETCH("kern.maxmbufmem", &maxmbufmem);
+ TUNABLE_QUAD_FETCH("kern.maxmbufmem", &maxmbufmem);
It is a more intractable problem that tunables also haven't caught up
with C99 despite them being born in 1999 too. Nothing larger than
quad_t is supported. Values larger than QUAD_MAX are blindly clamped
to QUAD_MAX. This is better than for 32-bit ints and longs -- values
larger than the respective MAX are blindly truncated mod 2**32.
sysctls also haven't caught up with C99.
So there is nothing better than TUNABLE_QUAD_FETCH() here. Then you
can use any of quad_t, uquad_t, intmax_t or uintmax_t on the value.
But quad_t shouldn't be used after C99...
if (maxmbufmem > (realmem / 4) * 3)
maxmbufmem = (realmem / 4) * 3;
Index: sys/mbuf.h
===
--- sys/mbuf.h (revision 243631)
+++ sys/mbuf.h (working copy)
@@ -395,7 +395,7 @@
*
* The rest of it is defined in kern/kern_mbuf.c
*/
-extern longmaxmbufmem;
+extern quad_t maxmbufmem;
extern uma_zone_t zone_mbuf;
extern uma_zone_t zone_clust;
extern uma_zone_t zone_pack;
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: r243645 - head/usr.sbin/nfsd
Mateusz Guzik wrote:
> On Wed, Nov 28, 2012 at 02:47:32AM +, Alfred Perlstein wrote:
> > Author: alfred
> > Date: Wed Nov 28 02:47:31 2012
> > New Revision: 243645
> > URL: http://svnweb.freebsd.org/changeset/base/243645
> >
> > Log:
> > Don't allow minthreads > maxthreads.
> >
> > Suggested by: rmacklem
> >
> > Modified:
> > head/usr.sbin/nfsd/nfsd.c
> >
> > Modified: head/usr.sbin/nfsd/nfsd.c
> > ==
> > --- head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:23:59 2012 (r243644)
> > +++ head/usr.sbin/nfsd/nfsd.c Wed Nov 28 02:47:31 2012 (r243645)
> > @@ -224,6 +224,10 @@ main(int argc, char **argv)
> > udpflag = 1;
> > argv += optind;
> > argc -= optind;
> > + if (minthreads_set && maxthreads_set && minthreads > maxthreads)
> > + errx(EX_USAGE,
> > + "error: minthreads(%d) can't be greater than "
> > + "maxthreads(%d)", minthreads, maxthreads);
> >
> > /*
> > * XXX
>
> Should not this be also checked in the kernel? Looks like nfssvc_nfsd
> is
> trustful:
> [..]
> if (args) {
> nfsrv_pool->sp_minthreads = args->minthreads;
> nfsrv_pool->sp_maxthreads = args->maxthreads;
> } else {
> nfsrv_pool->sp_minthreads = 4;
> nfsrv_pool->sp_maxthreads = 4;
> }
> [..]
>
Well, since only root can do this and I can't think of why a sysadmin
would use anything other than nfsd, I'm not sure it matters much?
(But I don't see a problem with adding a sanity check in the kernel code.)
rick
> --
> Mateusz Guzik
___
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: r243663 - in head/sys/dev/usb: . serial
Author: eadler Date: Thu Nov 29 00:32:03 2012 New Revision: 243663 URL: http://svnweb.freebsd.org/changeset/base/243663 Log: Add support for AT&T Sierra Wireless USB 3G adapter PR: kern/173982 Submitted by: Eric Camachat Approved by: cperciva (implicit) MFC after:1 week Modified: head/sys/dev/usb/serial/u3g.c head/sys/dev/usb/usbdevs Modified: head/sys/dev/usb/serial/u3g.c == --- head/sys/dev/usb/serial/u3g.c Wed Nov 28 22:04:18 2012 (r243662) +++ head/sys/dev/usb/serial/u3g.c Thu Nov 29 00:32:03 2012 (r243663) @@ -213,6 +213,7 @@ static const STRUCT_USB_HOST_ID u3g_devs U3G_DEV(ACERP, H10, 0), U3G_DEV(AIRPLUS, MCD650, 0), U3G_DEV(AIRPRIME, PC5220, 0), + U3G_DEV(AIRPRIME, AC313U, 0), U3G_DEV(ALINK, 3G, 0), U3G_DEV(ALINK, 3GU, 0), U3G_DEV(ALINK, DWM652U5, 0), Modified: head/sys/dev/usb/usbdevs == --- head/sys/dev/usb/usbdevsWed Nov 28 22:04:18 2012(r243662) +++ head/sys/dev/usb/usbdevsThu Nov 29 00:32:03 2012(r243663) @@ -943,6 +943,7 @@ product AIRPLUS MCD650 0x3198 MCD650 mo /* AirPrime products */ product AIRPRIME PC52200x0112 CDMA Wireless PC Card product AIRPRIME USB3080x68A3 USB308 HSPA+ USB Modem +product AIRPRIME AC313U0x68aa Sierra Wireless AirCard 313U /* AirTies products */ product AIRTIES RT3070 0x2310 RT3070 ___ 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: r243627 - head/sys/kern
> a sign of poor prior testing and too much trust into the submitter of > the patch it wasn't an earth shattering event. Doesn't distract from Andre, I am really quite disappointed to read this from you. The patch I sent you was fine, and has been well tested here. You modified it and made the error. If you wish, I can attach the mail I sent with the patch. -vijay ___ 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: r243645 - head/usr.sbin/nfsd
On Wed, Nov 28, 2012 at 07:14:23PM -0500, Rick Macklem wrote: > Mateusz Guzik wrote: > > On Wed, Nov 28, 2012 at 02:47:32AM +, Alfred Perlstein wrote: > > > Author: alfred > > > Date: Wed Nov 28 02:47:31 2012 > > > New Revision: 243645 > > > URL: http://svnweb.freebsd.org/changeset/base/243645 > > > > > > Log: > > > Don't allow minthreads > maxthreads. > > > > > Should not this be also checked in the kernel? Looks like nfssvc_nfsd > > is > > trustful: > > > Well, since only root can do this and I can't think of why a sysadmin > would use anything other than nfsd, I'm not sure it matters much? > (But I don't see a problem with adding a sanity check in the kernel code.) > It's nothing serious and I'm not going to insist, especially since this is your code. I just prefer the kernel to be resistant if it does not cost much. I had something like this in mind (untested): http://people.freebsd.org/~mjg/patches/nfs-threads-check.diff -- Mateusz Guzik ___ 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: r243554 - in head/usr.sbin/pkg_install: add create delete info lib updating version
On Mon, 26 Nov 2012 05:11:07 + (UTC) Eitan Adler mentioned: > Author: eadler > Date: Mon Nov 26 05:11:07 2012 > New Revision: 243554 > URL: http://svnweb.freebsd.org/changeset/base/243554 > > Log: > Provide an option to users to shoot themselves in the foot. > This should probably be a default behavior. It's not good when all these warnings pop up everywhere just because local.sqlite file is present. -- Stanislav Sedov ST4096-RIPE () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments ___ 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: r243554 - in head/usr.sbin/pkg_install: add create delete info lib updating version
On 28 November 2012 20:00, Stanislav Sedov wrote: > On Mon, 26 Nov 2012 05:11:07 + (UTC) > Eitan Adler mentioned: > >> Author: eadler >> Date: Mon Nov 26 05:11:07 2012 >> New Revision: 243554 >> URL: http://svnweb.freebsd.org/changeset/base/243554 >> >> Log: >> Provide an option to users to shoot themselves in the foot. >> > > This should probably be a default behavior. It's not good when all these > warnings pop up everywhere just because local.sqlite file is present. Making the foot shooting behavior default defeats the point. If local.sqlite exists it almost certainly means that you don't want to run the old pkg_ tools. For the few correct uses an opt-out option is provided. -- Eitan Adler Source, Ports, Doc committer Bugmeister, Ports Security teams ___ 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: r243554 - in head/usr.sbin/pkg_install: add create delete info lib updating version
On Wed, 28 Nov 2012 21:03:58 -0500 Eitan Adler mentioned: > On 28 November 2012 20:00, Stanislav Sedov wrote: > > On Mon, 26 Nov 2012 05:11:07 + (UTC) > > Eitan Adler mentioned: > > > >> Author: eadler > >> Date: Mon Nov 26 05:11:07 2012 > >> New Revision: 243554 > >> URL: http://svnweb.freebsd.org/changeset/base/243554 > >> > >> Log: > >> Provide an option to users to shoot themselves in the foot. > >> > > > > This should probably be a default behavior. It's not good when all these > > warnings pop up everywhere just because local.sqlite file is present. > > Making the foot shooting behavior default defeats the point. > If local.sqlite exists it almost certainly means that you don't want > to run the old pkg_ tools. > For the few correct uses an opt-out option is provided. > Well, it's not entirely true. I did end up with local.sqlite because I stepped on the landmine of portmgr-pkg becoming the default for some reason. So I did end up with half of my packages being in the pkgng sqlite database, and half in standard /var/db/pkg. I had to convert them back to standard format by hand, but I still have the sqlite database just in case. I guess if you really want to prevent a foot-shooting, you should add a message to pkgng sayng in all caps e.g. "You are running experimental package manager and there's no migration plan from pkgng to old pkg exists." and maybe ask for confirmation. I don't really see how this message being in pkg_ tools helps to prevent a possible foot-shooting at all. At very least, it makes sense to make it conditional on WITH_PKGNG, so this code does not end up compiled in if PKGNG is disabled in src.conf. -- Stanislav Sedov ST4096-RIPE () ascii ribbon campaign - against html e-mail /\ www.asciiribbon.org - against proprietary attachments ___ 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: r243554 - in head/usr.sbin/pkg_install: add create delete info lib updating version
On 28 November 2012 21:34, Stanislav Sedov wrote: > Well, it's not entirely true. I did end up with local.sqlite because I > stepped on the landmine of portmgr-pkg becoming the default for some > reason. So I did end up with half of my packages being in the pkgng > sqlite database, and half in standard /var/db/pkg. How did this happen without running pkg2ng ? > I had to convert > them back to standard format by hand, but I still have the sqlite database > just in case. This is a rare case and exactly the use case for the environment variable. Perhaps you could also just rename the file. > I guess if you really want to prevent a foot-shooting, you should add > a message to pkgng sayng in all caps e.g. "You are running experimental > package manager and there's no migration plan from pkgng to old pkg > exists." and maybe ask for confirmation. pkg is not maintained by FreeBSD so I couldn't add this if I wanted to ;) > I don't really see how this > message being in pkg_ tools helps to prevent a possible foot-shooting > at all. It reminds people that using the pkg_* tools once a conversion is done is wrong. > At very least, it makes sense to make it conditional on WITH_PKGNG, so this > code does not end up compiled in if PKGNG is disabled in src.conf. Interesting idea. Maybe I shall do this. -- Eitan Adler Source, Ports, Doc committer Bugmeister, Ports Security teams ___ 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: r243664 - head/sys/conf
Author: marcel
Date: Thu Nov 29 03:48:39 2012
New Revision: 243664
URL: http://svnweb.freebsd.org/changeset/base/243664
Log:
Fix LINT build for arm: NOTES defines LDFLAGS by way of a make option
but LDFLAGS is not (yet) passed on to the linker (via SYSTEM_LD et al).
Do so now. As such, any kernel configuration can now define linker
flags by setting LDFLAGS as normal and not have to revert to hacks
like setting DEBUG for flags that do not relate to debugging (see
sys/powerpc/conf/MPC85XX).
Modified:
head/sys/conf/Makefile.arm
head/sys/conf/kern.pre.mk
Modified: head/sys/conf/Makefile.arm
==
--- head/sys/conf/Makefile.arm Thu Nov 29 00:32:03 2012(r243663)
+++ head/sys/conf/Makefile.arm Thu Nov 29 03:48:39 2012(r243664)
@@ -43,7 +43,7 @@ STRIP_FLAGS = -S
CFLAGS += -mno-apcs-frame
.endif
-SYSTEM_LD_ = ${LD} -Bdynamic -T ldscript.$M.noheader \
+SYSTEM_LD_ = ${LD} -Bdynamic -T ldscript.$M.noheader ${LDFLAGS} \
-warn-common -export-dynamic -dynamic-linker /red/herring -o \
${FULLKERNEL}.noheader -X ${SYSTEM_OBJS} vers.o
SYSTEM_LD_TAIL +=;sed s/" + SIZEOF_HEADERS"// ldscript.$M\
Modified: head/sys/conf/kern.pre.mk
==
--- head/sys/conf/kern.pre.mk Thu Nov 29 00:32:03 2012(r243663)
+++ head/sys/conf/kern.pre.mk Thu Nov 29 03:48:39 2012(r243664)
@@ -167,7 +167,7 @@ SYSTEM_DEP= Makefile ${SYSTEM_OBJS}
SYSTEM_OBJS= locore.o ${MDOBJS} ${OBJS}
SYSTEM_OBJS+= ${SYSTEM_CFILES:.c=.o}
SYSTEM_OBJS+= hack.So
-SYSTEM_LD= @${LD} -Bdynamic -T ${LDSCRIPT} --no-warn-mismatch \
+SYSTEM_LD= @${LD} -Bdynamic -T ${LDSCRIPT} ${LDFLAGS} --no-warn-mismatch \
-warn-common -export-dynamic -dynamic-linker /red/herring \
-o ${.TARGET} -X ${SYSTEM_OBJS} vers.o
SYSTEM_LD_TAIL= @${OBJCOPY} --strip-symbol gcc2_compiled. ${.TARGET} ; \
___
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"
Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
On Nov 28, 2012, at 9:39 AM, Alfred Perlstein wrote: > On 11/28/12 12:01 AM, Andre Oppermann wrote: >> On 28.11.2012 00:59, Robert N. M. Watson wrote: >>> >>> On 27 Nov 2012, at 23:29, Andre Oppermann wrote: >>> > Andre.. this breaks incoming connections. TCP is immediately reset > and never even gets to the > listener process. You need to back out of fix this urgently please. I just found out and fixed it. Sorry for the breakage. >>> >>> I'd like to see a much more thorough use of "Reviewed by:" in socket >>> and TCP-related commits -- this >>> is very sensitive code, and a second pair of eyes is always valuable. >>> Post-commit review is not a >>> substitute. Looking back over similar changes in the socket code over >>> the last two years, I see >>> that almost all have reviewers, so I think it would be reasonable to >>> consider it mandatory for these >>> subsystems at this point. The good news is that we have lots of people >>> with expertise in it. >> >> Good to see you becoming more active again. :-) And yes, >> you have a point there. > > Yes -- this is only about three weeks old, however; for the prior > six-twelve months, I've been fairly non-existent in FreeBSD-land due to > outside obligations :-). Just saw that I did indeed send you a review request three weeks ago. ;-) At the end of a rather long email though. >>> >>> Yes, indeed -- no patch was attached, and it followed quite a long e-mail >>> on your plans to rewrite the TCP stack. I'm afraid that went onto the "read >>> this later as time permits" pile as I was at a conference, rather than the >>> fast-path "oh, quickly review this patch" pile. However, simply committing >>> the patch rather than trying a bit harder to find a reviewer isn't the >>> right answer either. To maximise the likelihood of a review, construct an >>> e-mail with a subject line like "Review request: (patch description)", >>> attach the patch, and include a proposed commit message. >> >> Yes, and I didn't really expect you to answer (at least quickly) during >> your FreeBSD hiatus. So it was seeking review by chance. >> >> Alas I found and fixed the bug myself within 2.5hrs. While not optimal, >> a sign of poor prior testing and too much trust into the submitter of >> the patch it wasn't an earth shattering event. Doesn't distract from >> the fact that it was mea culpa in any case though. >> >> For prior review of kern_socket* and netinet/tcp_* related changes it has >> been on and off by various committers over the past year. If we do have >> a policy of prior review required then it should be made official and >> codified in MAINTAINERS and universally applied to all. > Personally I don't think we need any more anchors attached to people's feet > when developing FreeBSD. > > Mistakes will happen, they will happen in head. Slowing down the process to > eliminate mistakes only works to slow down change and give a false sense of > "fixing stability" when in fact the only thing "stable" is the slowness of > submitting code. Organizing cabals of people to review code is the best way to go. It's an extension of the maintainers concept applied in a better fashion because it encourages several developers to provide input on a series of commits instead of one. I know it's sort of done in some groups [based on commit messages], but it would be nice to have it better formalized and socialized as well. Things like this are helpful for other potential freebsd contributors/developers (like some folks I work with for instance!). An extension of this code review idea would probably be reviewboard. Email based review is doable and a lot of OSS groups use it, but there are some nice points to using a more advanced tool, in particular: 1. Colorized diffs. 2. Various diff niceties (hide white space changes, etc). 3. It does a reasonable job at establishing code context (code block A has moved to B). There are 2 FreeBSD corporate consumers that use it, with at least one other group considering it, and there are probably more groups that using it as well. Plus it integrates well with p4, and does pretty well with git and svn. 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: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
On 11/28/12 7:49 PM, Garrett Cooper wrote: On Nov 28, 2012, at 9:39 AM, Alfred Perlstein wrote: On 11/28/12 12:01 AM, Andre Oppermann wrote: On 28.11.2012 00:59, Robert N. M. Watson wrote: On 27 Nov 2012, at 23:29, Andre Oppermann wrote: Andre.. this breaks incoming connections. TCP is immediately reset and never even gets to the listener process. You need to back out of fix this urgently please. I just found out and fixed it. Sorry for the breakage. I'd like to see a much more thorough use of "Reviewed by:" in socket and TCP-related commits -- this is very sensitive code, and a second pair of eyes is always valuable. Post-commit review is not a substitute. Looking back over similar changes in the socket code over the last two years, I see that almost all have reviewers, so I think it would be reasonable to consider it mandatory for these subsystems at this point. The good news is that we have lots of people with expertise in it. Good to see you becoming more active again. :-) And yes, you have a point there. Yes -- this is only about three weeks old, however; for the prior six-twelve months, I've been fairly non-existent in FreeBSD-land due to outside obligations :-). Just saw that I did indeed send you a review request three weeks ago. ;-) At the end of a rather long email though. Yes, indeed -- no patch was attached, and it followed quite a long e-mail on your plans to rewrite the TCP stack. I'm afraid that went onto the "read this later as time permits" pile as I was at a conference, rather than the fast-path "oh, quickly review this patch" pile. However, simply committing the patch rather than trying a bit harder to find a reviewer isn't the right answer either. To maximise the likelihood of a review, construct an e-mail with a subject line like "Review request: (patch description)", attach the patch, and include a proposed commit message. Yes, and I didn't really expect you to answer (at least quickly) during your FreeBSD hiatus. So it was seeking review by chance. Alas I found and fixed the bug myself within 2.5hrs. While not optimal, a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from the fact that it was mea culpa in any case though. For prior review of kern_socket* and netinet/tcp_* related changes it has been on and off by various committers over the past year. If we do have a policy of prior review required then it should be made official and codified in MAINTAINERS and universally applied to all. Personally I don't think we need any more anchors attached to people's feet when developing FreeBSD. Mistakes will happen, they will happen in head. Slowing down the process to eliminate mistakes only works to slow down change and give a false sense of "fixing stability" when in fact the only thing "stable" is the slowness of submitting code. Organizing cabals of people to review code is the best way to go. It's an extension of the maintainers concept applied in a better fashion because it encourages several developers to provide input on a series of commits instead of one. I know it's sort of done in some groups [based on commit messages], but it would be nice to have it better formalized and socialized as well. Things like this are helpful for other potential freebsd contributors/developers (like some folks I work with for instance!). An extension of this code review idea would probably be reviewboard. Email based review is doable and a lot of OSS groups use it, but there are some nice points to using a more advanced tool, in particular: 1. Colorized diffs. 2. Various diff niceties (hide white space changes, etc). 3. It does a reasonable job at establishing code context (code block A has moved to B). There are 2 FreeBSD corporate consumers that use it, with at least one other group considering it, and there are probably more groups that using it as well. Plus it integrates well with p4, and does pretty well with git and svn. I've seen what happens with large groups, it doesn't scale and basically you wind up with the following type of reviewers: 1) whitespace nazis 2) rubber stampers 3) forever absentee reviewers 4) name nazis The whitespace nazis will never give review approval until you've gone through 600 iterations of the code. Imagine Bruce... but if he wasn't pragmatic and awesome and didn't have other amazing skills. The rubber stampers are friends or people that feel bad about what's happening to you by the whitespace nazis and name nazis so they just glance and approve based on the fact that you're a good guy who has committed to the same place before and usually doesn't break stuff. The forever absentee reviewers are the ones that sign up to review, but never are around to review, or just don't have the time. Eventually an entire subsystem becomes "owned" by absentee reviewers and then it's a huge, delicate and annoying pro
svn commit: r243665 - head/sbin/dump
Author: eadler Date: Thu Nov 29 05:16:50 2012 New Revision: 243665 URL: http://svnweb.freebsd.org/changeset/base/243665 Log: Mark non-returning function as such PR: bin/172978 Approved by: cperciva MFC after:3 days Modified: head/sbin/dump/dump.h Modified: head/sbin/dump/dump.h == --- head/sbin/dump/dump.h Thu Nov 29 03:48:39 2012(r243664) +++ head/sbin/dump/dump.h Thu Nov 29 05:16:50 2012(r243665) @@ -121,7 +121,7 @@ voidtrewind(void); void writerec(char *dp, int isspcl); void Exit(int status) __dead2; -void dumpabort(int signo); +void dumpabort(int signo) __dead2; void dump_getfstab(void); char *rawname(char *cp); ___ 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: r243666 - head/sys/arm/broadcom/bcm2835
Author: gonzo Date: Thu Nov 29 05:46:46 2012 New Revision: 243666 URL: http://svnweb.freebsd.org/changeset/base/243666 Log: Fix hardcoded bpp value Modified: head/sys/arm/broadcom/bcm2835/bcm2835_fb.c Modified: head/sys/arm/broadcom/bcm2835/bcm2835_fb.c == --- head/sys/arm/broadcom/bcm2835/bcm2835_fb.c Thu Nov 29 05:16:50 2012 (r243665) +++ head/sys/arm/broadcom/bcm2835/bcm2835_fb.c Thu Nov 29 05:46:46 2012 (r243666) @@ -751,7 +751,7 @@ bcmfb_putc(video_adapter_t *adp, vm_offs p = sc->font + c*BCMFB_FONT_HEIGHT; addr = (uint8_t *)sc->fb_addr + (row + sc->ymargin)*(sc->stride) - + 3 * (col + sc->xmargin); + + (sc->depth/8) * (col + sc->xmargin); fg = a & 0xf ; bg = (a >> 8) & 0xf; ___ 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: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
>On 11/28/12 7:49 PM, Garrett Cooper wrote: >> I know it's sort of done in some groups [based on commit messages], but it w >ould be nice to have it better formalized and socialized as well. Things like Trying to get too formalized could be counter productive. >> An extension of this code review idea would probably be reviewboard. Email b >ased review is doable and a lot of OSS groups use it, but there are some nice >points to using a more advanced tool, in particular: >> 1. Colorized diffs. FWIW all the Colorized tools I've seen just hurt my eyes. On Wed, 28 Nov 2012 20:17:33 -0800, Alfred Perlstein writes: >I've seen what happens with large groups, it doesn't scale and basically >you wind up with the following type of reviewers: The issues you cite, are the result of taking a good idea too far. Mandating reviews for all changes - does not make sense, and all the ills you describe are natural consequences. But AFAIK that wasn't what Robert was talking about. Some bits of code do warrant extra care when touching. Unless you are the sole author and maintainer (and sometimes even then) getting a 2nd opinion (from someone familiar with the code) makes sense. That does not have to map to a requirement for formal reviewers, review tracking etc - none of which would scale in a volunteer organization anyway. ___ 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: r243627 - head/sys/kern
On 29.11.2012 01:35, Vijay Singh wrote: a sign of poor prior testing and too much trust into the submitter of the patch it wasn't an earth shattering event. Doesn't distract from Andre, I am really quite disappointed to read this from you. The patch I sent you was fine, and has been well tested here. You modified it and made the error. If you wish, I can attach the mail I sent with the patch. Vijay, Your original patch indeed was correct wrt. testing against the head pointer. On Nov. 2 I sent you my (wrongly) modified patch for review and testing. On Nov. 5 you replied that is looks good to you. There is no doubt that I screwed up about everything else on this patch but I did send it for review and you didn't catch the mistake I made. -- Andre ___ 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: r243668 - in head/sys: kern sys
Author: andre
Date: Thu Nov 29 07:30:42 2012
New Revision: 243668
URL: http://svnweb.freebsd.org/changeset/base/243668
Log:
Using a long is the wrong type to represent the realmem and maxmbufmem
variable as they may overflow on i386/PAE and i386 with > 2GB RAM.
Use 64bit quad_t instead. It has broader kernel infrastructure support
with TUNABLE_QUAD_FETCH() and qmin/qmax() than other available types.
Pointed out by: alc, bde
Modified:
head/sys/kern/subr_param.c
head/sys/sys/mbuf.h
Modified: head/sys/kern/subr_param.c
==
--- head/sys/kern/subr_param.c Thu Nov 29 06:26:42 2012(r243667)
+++ head/sys/kern/subr_param.c Thu Nov 29 07:30:42 2012(r243668)
@@ -93,7 +93,7 @@ int ncallout; /* maximum # of timer ev
intnbuf;
intngroups_max;/* max # groups per process */
intnswbuf;
-long maxmbufmem; /* max mbuf memory */
+quad_t maxmbufmem; /* max mbuf memory */
pid_t pid_max = PID_MAX;
long maxswzone; /* max swmeta KVA storage */
long maxbcache; /* max buffer cache KVA storage */
@@ -271,7 +271,7 @@ init_param1(void)
void
init_param2(long physpages)
{
- long realmem;
+ quad_t realmem;
/* Base parameters */
maxusers = MAXUSERS;
@@ -332,10 +332,10 @@ init_param2(long physpages)
* available kernel memory (physical or kmem).
* At most it can be 3/4 of available kernel memory.
*/
- realmem = lmin(physpages * PAGE_SIZE,
+ realmem = qmin(physpages * PAGE_SIZE,
VM_MAX_KERNEL_ADDRESS - VM_MIN_KERNEL_ADDRESS);
maxmbufmem = realmem / 2;
- TUNABLE_LONG_FETCH("kern.maxmbufmem", &maxmbufmem);
+ TUNABLE_QUAD_FETCH("kern.maxmbufmem", &maxmbufmem);
if (maxmbufmem > (realmem / 4) * 3)
maxmbufmem = (realmem / 4) * 3;
Modified: head/sys/sys/mbuf.h
==
--- head/sys/sys/mbuf.h Thu Nov 29 06:26:42 2012(r243667)
+++ head/sys/sys/mbuf.h Thu Nov 29 07:30:42 2012(r243668)
@@ -395,7 +395,7 @@ struct mbstat {
*
* The rest of it is defined in kern/kern_mbuf.c
*/
-extern longmaxmbufmem;
+extern quad_t maxmbufmem;
extern uma_zone_t zone_mbuf;
extern uma_zone_t zone_clust;
extern uma_zone_t zone_pack;
___
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: r243554 - in head/usr.sbin/pkg_install: add create delete info lib updating version
On Wed, Nov 28, 2012 at 06:34:22PM -0800, Stanislav Sedov wrote: > On Wed, 28 Nov 2012 21:03:58 -0500 > Eitan Adler mentioned: > > > On 28 November 2012 20:00, Stanislav Sedov wrote: > > > On Mon, 26 Nov 2012 05:11:07 + (UTC) > > > Eitan Adler mentioned: > > > > > >> Author: eadler > > >> Date: Mon Nov 26 05:11:07 2012 > > >> New Revision: 243554 > > >> URL: http://svnweb.freebsd.org/changeset/base/243554 > > >> > > >> Log: > > >> Provide an option to users to shoot themselves in the foot. > > >> > > > > > > This should probably be a default behavior. It's not good when all these > > > warnings pop up everywhere just because local.sqlite file is present. > > > > Making the foot shooting behavior default defeats the point. > > If local.sqlite exists it almost certainly means that you don't want > > to run the old pkg_ tools. > > For the few correct uses an opt-out option is provided. > > > > Well, it's not entirely true. I did end up with local.sqlite because I > stepped on the landmine of portmgr-pkg becoming the default for some > reason. So I did end up with half of my packages being in the pkgng > sqlite database, and half in standard /var/db/pkg. I had to convert > them back to standard format by hand, but I still have the sqlite database > just in case. > > I guess if you really want to prevent a foot-shooting, you should add > a message to pkgng sayng in all caps e.g. "You are running experimental > package manager and there's no migration plan from pkgng to old pkg > exists." and maybe ask for confirmation. I don't really see how this > message being in pkg_ tools helps to prevent a possible foot-shooting > at all. pkg is no more experimental at all, it is not perfect there are large rooms for improvements, but it is perfectly ready to be used, if you have any concern about some missing "feature" just report it; Concerning a landmine, when you have big flashy lights all over the place: http://lists.freebsd.org/pipermail/freebsd-ports-announce/2012-October/32.html http://lists.freebsd.org/pipermail/freebsd-current/2012-October/037001.html a fanfare playing in front of it saying beware landsmine: http://svnweb.freebsd.org/ports?view=revision&revision=305637 and maps available all over the places to explains where the mine are how to workaround them, or be mine proof: http://svnweb.freebsd.org/ports/head/CHANGES (entry 20121010) http://svnweb.freebsd.org/ports/head/UPDATING (entry 20121010) I'd say in that case it is no more considered as a landmine but just a new safe way. Bapt pgpFmHu1JUeIg.pgp Description: PGP signature
Re: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
On 29 Nov 2012, at 04:17, Alfred Perlstein wrote: > I've seen what happens with large groups, it doesn't scale and basically you > wind up with the following type of reviewers: I think we have to assume best intent here -- the goal of code review in complex critical components is not to eliminate creativity, but to reduce the occurrence of errors and improve the extent to which larger architectural goals are applied and understood in substantive changes. I would much rather we agree by consensus that this sort of code requires mutual code review than chase the "stick" aspect -- as I've said, I feel strongly that code review applied in the last few years has improved code quality markedly in this area, and prevented a lot of problems from being shipped in production kernels. Obviously, you've had some bad experiences with code review, and I have sympathies -- however, if we grant commit bits on the basis of not just technical competence and independence, but also their ability to work well with others, then we need to trust them to perform code reviews in a mature way. And by "code review", I'm incorporating the idea of "design review" as well -- som e collaboration throughout development of non-trivial changes (e.g., in a branch) really does improve not just quality of local areas of code, but the overall design. Robert > > 1) whitespace nazis > 2) rubber stampers > 3) forever absentee reviewers > 4) name nazis > > The whitespace nazis will never give review approval until you've gone > through 600 iterations of the code. Imagine Bruce... but if he wasn't > pragmatic and awesome and didn't have other amazing skills. > > The rubber stampers are friends or people that feel bad about what's > happening to you by the whitespace nazis and name nazis so they just glance > and approve based on the fact that you're a good guy who has committed to the > same place before and usually doesn't break stuff. > > The forever absentee reviewers are the ones that sign up to review, but never > are around to review, or just don't have the time. Eventually an entire > subsystem becomes "owned" by absentee reviewers and then it's a huge, > delicate and annoying process to get that part of the code under some other > list or remove the absentee people. > > Finally the name nazis, these will debate you on if your code should be of > the name kern_foo.c, subr_foo.c or maybe in the new-name-ng should actually > be called kernel_subr_foo.c, but whatever you name something, they will make > you change it, because you are wrong. > > Anyhow, that is my opinion on heavy handed reviews in a large project. > > I've been fortunate enough to work on one project where luckily it was > dominated by rubber stampers, that was silly but awesome because at least we > could get stuff done... > > Unfortunately I then worked on quite a few dominated by a mix of all the four > I mentioned. The result sucked. > > -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: Reviewing before commit and stability minibikeshed (Re: svn commit: r243627 - head/sys/kern)
On 29 Nov 2012, at 07:02, Simon J. Gerraty wrote: > On Wed, 28 Nov 2012 20:17:33 -0800, Alfred Perlstein writes: >> I've seen what happens with large groups, it doesn't scale and basically >> you wind up with the following type of reviewers: > > The issues you cite, are the result of taking a good idea too far. > Mandating reviews for all changes - does not make sense, and all the > ills you describe are natural consequences. > > But AFAIK that wasn't what Robert was talking about. > > Some bits of code do warrant extra care when touching. > Unless you are the sole author and maintainer (and sometimes even then) > getting a 2nd opinion (from someone familiar with the code) makes sense. > > That does not have to map to a requirement for formal reviewers, review > tracking etc - none of which would scale in a volunteer organization > anyway. Yes, exactly so. Perhaps I took too much for granted in my original e-mail, and too much focus was placed on the original commit that triggered the larger thread, so I should be more explicit: - I have in mind developers increasing their practice of seeking code review when touching critical and extremely sensitive subsystems in the network stack -- e.g., socket buffers, inter-layer locking, TCP structure, etc, rather than an edge-case device drivers. - When I say "mandatory", what I mean is a mutual consensus that everyone involved in that area thinks that review is an important part of working on it, and seeks review as part of their development process. - That this obviously applies to non-trivial changes rather than every tweak to a comment. - That it be done sensibly and with good intentions by people who understand the code. - That it occur in a culture of mutual review: if we have four active developers in the socket code, then they all agree to participate in both sides of the process. E.g., I suggest code review to Andre and agree to do some, but in return, he reviews some of my changes. An imbalance in code production and review leads to dissatisfaction for everyone, and is not sustainable. Looking back through svn log on netinet over the last thousand changes, excluding the last month and a half, SCTP, and mechanical/cosmetic changes, we can see that roughly half of changes are "reviewed by" or "discussed with", and there are patterns of mutual review, especially for major changes. I suspect more were reviewed but that it wasn't specifically tagged in commit messages. That doesn't mean errors don't creep in, changes don't have to be backed out, etc, but it has significant benefits. 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"
