Skip site navigation (1)Skip section navigation (2)
Date:      Fri, 1 May 2015 18:56:33 +0200
From:      Mateusz Guzik <mjguzik@gmail.com>
To:        Bruce Evans <brde@optusnet.com.au>
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>
In-Reply-To: <20150428181802.F1119@besplex.bde.org>
References:  <1430188443-19413-1-git-send-email-mjguzik@gmail.com> <1430188443-19413-2-git-send-email-mjguzik@gmail.com> <20150428181802.F1119@besplex.bde.org>

next in thread | previous in thread | raw e-mail | index | archive | help
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 <sys/kernel.h>
 #include <sys/lock.h>
 #include <sys/module.h>
+#include <sys/mutex.h>
+#include <sys/proc.h>
 #include <sys/sx.h>
 #include <sys/syscall.h>
 #include <sys/sysent.h>
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 <mjguzik gmail.com>



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150501165633.GA7112>