Date: Tue, 28 Apr 2015 18:45:01 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-arch@freebsd.org, Mateusz Guzik <mjg@freebsd.org> Subject: Re: [PATCH 1/2] Generalised support for copy-on-write structures shared by threads. Message-ID: <20150428181802.F1119@besplex.bde.org> In-Reply-To: <1430188443-19413-2-git-send-email-mjguzik@gmail.com> References: <1430188443-19413-1-git-send-email-mjguzik@gmail.com> <1430188443-19413-2-git-send-email-mjguzik@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 28 Apr 2015, Mateusz Guzik wrote:
> 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".
> 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.
> 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.
> @@ -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.
> +} 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20150428181802.F1119>