From owner-freebsd-arch@FreeBSD.ORG Fri May 1 16:56:39 2015 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 652C5FCE for ; Fri, 1 May 2015 16:56:39 +0000 (UTC) Received: from mail-wg0-x231.google.com (mail-wg0-x231.google.com [IPv6:2a00:1450:400c:c00::231]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id E006510E9 for ; Fri, 1 May 2015 16:56:38 +0000 (UTC) Received: by wgin8 with SMTP id n8so95495050wgi.0 for ; Fri, 01 May 2015 09:56:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=9WFq2dRwwItZbk/UUMZi5StodnW2EEnemXORL6yI3zM=; b=YVkMYEAyAzQDL4zlQBkpdYwNTjTB7hZle2usKFwqmUVPBQ9x52trWCYZQHoDs9DpXU xMranw+o9D3UEE3Eo/02SWxmBfPOfnAemLg9c0DdJsijUnuxlalst4DQ13LSKi0VTjge YzdBlIl0fV2zYtuXRnXowVe5hRcHsLVGVKq7VW5SSDBeqIelE5sqJT2TkIU/vWA26vuj G5kKfLj05DcFE74lXOBo6uBhyYGB+3tdNs2w5rmKDq4zsv8xqIShmLILZJxq7ciKvti5 04+MYY72ahMzpzRMfo55xVgdKTvoscUf5/ZM0galDPIB0/T1Yj7Mzv0UjxiytxC5kO48 wcEA== X-Received: by 10.194.248.132 with SMTP id ym4mr20146995wjc.74.1430499397328; Fri, 01 May 2015 09:56:37 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id nb9sm7428478wic.10.2015.05.01.09.56.35 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Fri, 01 May 2015 09:56:36 -0700 (PDT) Date: Fri, 1 May 2015 18:56:33 +0200 From: Mateusz Guzik To: Bruce Evans Cc: freebsd-arch@freebsd.org Subject: Re: [PATCH 1/2] Generalised support for copy-on-write structures shared by threads. Message-ID: <20150501165633.GA7112@dft-labs.eu> References: <1430188443-19413-1-git-send-email-mjguzik@gmail.com> <1430188443-19413-2-git-send-email-mjguzik@gmail.com> <20150428181802.F1119@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150428181802.F1119@besplex.bde.org> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 May 2015 16:56:39 -0000 On Tue, Apr 28, 2015 at 06:45:01PM +1000, Bruce Evans wrote: > On Tue, 28 Apr 2015, Mateusz Guzik wrote: > >diff --git a/sys/sys/proc.h b/sys/sys/proc.h > >index 64b99fc..f29d796 100644 > >--- a/sys/sys/proc.h > >+++ b/sys/sys/proc.h > >@@ -225,6 +225,7 @@ struct thread { > >/* Cleared during fork1() */ > >#define td_startzero td_flags > > int td_flags; /* (t) TDF_* flags. */ > >+ u_int td_cowgeneration;/* (k) Generation of COW pointers. */ > > int td_inhibitors; /* (t) Why can not run. */ > > int td_pflags; /* (k) Private thread (TDP_*) flags. */ > > int td_dupfd; /* (k) Ret value from fdopen. XXX */ > > This name is so verbose that it messes up the comment indentation. > Yeah, that's crap, but the naming is already inconsistent and verbose. For instance there is td_generation alrady. Is _cowgen variant ok? > >@@ -830,6 +832,11 @@ extern pid_t pid_max; > > KASSERT((p)->p_lock == 0, ("process held")); \ > >} while (0) > > > >+#define PROC_UPDATE_COW(p) do { \ > >+ PROC_LOCK_ASSERT((p), MA_OWNED); \ > >+ p->p_cowgeneration++; \ > > Missing parentheses. Oops, fixed. > > >+} while (0) > >+ > >/* Check whether a thread is safe to be swapped out. */ > >#define thread_safetoswapout(td) ((td)->td_flags & TDF_CANSWAP) > > > >@@ -976,6 +983,10 @@ struct thread *thread_alloc(int pages); > >int thread_alloc_stack(struct thread *, int pages); > >void thread_exit(void) __dead2; > >void thread_free(struct thread *td); > >+void thread_get_cow_proc(struct thread *newtd, struct proc *p); > >+void thread_get_cow(struct thread *newtd, struct thread *td); > >+void thread_free_cow(struct thread *td); > >+void thread_update_cow(struct thread *td); > > Insertion sort errors. > > Namespace errors. I don't like the style of naming things with objects > first and verbs last, but it is good for sorting related objects. Here > the verbs "get" and "free" are in the middle of the objects > "thread_cow_proc" and "thread_cow". Also, shouldn't it be "thread_proc_cow" > (but less verbose, maybe "tpcow"), not "thread_cow_proc", to indicate > that the cow is hung of the proc? I didn't notice the details, but it > makes no sense to hang a proc of a cow :-). > Well all current funcs are named thread_*, so tpcow and the like would be inconsistent. On another look existence of thread_suspend_* suggests thread_cow_* naming. With this putting _proc variant anywhere but at the end also breaks consistency. 'thread_cow_from_proc' would increase verbosity. That said, I would say the patch below is ok enough. diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c index 193d207..cef3221 100644 --- a/sys/amd64/amd64/trap.c +++ b/sys/amd64/amd64/trap.c @@ -257,8 +257,8 @@ trap(struct trapframe *frame) td->td_pticks = 0; td->td_frame = frame; addr = frame->tf_rip; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); switch (type) { case T_PRIVINFLT: /* privileged instruction fault */ diff --git a/sys/arm/arm/trap-v6.c b/sys/arm/arm/trap-v6.c index abafa86..7463d3c 100644 --- a/sys/arm/arm/trap-v6.c +++ b/sys/arm/arm/trap-v6.c @@ -394,8 +394,8 @@ abort_handler(struct trapframe *tf, int prefetch) p = td->td_proc; if (usermode) { td->td_pticks = 0; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); } /* Invoke the appropriate handler, if necessary. */ diff --git a/sys/arm/arm/trap.c b/sys/arm/arm/trap.c index 0f142ce..d7fb73a 100644 --- a/sys/arm/arm/trap.c +++ b/sys/arm/arm/trap.c @@ -214,8 +214,8 @@ abort_handler(struct trapframe *tf, int type) if (user) { td->td_pticks = 0; td->td_frame = tf; - if (td->td_ucred != td->td_proc->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != td->td_proc->p_cowgen) + thread_cow_update(td); } /* Grab the current pcb */ @@ -644,8 +644,8 @@ prefetch_abort_handler(struct trapframe *tf) if (TRAP_USERMODE(tf)) { td->td_frame = tf; - if (td->td_ucred != td->td_proc->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != td->td_proc->p_cowgen) + thread_cow_update(td); } fault_pc = tf->tf_pc; if (td->td_md.md_spinlock_count == 0) { diff --git a/sys/i386/i386/trap.c b/sys/i386/i386/trap.c index d783a2b..b118e73 100644 --- a/sys/i386/i386/trap.c +++ b/sys/i386/i386/trap.c @@ -306,8 +306,8 @@ trap(struct trapframe *frame) td->td_pticks = 0; td->td_frame = frame; addr = frame->tf_eip; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); switch (type) { case T_PRIVINFLT: /* privileged instruction fault */ diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c index b77b788..e0042e9 100644 --- a/sys/kern/init_main.c +++ b/sys/kern/init_main.c @@ -522,8 +522,6 @@ proc0_init(void *dummy __unused) #ifdef MAC mac_cred_create_swapper(newcred); #endif - td->td_ucred = crhold(newcred); - /* Create sigacts. */ p->p_sigacts = sigacts_alloc(); @@ -555,6 +553,10 @@ proc0_init(void *dummy __unused) p->p_limit->pl_rlimit[RLIMIT_MEMLOCK].rlim_max = pageablemem; p->p_cpulimit = RLIM_INFINITY; + PROC_LOCK(p); + thread_cow_get_proc(td, p); + PROC_UNLOCK(p); + /* Initialize resource accounting structures. */ racct_create(&p->p_racct); @@ -842,10 +844,10 @@ create_init(const void *udata __unused) audit_cred_proc1(newcred); #endif proc_set_cred(initproc, newcred); + cred_update_thread(FIRST_THREAD_IN_PROC(initproc)); PROC_UNLOCK(initproc); sx_xunlock(&proctree_lock); crfree(oldcred); - cred_update_thread(FIRST_THREAD_IN_PROC(initproc)); cpu_set_fork_handler(FIRST_THREAD_IN_PROC(initproc), start_init, NULL); } SYSINIT(init, SI_SUB_CREATE_INIT, SI_ORDER_FIRST, create_init, NULL); diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c index c3dd792..0dfecff 100644 --- a/sys/kern/kern_fork.c +++ b/sys/kern/kern_fork.c @@ -496,7 +496,6 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, p2->p_swtick = ticks; if (p1->p_flag & P_PROFIL) startprofclock(p2); - td2->td_ucred = crhold(p2->p_ucred); if (flags & RFSIGSHARE) { p2->p_sigacts = sigacts_hold(p1->p_sigacts); @@ -526,6 +525,8 @@ do_fork(struct thread *td, int flags, struct proc *p2, struct thread *td2, */ lim_fork(p1, p2); + thread_cow_get_proc(td2, p2); + pstats_fork(p1->p_stats, p2->p_stats); PROC_UNLOCK(p1); diff --git a/sys/kern/kern_kthread.c b/sys/kern/kern_kthread.c index ee94de0..863bbc6 100644 --- a/sys/kern/kern_kthread.c +++ b/sys/kern/kern_kthread.c @@ -289,7 +289,7 @@ kthread_add(void (*func)(void *), void *arg, struct proc *p, cpu_set_fork_handler(newtd, func, arg); newtd->td_pflags |= TDP_KTHREAD; - newtd->td_ucred = crhold(p->p_ucred); + thread_cow_get_proc(newtd, p); /* this code almost the same as create_thread() in kern_thr.c */ p->p_flag |= P_HADTHREADS; diff --git a/sys/kern/kern_prot.c b/sys/kern/kern_prot.c index 9c49f71..b531763 100644 --- a/sys/kern/kern_prot.c +++ b/sys/kern/kern_prot.c @@ -1946,9 +1946,8 @@ cred_update_thread(struct thread *td) p = td->td_proc; cred = td->td_ucred; - PROC_LOCK(p); + PROC_LOCK_ASSERT(p, MA_OWNED); td->td_ucred = crhold(p->p_ucred); - PROC_UNLOCK(p); if (cred != NULL) crfree(cred); } @@ -1987,6 +1986,8 @@ proc_set_cred(struct proc *p, struct ucred *newcred) oldcred = p->p_ucred; p->p_ucred = newcred; + if (newcred != NULL) + PROC_UPDATE_COW(p); return (oldcred); } diff --git a/sys/kern/kern_syscalls.c b/sys/kern/kern_syscalls.c index dada746..3d3df01 100644 --- a/sys/kern/kern_syscalls.c +++ b/sys/kern/kern_syscalls.c @@ -31,6 +31,8 @@ __FBSDID("$FreeBSD$"); #include #include #include +#include +#include #include #include #include diff --git a/sys/kern/kern_thr.c b/sys/kern/kern_thr.c index 6911bb97..a53bd25 100644 --- a/sys/kern/kern_thr.c +++ b/sys/kern/kern_thr.c @@ -228,13 +228,13 @@ create_thread(struct thread *td, mcontext_t *ctx, bcopy(&td->td_startcopy, &newtd->td_startcopy, __rangeof(struct thread, td_startcopy, td_endcopy)); newtd->td_proc = td->td_proc; - newtd->td_ucred = crhold(td->td_ucred); + thread_cow_get(newtd, td); if (ctx != NULL) { /* old way to set user context */ error = set_mcontext(newtd, ctx); if (error != 0) { + thread_cow_free(newtd); thread_free(newtd); - crfree(td->td_ucred); goto fail; } } else { @@ -246,8 +246,8 @@ create_thread(struct thread *td, mcontext_t *ctx, /* Setup user TLS address and TLS pointer register. */ error = cpu_set_user_tls(newtd, tls_base); if (error != 0) { + thread_cow_free(newtd); thread_free(newtd); - crfree(td->td_ucred); goto fail; } } diff --git a/sys/kern/kern_thread.c b/sys/kern/kern_thread.c index 0a93dbd..063dfe9 100644 --- a/sys/kern/kern_thread.c +++ b/sys/kern/kern_thread.c @@ -324,8 +324,7 @@ thread_reap(void) mtx_unlock_spin(&zombie_lock); while (td_first) { td_next = TAILQ_NEXT(td_first, td_slpq); - if (td_first->td_ucred) - crfree(td_first->td_ucred); + thread_cow_free(td_first); thread_free(td_first); td_first = td_next; } @@ -381,6 +380,44 @@ thread_free(struct thread *td) uma_zfree(thread_zone, td); } +void +thread_cow_get_proc(struct thread *newtd, struct proc *p) +{ + + PROC_LOCK_ASSERT(p, MA_OWNED); + newtd->td_ucred = crhold(p->p_ucred); + newtd->td_cowgen = p->p_cowgen; +} + +void +thread_cow_get(struct thread *newtd, struct thread *td) +{ + + newtd->td_ucred = crhold(td->td_ucred); + newtd->td_cowgen = td->td_cowgen; +} + +void +thread_cow_free(struct thread *td) +{ + + if (td->td_ucred) + crfree(td->td_ucred); +} + +void +thread_cow_update(struct thread *td) +{ + struct proc *p; + + p = td->td_proc; + PROC_LOCK(p); + if (td->td_ucred != p->p_ucred) + cred_update_thread(td); + td->td_cowgen = p->p_cowgen; + PROC_UNLOCK(p); +} + /* * Discard the current thread and exit from its context. * Always called with scheduler locked. @@ -518,7 +555,7 @@ thread_wait(struct proc *p) cpuset_rel(td->td_cpuset); td->td_cpuset = NULL; cpu_thread_clean(td); - crfree(td->td_ucred); + thread_cow_free(td); thread_reap(); /* check for zombie threads etc. */ } diff --git a/sys/kern/subr_syscall.c b/sys/kern/subr_syscall.c index 1bf78b8..070ba28 100644 --- a/sys/kern/subr_syscall.c +++ b/sys/kern/subr_syscall.c @@ -61,8 +61,8 @@ syscallenter(struct thread *td, struct syscall_args *sa) p = td->td_proc; td->td_pticks = 0; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); if (p->p_flag & P_TRACED) { traced = 1; PROC_LOCK(p); diff --git a/sys/kern/subr_trap.c b/sys/kern/subr_trap.c index 93f7557..e5e55dd 100644 --- a/sys/kern/subr_trap.c +++ b/sys/kern/subr_trap.c @@ -213,8 +213,8 @@ ast(struct trapframe *framep) thread_unlock(td); PCPU_INC(cnt.v_trap); - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); if (td->td_pflags & TDP_OWEUPC && p->p_flag & P_PROFIL) { addupc_task(td, td->td_profil_addr, td->td_profil_ticks); td->td_profil_ticks = 0; diff --git a/sys/powerpc/powerpc/trap.c b/sys/powerpc/powerpc/trap.c index 0ceb170..bfbd94d 100644 --- a/sys/powerpc/powerpc/trap.c +++ b/sys/powerpc/powerpc/trap.c @@ -196,8 +196,8 @@ trap(struct trapframe *frame) if (user) { td->td_pticks = 0; td->td_frame = frame; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); /* User Mode Traps */ switch (type) { diff --git a/sys/sparc64/sparc64/trap.c b/sys/sparc64/sparc64/trap.c index b4f0e27..e9917e5 100644 --- a/sys/sparc64/sparc64/trap.c +++ b/sys/sparc64/sparc64/trap.c @@ -277,8 +277,8 @@ trap(struct trapframe *tf) td->td_pticks = 0; td->td_frame = tf; addr = tf->tf_tpc; - if (td->td_ucred != p->p_ucred) - cred_update_thread(td); + if (td->td_cowgen != p->p_cowgen) + thread_cow_update(td); switch (tf->tf_type) { case T_DATA_MISS: diff --git a/sys/sys/proc.h b/sys/sys/proc.h index 64b99fc..5033957 100644 --- a/sys/sys/proc.h +++ b/sys/sys/proc.h @@ -225,6 +225,7 @@ struct thread { /* Cleared during fork1() */ #define td_startzero td_flags int td_flags; /* (t) TDF_* flags. */ + u_int td_cowgen; /* (k) Generation of COW pointers. */ int td_inhibitors; /* (t) Why can not run. */ int td_pflags; /* (k) Private thread (TDP_*) flags. */ int td_dupfd; /* (k) Ret value from fdopen. XXX */ @@ -531,6 +532,7 @@ struct proc { pid_t p_oppid; /* (c + e) Save ppid in ptrace. XXX */ struct vmspace *p_vmspace; /* (b) Address space. */ u_int p_swtick; /* (c) Tick when swapped in or out. */ + u_int p_cowgen; /* (c) Generation of COW pointers. */ struct itimerval p_realtimer; /* (c) Alarm timer. */ struct rusage p_ru; /* (a) Exit information. */ struct rusage_ext p_rux; /* (cu) Internal resource usage. */ @@ -830,6 +832,11 @@ extern pid_t pid_max; KASSERT((p)->p_lock == 0, ("process held")); \ } while (0) +#define PROC_UPDATE_COW(p) do { \ + PROC_LOCK_ASSERT((p), MA_OWNED); \ + (p)->p_cowgen++; \ +} while (0) + /* Check whether a thread is safe to be swapped out. */ #define thread_safetoswapout(td) ((td)->td_flags & TDF_CANSWAP) @@ -974,6 +981,10 @@ void cpu_thread_swapin(struct thread *); void cpu_thread_swapout(struct thread *); struct thread *thread_alloc(int pages); int thread_alloc_stack(struct thread *, int pages); +void thread_cow_get_proc(struct thread *newtd, struct proc *p); +void thread_cow_get(struct thread *newtd, struct thread *td); +void thread_cow_free(struct thread *td); +void thread_cow_update(struct thread *td); void thread_exit(void) __dead2; void thread_free(struct thread *td); void thread_link(struct thread *td, struct proc *p); -- Mateusz Guzik