> > I tested removing some slop (i.e. structure packing/de-holing) on amd64,
> > this went through a full kernel + userland build.
> >
>
> Parts of this are probably okay, but there's some stuff which needs better
> placement vs comments and at least one move which needs a justification for
> it being safe (or not).
Thanks for your feedback!
> > --- a/sys/sys/proc.h
> > +++ b/sys/sys/proc.h
> > @@ -170,8 +170,8 @@ struct process {
> >
> > /* The following fields are all zeroed upon creation in process_new. */
> > #define ps_startzero ps_klist
> > - struct klist ps_klist; /* knotes attached to this process
> > */
> > int ps_flags; /* PS_* flags. */
> > + struct klist ps_klist; /* knotes attached to this process
> > */
> >
>
> Nope: you've moved ps_flags from inside the "zeroed out on fork" region to
> outside of it
> a) without justifying why that's safe, and
> b) while leaving it below the comment saying that it's zeroed, when it no
> longer is.
My fault, I didn't read the defines properly before sending. Fixed by defining
ps_startzero to point to ps_flags, so it is zero'd out as before.
>
> Do any of the other moves here cross a start/end zero/copy marker?
Thanks for the hint. I re-checked now from the process_new() and thread_new()
functions in kern_fork.c. All the moves have been made within the
startcopy/startzero and endcopy/endzero defines in both struct proc and struct
process. So the memset to 0, and memcpy from parents will work as before. I
updated a comment to point to thread_new() function, so it is clear where
struct proc is inited. Please let me know if I have overlooked anything.
>
> > @@ -285,6 +284,7 @@ struct proc {
> > struct futex *p_futex; /* Current sleeping futex. */
> >
> > /* substructures: */
> > + LIST_ENTRY(proc) p_hash; /* Hash chain. */
> > struct filedesc *p_fd; /* copy of p_p->ps_fd */
> > struct vmspace *p_vmspace; /* copy of p_p->ps_vmspace */
> >
>
> p_hash isn't a substructure, so putting it below the /* substructures: */
> comment is wrong. Please pay attention to the comments and consider how
> the apply (or don't) to the members you're moving.
Fixed.
>
> > @@ -305,6 +304,11 @@ struct proc {
> > long p_thrslpid; /* for thrsleep syscall */
> >
> > /* scheduling */
> > + struct cpu_info * volatile p_cpu; /* CPU we're running on. */
> > +
> > + struct rusage p_ru; /* Statistics */
> > + struct tusage p_tu; /* accumulated times. */
> > + struct timespec p_rtime; /* Real time. */
> > u_int p_estcpu; /* Time averaged value of p_cpticks. */
> > int p_cpticks; /* Ticks of cpu time. */
> >
>
> Again, you've separated the scheduling parameter from the /* scheduling */
> comment, putting member that aren't about scheduling between them.
Fixed. The structs rusage/tusage/timespec are not part of scheduling, so I
moved them before the scheduling comment.
Updated diff follows. This survived a kernel compile, reboot, and use for quite
some time.
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 1c7ea4697e2..d6082cb0551 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -169,9 +169,9 @@ struct process {
pid_t ps_pid; /* Process identifier. */
/* The following fields are all zeroed upon creation in process_new. */
-#define ps_startzero ps_klist
- struct klist ps_klist; /* knotes attached to this process */
+#define ps_startzero ps_flags
int ps_flags; /* PS_* flags. */
+ struct klist ps_klist; /* knotes attached to this process */
struct proc *ps_single; /* Single threading to this thread. */
int ps_singlecount; /* Not yet suspended threads. */
@@ -200,15 +200,6 @@ struct process {
struct pgrp *ps_pgrp; /* Pointer to process group. */
struct emul *ps_emul; /* Emulation information */
- char ps_comm[MAXCOMLEN+1];
-
- vaddr_t ps_strings; /* User pointers to argv/env */
- vaddr_t ps_sigcode; /* User pointer to the signal code */
- vaddr_t ps_sigcoderet; /* User pointer to sigreturn retPC */
- u_long ps_sigcookie;
- u_int ps_rtableid; /* Process routing table/domain. */
- char ps_nice; /* Process "nice" value. */
-
struct uprof { /* profile arguments */
caddr_t pr_base; /* buffer base */
size_t pr_size; /* buffer size */
@@ -216,7 +207,15 @@ struct process {
u_int pr_scale; /* pc scaling */
} ps_prof;
+ char ps_comm[MAXCOMLEN+1];
+ char ps_nice; /* Process "nice" value. */
u_short ps_acflag; /* Accounting flags. */
+ u_int ps_rtableid; /* Process routing table/domain. */
+
+ vaddr_t ps_strings; /* User pointers to argv/env */
+ vaddr_t ps_sigcode; /* User pointer to the signal code */
+ vaddr_t ps_sigcoderet; /* User pointer to sigreturn retPC */
+ u_long ps_sigcookie;
uint64_t ps_pledge;
uint64_t ps_execpledge;
@@ -284,6 +283,8 @@ struct proc {
TAILQ_ENTRY(proc) p_fut_link; /* Threads in a futex linkage. */
struct futex *p_futex; /* Current sleeping futex. */
+ LIST_ENTRY(proc) p_hash; /* Hash chain. */
+
/* substructures: */
struct filedesc *p_fd; /* copy of p_p->ps_fd */
struct vmspace *p_vmspace; /* copy of p_p->ps_vmspace */
@@ -296,15 +297,19 @@ struct proc {
u_char p_descfd; /* if not 255, fdesc permits this fd */
pid_t p_tid; /* Thread identifier. */
- LIST_ENTRY(proc) p_hash; /* Hash chain. */
-/* The following fields are all zeroed upon creation in fork. */
+/* The following fields are all zeroed upon creation in thread_new. */
#define p_startzero p_dupfd
int p_dupfd; /* Sideways return value from filedescopen.
XXX */
long p_thrslpid; /* for thrsleep syscall */
+ struct rusage p_ru; /* Statistics */
+ struct tusage p_tu; /* accumulated times. */
+ struct timespec p_rtime; /* Real time. */
+
/* scheduling */
+ struct cpu_info * volatile p_cpu; /* CPU we're running on. */
u_int p_estcpu; /* Time averaged value of p_cpticks. */
int p_cpticks; /* Ticks of cpu time. */
const volatile void *p_wchan;/* Sleep address. */
@@ -315,11 +320,6 @@ struct proc {
u_int p_uticks; /* Statclock hits in user mode. */
u_int p_sticks; /* Statclock hits in system mode. */
u_int p_iticks; /* Statclock hits processing intr. */
- struct cpu_info * volatile p_cpu; /* CPU we're running on. */
-
- struct rusage p_ru; /* Statistics */
- struct tusage p_tu; /* accumulated times. */
- struct timespec p_rtime; /* Real time. */
int p_siglist; /* Signals arrived but not delivered. */
