Re: de-hole some structs on amd64
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. */
> > > #defineps_startzerops_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 {
> > > longp_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_startzerops_klist
> - struct klist ps_klist; /* knotes attached to this process */
> +#define ps_startzerops_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 */
>
> - charps_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. */
> - charps_nice;/* Process "nice" value. */
> -
> struct uprof { /* profile arguments */
> caddr_t pr_base;/* buffer base */
> size_t pr_size;/* buffer size */
> @@ -216,
Re: de-hole some structs on amd64
> > 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. */
> > #defineps_startzerops_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 {
> > longp_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. */
-#defineps_startzerops_klist
- struct klist ps_klist; /* knotes attached to this process */
+#defineps_startzerops_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 */
- charps_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. */
- charps_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;
+ charps_comm[MAXCOMLEN+1];
+ charps_nice;/* Process "nice" value. */
u_short ps_acf
Re: de-hole some structs on amd64
On Sat, May 19, 2018 at 4:30 PM, Amit Kulkarni 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. */
> #defineps_startzerops_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 {
> longp_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
de-hole some structs on amd64
Hi,
I tested removing some slop (i.e. structure packing/de-holing) on amd64, this
went through a full kernel + userland build.
struct proc 20 bytes (6 places) --> 4 bytes (2 places)
struct process 28 bytes (6 places) --> 4 bytes (1 place)
struct vm_map 8 bytes (2 places) --> 0 bytes
Thanks
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 1c7ea4697e2..d2955e2d0f7 100644
--- 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. */
#defineps_startzerops_klist
- struct klist ps_klist; /* knotes attached to this process */
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 */
- charps_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. */
- charps_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;
+ charps_comm[MAXCOMLEN+1];
+ charps_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;
@@ -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 */
#definep_rlimitp_p->ps_limit->pl_rlimit
@@ -296,7 +296,6 @@ 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. */
#definep_startzero p_dupfd
@@ -305,6 +304,11 @@ struct proc {
longp_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. */
const volatile void *p_wchan;/* Sleep address. */
@@ -315,11 +319,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. */
diff --git a/sys/uvm/uvm_map.h b/sys/uvm/uvm_map.h
index 07ca0d0ef45..4a63463d325 100644
--- a/sys/uvm/uvm_map.h
+++ b/sys/uvm/uvm_map.h
@@ -292,16 +292,15 @@ struct vm_map {
struct pmap * pmap; /* Physical map */
struct rwlock lock; /* Lock for map data */
struct mutexmtx;
- u_int serial; /* signals stack changes */
-
struct uvm_map_addr addr; /* Entry tree, by addr */
+ u_int serial; /* signals stack changes */
- vsize_t size; /* virtual size */
int
