Date: Thu, 03 Oct 2002 16:41:46 +0200 From: Poul-Henning Kamp <phk@critter.freebsd.dk> To: Stefan Farfeleder <e0026813@stud3.tuwien.ac.at> Cc: current@FreeBSD.ORG Subject: Re: Junior Kernel Hacker page updated... Message-ID: <4860.1033656106@critter.freebsd.dk> In-Reply-To: Your message of "Wed, 02 Oct 2002 23:40:28 %2B0200." <20021002214028.GA94673@frog.fafoe>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Stefan, I tried this patch and it paniced my (almost-) current machine with a pagefault in the kqueue code: Bravo! I can see that there is some amount of #ifdef stuff in your patch, in light of that, would it be possible to make an #ifdef'ed version of your patch which we could commit ? That way we give the kqueue hackers a good testcase, and we can easily enable when they have solved the problem. Poul-Henning In message <20021002214028.GA94673@frog.fafoe>, Stefan Farfeleder writes: > >--VS++wcV0S1rZb1Fb >Content-Type: text/plain; charset=us-ascii >Content-Disposition: inline > >On Sat, Sep 14, 2002 at 12:17:53PM +0200, Poul-Henning Kamp wrote: >> >> This is just to note that I have updated the JKH page with a lot of new >> stuff, so if your coding-pencil itches: >> >> http://people.freebsd.org/~phk/TODO/ > >|Make -j improvement >| >|make(1) with -j option uses a select loop to wait for events, and every >|100msec it drops out to look for processes exited etc. A pure "make >|buildworld" on a single-CPU machine is up to 25% faster that the best >|"make -j N buildworld" time on the same hardware. Changing to timeout >|to be 10msec improves things about 10%. >|I think that make(1) should use kqueue(2) instead, since that would >|eliminate the need for timeouts. > >Ok, here's what I came up with. However, with the patch applied, each >'make buildworld' on a SMP machine throws tons of > >/freebsd/current/src/sys/vm/uma_core.c:1307: could sleep with "filedesc structure" locked from /freebsd/current/src/sys/kern/kern_event.c:959 > >at me and freezes badly at some point (no breaking into ddb possible). >This is totally repeatable. Is anybody able to reproduce (and maybe >fix) this? > >Regards, >Stefan Farfeleder > >--VS++wcV0S1rZb1Fb >Content-Type: text/plain; charset=us-ascii >Content-Disposition: attachment; filename="make.diff" > >Index: job.c >=================================================================== >RCS file: /home/ncvs/src/usr.bin/make/job.c,v >retrieving revision 1.43 >diff -u -r1.43 job.c >--- job.c 29 Sep 2002 00:20:28 -0000 1.43 >+++ job.c 2 Oct 2002 21:34:51 -0000 >@@ -64,9 +64,8 @@ > * Job_CatchOutput Print any output our children have produced. > * Should also be called fairly frequently to > * keep the user informed of what's going on. >- * If no output is waiting, it will block for >- * a time given by the SEL_* constants, below, >- * or until output is ready. >+ * If no output is waiting, it will block until >+ * a child terminates or until output is ready. > * > * Job_Init Called to intialize this module. in addition, > * any commands attached to the .BEGIN target >@@ -107,6 +106,8 @@ > #include <sys/file.h> > #include <sys/time.h> > #include <sys/wait.h> >+#include <sys/event.h> >+#include <sys/time.h> > #include <err.h> > #include <errno.h> > #include <fcntl.h> >@@ -237,8 +238,7 @@ > * (2) a job can only be run locally, but > * nLocal equals maxLocal */ > #ifndef RMT_WILL_WATCH >-static fd_set outputs; /* Set of descriptors of pipes connected to >- * the output channels of children */ >+static int kqfd; /* File descriptor obtained by kqueue() */ > #endif > > STATIC GNode *lastNode; /* The node for which output was most recently >@@ -692,8 +692,6 @@ > if (usePipes) { > #ifdef RMT_WILL_WATCH > Rmt_Ignore(job->inPipe); >-#else >- FD_CLR(job->inPipe, &outputs); > #endif > if (job->outPipe != job->inPipe) { > (void) close(job->outPipe); >@@ -1265,14 +1263,25 @@ > /* > * The first time a job is run for a node, we set the current > * position in the buffer to the beginning and mark another >- * stream to watch in the outputs mask >+ * stream to watch in the outputs mask. The termination of this >+ * job and the availability of new data in the pipe are >+ * registered in the kqueue. > */ >+ struct kevent kev[2]; >+ > job->curPos = 0; > > #ifdef RMT_WILL_WATCH > Rmt_Watch(job->inPipe, JobLocalInput, job); > #else >- FD_SET(job->inPipe, &outputs); >+ EV_SET(&kev[0], job->inPipe, EVFILT_READ, EV_ADD, 0, 0, job); >+ EV_SET(&kev[1], job->pid, EVFILT_PROC, EV_ADD | EV_ONESHOT, >+ NOTE_EXIT, 0, NULL); >+ if (kevent(kqfd, kev, 2, NULL, 0, NULL) != 0) { >+ /* kevent() will fail if the job is already finished */ >+ if (errno != EBADF && errno != ESRCH) >+ Punt("kevent: %s", strerror(errno)); >+ } > #endif /* RMT_WILL_WATCH */ > } > >@@ -2229,12 +2238,12 @@ > Job_CatchOutput() > { > int nfds; >- struct timeval timeout; >- fd_set readfds; >- LstNode ln; >- Job *job; > #ifdef RMT_WILL_WATCH > int pnJobs; /* Previous nJobs */ >+#else >+ struct kevent kev[4]; /* 4 is somewhat arbitrary */ >+ size_t kevsize = sizeof(kev) / sizeof(kev[0]); >+ int i; > #endif > > (void) fflush(stdout); >@@ -2262,25 +2271,20 @@ > } > #else > if (usePipes) { >- readfds = outputs; >- timeout.tv_sec = SEL_SEC; >- timeout.tv_usec = SEL_USEC; >- >- if ((nfds = select(FD_SETSIZE, &readfds, (fd_set *) 0, >- (fd_set *) 0, &timeout)) <= 0) >- return; >- else { >- if (Lst_Open(jobs) == FAILURE) { >- Punt("Cannot open job table"); >- } >- while (nfds && (ln = Lst_Next(jobs)) != NULL) { >- job = (Job *) Lst_Datum(ln); >- if (FD_ISSET(job->inPipe, &readfds)) { >- JobDoOutput(job, FALSE); >- nfds -= 1; >+ if ((nfds = kevent(kqfd, NULL, 0, kev, kevsize, NULL)) == -1) { >+ Punt("kevent: %s", strerror(errno)); >+ } else { >+ for (i = 0; i < nfds; i++) { >+ switch (kev[i].filter) { >+ case EVFILT_READ: >+ JobDoOutput(kev[i].udata, FALSE); >+ break; >+ case EVFILT_PROC: >+ /* Just wake up and let Job_CatchChildren() collect the >+ * terminated job. */ >+ break; > } > } >- Lst_Close(jobs); > } > } > #endif /* RMT_WILL_WATCH */ >@@ -2408,6 +2412,13 @@ > } > if (signal(SIGWINCH, SIG_IGN) != SIG_IGN) { > (void) signal(SIGWINCH, JobPassSig); >+ } >+#endif >+ >+#ifndef RMT_WILL_WATCH >+ >+ if ((kqfd = kqueue()) == -1) { >+ Punt("kqueue: %s", strerror(errno)); > } > #endif > >Index: job.h >=================================================================== >RCS file: /home/ncvs/src/usr.bin/make/job.h,v >retrieving revision 1.18 >diff -u -r1.18 job.h >--- job.h 26 Sep 2002 01:39:22 -0000 1.18 >+++ job.h 1 Oct 2002 16:31:10 -0000 >@@ -50,14 +50,6 @@ > > #define TMPPAT "/tmp/makeXXXXXXXXXX" > >-/* >- * The SEL_ constants determine the maximum amount of time spent in select >- * before coming out to see if a child has finished. SEL_SEC is the number of >- * seconds and SEL_USEC is the number of micro-seconds >- */ >-#define SEL_SEC 0 >-#define SEL_USEC 100000 >- > > /*- > * Job Table definitions. > >--VS++wcV0S1rZb1Fb-- > -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 phk@FreeBSD.ORG | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-current" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4860.1033656106>