Hi,
Is there any feedback on this?
Thanks
> > > 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. */
>
