From owner-cvs-src@FreeBSD.ORG Tue Jan 24 00:16:16 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id B2BFA16A41F; Tue, 24 Jan 2006 00:16:16 +0000 (GMT) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id 68E3B43D53; Tue, 24 Jan 2006 00:16:16 +0000 (GMT) (envelope-from davidxu@freebsd.org) Received: from [127.0.0.1] (root@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id k0O0GDn8047633; Tue, 24 Jan 2006 00:16:14 GMT (envelope-from davidxu@freebsd.org) Message-ID: <43D571C7.2070203@freebsd.org> Date: Tue, 24 Jan 2006 08:16:07 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.7.12) Gecko/20060107 X-Accept-Language: en-us, en MIME-Version: 1.0 To: John Baldwin References: <200601220559.k0M5xS5j026930@repoman.freebsd.org> <200601231318.54234.jhb@freebsd.org> In-Reply-To: <200601231318.54234.jhb@freebsd.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern syscalls.master vfs_aio.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jan 2006 00:16:16 -0000 John Baldwin wrote: >On Sunday 22 January 2006 00:59, David Xu wrote: > > >>davidxu 2006-01-22 05:59:28 UTC >> >> FreeBSD src repository >> >> Modified files: >> sys/kern syscalls.master vfs_aio.c >> Log: >> Make aio code MP safe. >> >> > >Thanks for doing this. This is something I had sort of been working on but >hadn't gotten too in a while. (I don't think I had many uncommitted diffs, I >think mostly I had just done the fget() and TAILQ_FOREACH_SAFE() cleanups I >committed earlier.) One thing I would appreciate is if you could add some >comments explaining the locking strategy and annotating which fields are >protected by which locks similar to sys/proc.h, etc. I've attached my >current set of diffs in case you wanted to look at them. > > I will add locking annotating, in fact, I will do it today, it is morning here, I have slept, so now I have interest to work on it again : -) > > >------------------------------------------------------------------------ > >--- //depot/projects/smpng/sys/kern/vfs_aio.c 2006/01/17 17:16:05 >+++ //depot/user/jhb/proc/kern/vfs_aio.c 2006/01/17 17:49:55 >@@ -108,6 +108,8 @@ > #define AIOD_LIFETIME_DEFAULT (30 * hz) > #endif > >+static int aiod_creation_in_progress; >+ > static SYSCTL_NODE(_vfs, OID_AUTO, aio, CTLFLAG_RW, 0, "Async IO management"); > > static int max_aio_procs = MAX_AIO_PROCS; >@@ -266,7 +268,7 @@ > static void aio_onceonly(void); > static int aio_free_entry(struct aiocblist *aiocbe); > static void aio_process(struct aiocblist *aiocbe); >-static int aio_newproc(void); >+static int aio_newproc(int always); > static int aio_aqueue(struct thread *td, struct aiocb *job, int type, > int osigev); > static void aio_physwakeup(struct buf *bp); >@@ -411,9 +413,17 @@ > error = kqueue_del_filteropts(EVFILT_AIO); > if (error) > return error; >+ error = kqueue_del_filteropts(EVFILT_LIO); >+ if (error) >+ return error; > > async_io_version = 0; > aio_swake = NULL; >+ uma_zdestroy(kaio_zone); >+ uma_zdestroy(aiop_zone); >+ uma_zdestroy(aiocb_zone); >+ uma_zdestroy(aiol_zone); >+ uma_zdestroy(aiolio_zone); > EVENTHANDLER_DEREGISTER(process_exit, exit_tag); > EVENTHANDLER_DEREGISTER(process_exec, exec_tag); > mtx_destroy(&aio_freeproc_mtx); >@@ -456,8 +466,10 @@ > uma_zfree(kaio_zone, ki); > } > >+ mtx_lock(&aio_freeproc_mtx); > while (num_aio_procs < target_aio_procs) >- aio_newproc(); >+ aio_newproc(0); >+ mtx_unlock(&aio_freeproc_mtx); > } > > static int >@@ -901,8 +913,6 @@ > struct proc *curcp, *mycp, *userp; > struct vmspace *myvm, *tmpvm; > struct thread *td = curthread; >- struct pgrp *newpgrp; >- struct session *newsess; > > /* > * Local copies of curproc (cp) and vmspace (myvm) >@@ -918,16 +928,8 @@ > */ > aiop = uma_zalloc(aiop_zone, M_WAITOK); > aiop->aiothread = td; >- aiop->aiothreadflags |= AIOP_FREE; > > /* >- * Place thread (lightweight process) onto the AIO free thread list. >- */ >- mtx_lock(&aio_freeproc_mtx); >- TAILQ_INSERT_HEAD(&aio_freeproc, aiop, list); >- mtx_unlock(&aio_freeproc_mtx); >- >- /* > * Get rid of our current filedescriptors. AIOD's don't need any > * filedescriptors, except as temporarily inherited from the client. > */ >@@ -936,15 +938,16 @@ > > mtx_unlock(&Giant); > /* The daemon resides in its own pgrp. */ >- MALLOC(newpgrp, struct pgrp *, sizeof(struct pgrp), M_PGRP, >- M_WAITOK | M_ZERO); >- MALLOC(newsess, struct session *, sizeof(struct session), M_SESSION, >- M_WAITOK | M_ZERO); >+ setsid(td, NULL); > >- sx_xlock(&proctree_lock); >- enterpgrp(mycp, mycp->p_pid, newpgrp, newsess); >- sx_xunlock(&proctree_lock); >- mtx_lock(&Giant); >+ /* >+ * Place thread (lightweight process) onto the AIO free thread list. >+ */ >+ mtx_lock(&aio_freeproc_mtx); >+ num_aio_procs++; >+ aiop->aiothreadflags |= AIOP_FREE; >+ TAILQ_INSERT_HEAD(&aio_freeproc, aiop, list); >+ mtx_unlock(&aio_freeproc_mtx); > > /* > * Wakeup parent process. (Parent sleeps to keep from blasting away >@@ -952,6 +955,7 @@ > */ > wakeup(mycp); > >+ mtx_lock(&aio_freeproc_mtx); > for (;;) { > /* > * curcp is the current daemon process context. >@@ -962,7 +966,6 @@ > /* > * Take daemon off of free queue > */ >- mtx_lock(&aio_freeproc_mtx); > if (aiop->aiothreadflags & AIOP_FREE) { > TAILQ_REMOVE(&aio_freeproc, aiop, list); > aiop->aiothreadflags &= ~AIOP_FREE; >@@ -972,6 +975,7 @@ > /* > * Check for jobs. > */ >+ mtx_lock(&Giant); > while ((aiocbe = aio_selectjob(aiop)) != NULL) { > cb = &aiocbe->uaiocb; > userp = aiocbe->userproc; >@@ -1054,6 +1058,7 @@ > > curcp = mycp; > } >+ mtx_unlock(&Giant); > > mtx_lock(&aio_freeproc_mtx); > TAILQ_INSERT_HEAD(&aio_freeproc, aiop, list); >@@ -1063,18 +1068,17 @@ > * If daemon is inactive for a long time, allow it to exit, > * thereby freeing resources. > */ >- if (msleep(aiop->aiothread, &aio_freeproc_mtx, PDROP | PRIBIO, >+ if (msleep(aiop->aiothread, &aio_freeproc_mtx, PRIBIO, > "aiordy", aiod_lifetime)) { > s = splnet(); > if (TAILQ_EMPTY(&aio_jobs)) { >- mtx_lock(&aio_freeproc_mtx); > if ((aiop->aiothreadflags & AIOP_FREE) && > (num_aio_procs > target_aio_procs)) { > TAILQ_REMOVE(&aio_freeproc, aiop, list); >+ num_aio_procs--; > mtx_unlock(&aio_freeproc_mtx); > splx(s); > uma_zfree(aiop_zone, aiop); >- num_aio_procs--; > #ifdef DIAGNOSTIC > if (mycp->p_vmspace->vm_refcnt <= 1) { > printf("AIOD: bad vm refcnt for" >@@ -1084,7 +1088,6 @@ > #endif > kthread_exit(0); > } >- mtx_unlock(&aio_freeproc_mtx); > } > splx(s); > } >@@ -1096,24 +1099,36 @@ > * AIO daemon modifies its environment itself. > */ > static int >-aio_newproc(void) >+aio_newproc(int always) > { >- int error; > struct proc *p; >+ int error, n; > >+ mtx_assert(&aio_freeproc_mtx, MA_OWNED); >+ n = num_aio_procs; >+ while (aiod_creation_in_progress) { >+ error = msleep(&aiod_creation_in_progress, &aio_freeproc_mtx, >+ PZERO, "aionew", aiod_timeout); >+ if (error || !always) >+ return (error); >+ } >+ aiod_creation_in_progress = 1; >+ mtx_unlock(&aio_freeproc_mtx); >+ > error = kthread_create(aio_daemon, curproc, &p, RFNOWAIT, 0, "aiod%d", >- num_aio_procs); >- if (error) >- return (error); >+ n); > >- /* >- * Wait until daemon is started, but continue on just in case to >- * handle error conditions. >- */ >- error = tsleep(p, PZERO, "aiosta", aiod_timeout); >+ mtx_lock(&aio_freeproc_mtx); >+ if (error == 0) >+ /* >+ * Wait until daemon is started, but continue on just in >+ * case to handle error conditions. >+ */ >+ error = msleep(p, &aio_freeproc_mtx, PZERO, "aiosta", >+ aiod_timeout); > >- num_aio_procs++; >- >+ aiod_creation_in_progress = 0; >+ wakeup(&aiod_creation_in_progress); > return (error); > } > >@@ -1597,9 +1612,7 @@ > ((ki->kaio_active_count + num_aio_resv_start) < > ki->kaio_maxactive_count)) { > num_aio_resv_start++; >- mtx_unlock(&aio_freeproc_mtx);ing >- error = aio_newproc(); >- mtx_lock(&aio_freeproc_mtx); >+ error = aio_newproc(1); > num_aio_resv_start--; > if (error) > goto retryproc; > > I will check my code to see if I missed something and will integrate your code if it is needed. Thank you!