[PATCH 0/2] generalised cow per-thread structs

From: Mateusz Guzik <***@freebsd.org>

Previously td_ucred was managed by comparing it to struct proc's version
on kernel<->userspace boundary.

Now a dedicated counter is introduced instead which makes it possible to
treat more structures this way without adding more tests for the common
case (no change).
---
sys/amd64/amd64/trap.c | 4 +--
sys/arm/arm/trap-v6.c | 4 +--
sys/arm/arm/trap.c | 11 ++++----
sys/i386/i386/trap.c | 4 +--
sys/kern/init_main.c | 8 +++---
sys/kern/kern_fork.c | 3 ++-
sys/kern/kern_kthread.c | 2 +-
sys/kern/kern_prot.c | 5 ++--
sys/kern/kern_syscalls.c | 2 ++
sys/kern/kern_thr.c | 6 ++---
sys/kern/kern_thread.c | 43 +++++++++++++++++++++++++++++---
sys/kern/subr_syscall.c | 4 +--
sys/kern/subr_trap.c | 4 +--
sys/powerpc/powerpc/trap.c | 4 +--
sys/sparc64/sparc64/trap.c | 4 +--
sys/sys/proc.h | 11 ++++++++
17 files changed, 86 insertions(+), 33 deletions(-)

diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index 193d207..1883727 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(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..f521785 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(td);
}

/* Invoke the appropriate handler, if necessary. */
diff --git a/sys/arm/arm/trap.c b/sys/arm/arm/trap.c
index 0f142ce..36faac2 100644
--- a/sys/arm/arm/trap.c
+++ b/sys/arm/arm/trap.c
@@ -214,9 +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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(td);
}
/* Grab the current pcb */
pcb = td->td_pcb;
@@ -644,8 +643,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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(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..41e62db 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(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..97e5878 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_get_cow_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..d04c3e3 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_get_cow_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..0614d89 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_get_cow_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 d5f1ce6..242e4dd 100644
--- a/sys/kern/kern_thr.c
+++ b/sys/kern/kern_thr.c
@@ -226,13 +226,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_get_cow(newtd, td);

if (ctx != NULL) { /* old way to set user context */
error = set_mcontext(newtd, ctx);
if (error != 0) {
+ thread_free_cow(newtd);
thread_free(newtd);
- crfree(td->td_ucred);
goto fail;
}
} else {
@@ -244,8 +244,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_free_cow(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..df8511b 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_free_cow(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_get_cow_proc(struct thread *newtd, struct proc *p)
+{
+
+ PROC_LOCK_ASSERT(p, MA_OWNED);
+ newtd->td_ucred = crhold(p->p_ucred);
+ newtd->td_cowgeneration = p->p_cowgeneration;
+}
+
+void
+thread_get_cow(struct thread *newtd, struct thread *td)
+{
+
+ newtd->td_ucred = crhold(td->td_ucred);
+ newtd->td_cowgeneration = td->td_cowgeneration;
+}
+
+void
+thread_free_cow(struct thread *td)
+{
+
+ if (td->td_ucred)
+ crfree(td->td_ucred);
+}
+
+void
+thread_update_cow(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_cowgeneration = p->p_cowgeneration;
+ 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_free_cow(td);
thread_reap(); /* check for zombie threads etc. */
}

diff --git a/sys/kern/subr_syscall.c b/sys/kern/subr_syscall.c
index 1bf78b8..8fdb828 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(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 cfc3ed7..e055e54 100644
--- a/sys/kern/subr_trap.c
+++ b/sys/kern/subr_trap.c
@@ -219,8 +219,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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(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..007752c 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(td);

/* User Mode Traps */
switch (type) {
diff --git a/sys/sparc64/sparc64/trap.c b/sys/sparc64/sparc64/trap.c
index b4f0e27..54c1ebe 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(td);

switch (tf->tf_type) {
case T_DATA_MISS:
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 */
@@ -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_cowgeneration;/* (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_cowgeneration++; \
+} 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);
void thread_link(struct thread *td, struct proc *p);
void thread_reap(void);
int thread_single(struct proc *p, int how);

--
2.3.6

Bruce Evans

2015-04-28 08:45:01 UTC

Post by Mateusz Guzik
diff --git a/sys/amd64/amd64/trap.c b/sys/amd64/amd64/trap.c
index 193d207..1883727 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_cowgeneration != p->p_cowgeneration)
+ thread_update_cow(td);
switch (type) {
case T_PRIVINFLT: /* privileged instruction fault */

This seems reasonable, but I don't like verbose names like p_cowgeneration.
It is especially bad to abbreviate "copy on write" to "cow" and then spell
"generation" in full. "gen" would be a reasonable abbreviation, but "g"
goes better with "cow".

Old bad names visible in the patch include "thread" instead of "td". "td"
is not such a good abbreviation for "thread pointer".

Post by Mateusz Guzik
diff --git a/sys/kern/kern_thr.c b/sys/kern/kern_thr.c
index d5f1ce6..242e4dd 100644
--- a/sys/kern/kern_thr.c
+++ b/sys/kern/kern_thr.c

"thread" has too many different spellings. For just file names, there
are kern_thr.c and kern_thread.c. For variable names, there is also
"t" in "tid". "tid" is the best of all the names mentioned so far.

Post by Mateusz Guzik
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.

Post by Mateusz Guzik
@@ -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.

Post by Mateusz Guzik
+} 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 :-).

Bruce

Mateusz Guzik

2015-05-01 16:56:33 UTC

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?

Missing parentheses.

Oops, fixed.

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>

Julian Elischer

2015-05-03 13:04:03 UTC

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?

td_cowgen is much preferable with comment "(k) generation of c.o.w.
pointers."

Missing parentheses.

Oops, fixed.

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) {
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);

Bruce Evans

2015-05-03 23:58:48 UTC

Post by Julian Elischer

Post by Mateusz Guzik
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?

td_cowgen is much preferable with comment "(k) generation of c.o.w.
pointers."
+ u_int td_cowgen; /* (k) generation of c.o.w. pointers. */

It is less preferable with such a comment. The change in the comment just
adds 3 style bugs:
- inconsistent capitalization. In this file, comments on struct members
start with a capital letter
- punctuation for COW. Acronyms are not punctuated. Perhaps this one
should be in lower case (like vm is usually in lower case).
- verboseness from the previous bug. It expands the line line to 80.

But there is a problem with the comment. It doesn't do much more than
echo the variable name with minor expansions and rearrangements. The
only useful parts of it are "(k)" and "pointers".

Bruce

Julian Elischer

2015-05-04 05:03:44 UTC

On Sun, 3 May 2015, Julian Elischer wrote: td_cowgen is much
preferable with comment "(k) generation of c.o.w. pointers."

+ u_int td_cowgen; /* (k) generation of c.o.w.
pointers. */

But there is a problem with the comment. It doesn't do much more than
echo the variable name with minor expansions and rearrangements. The
only useful parts of it are "(k)" and "pointers".

cow is not immediately obvious to everyone.

Bruce

Bruce Evans

2015-05-03 23:47:36 UTC

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.

td_generation is a much worse. It looks like it might be a generation
counter for reused thread instances, but is actually just a thread
preemption counter.

Post by Mateusz Guzik
Is _cowgen variant ok?

Yes. It is more verbose than _cowg, but easier to guess what it means.
Everyone knows what a cow is, but not what a g is.

However, since td_cowgen is not for threads generally, but just for a
couple of things hung off threads, perhaps its name should give a hint
about those things and not say anything about "cow". I didn't notice
how you named these things.

Missing parentheses.

Oops, fixed.

Further naming problems. This macro doesn't update cow things, but
just increases the generation count.

Post by Mateusz Guzik
That said, I would say the patch below is ok enough.

OK.

Bruce

Mateusz Guzik

2015-04-28 02:34:03 UTC