Date: Sun, 03 May 2015 21:04:03 +0800 From: Julian Elischer <julian@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com>, 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: <55461CC3.2020306@freebsd.org> In-Reply-To: <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> <20150501165633.GA7112@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 5/2/15 12:56 AM, Mateusz Guzik wrote:
> 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?
td_cowgen is much preferable with comment "(k) generation of c.o.w.
pointers."
>
>>> @@ -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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?55461CC3.2020306>