On Sat, May 19, 2018 at 4:30 PM, Amit Kulkarni <amit.o...@gmail.com> wrote:
>
> 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).
> --- 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.
Do any of the other moves here cross a start/end zero/copy marker?
> @@ -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.
> @@ -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.
Philip Guenther
