Message ID | 69a5ce012d9978ce73aade0004c5937964c54d61.1459952558.git.marcelo.leitner@gmail.com |
---|---|
State | Superseded, archived |
Delegated to: | David Miller |
Headers | show |
On Wed, 2016-04-06 at 14:53 -0300, Marcelo Ricardo Leitner wrote: > It wastes space and gets worse as we add new flags, so convert bit-wide > flags to a bitfield. > > Currently it already saves 4 bytes in sctp_sock, which are left as holes > in it for now. The whole struct needs packing, which should be done in > another patch. > > Note that do_auto_asconf cannot be merged, as explained in the comment > before it. > > Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > --- [] > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h [] > @@ -210,14 +210,14 @@ struct sctp_sock { > int user_frag; > > __u32 autoclose; > - __u8 nodelay; > - __u8 disable_fragments; > - __u8 v4mapped; > - __u8 frag_interleave; > __u32 adaptation_ind; > __u32 pd_point; > - __u8 recvrcvinfo; > - __u8 recvnxtinfo; > + __u16 nodelay:1, > + disable_fragments:1, > + v4mapped:1, > + frag_interleave:1, > + recvrcvinfo:1, > + recvnxtinfo:1; Might as well make this __u32 as the next field would be aligned on an atomic_t It might be better if these fields didn't use the __ prefix.
From: Joe Perches <joe@perches.com> Date: Wed, 06 Apr 2016 12:53:24 -0700 > On Wed, 2016-04-06 at 14:53 -0300, Marcelo Ricardo Leitner wrote: >> It wastes space and gets worse as we add new flags, so convert bit-wide >> flags to a bitfield. >> >> Currently it already saves 4 bytes in sctp_sock, which are left as holes >> in it for now. The whole struct needs packing, which should be done in >> another patch. >> >> Note that do_auto_asconf cannot be merged, as explained in the comment >> before it. >> >> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> >> --- > [] >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > [] >> @@ -210,14 +210,14 @@ struct sctp_sock { >> int user_frag; >> >> __u32 autoclose; >> - __u8 nodelay; >> - __u8 disable_fragments; >> - __u8 v4mapped; >> - __u8 frag_interleave; >> __u32 adaptation_ind; >> __u32 pd_point; >> - __u8 recvrcvinfo; >> - __u8 recvnxtinfo; >> + __u16 nodelay:1, >> + disable_fragments:1, >> + v4mapped:1, >> + frag_interleave:1, >> + recvrcvinfo:1, >> + recvnxtinfo:1; > > Might as well make this __u32 as the next field would be > aligned on an atomic_t > > It might be better if these fields didn't use the __ prefix. Indeed, this isn't in a UAPI file so __ prefixed types really aren't appropriate.
On Wed, Apr 06, 2016 at 03:57:35PM -0400, David Miller wrote: > From: Joe Perches <joe@perches.com> > Date: Wed, 06 Apr 2016 12:53:24 -0700 > > > On Wed, 2016-04-06 at 14:53 -0300, Marcelo Ricardo Leitner wrote: > >> It wastes space and gets worse as we add new flags, so convert bit-wide > >> flags to a bitfield. > >> > >> Currently it already saves 4 bytes in sctp_sock, which are left as holes > >> in it for now. The whole struct needs packing, which should be done in > >> another patch. > >> > >> Note that do_auto_asconf cannot be merged, as explained in the comment > >> before it. > >> > >> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> > >> --- > > [] > >> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > > [] > >> @@ -210,14 +210,14 @@ struct sctp_sock { > >> int user_frag; > >> > >> __u32 autoclose; > >> - __u8 nodelay; > >> - __u8 disable_fragments; > >> - __u8 v4mapped; > >> - __u8 frag_interleave; > >> __u32 adaptation_ind; > >> __u32 pd_point; > >> - __u8 recvrcvinfo; > >> - __u8 recvnxtinfo; > >> + __u16 nodelay:1, > >> + disable_fragments:1, > >> + v4mapped:1, > >> + frag_interleave:1, > >> + recvrcvinfo:1, > >> + recvnxtinfo:1; > > > > Might as well make this __u32 as the next field would be > > aligned on an atomic_t On changelog I meant that this (re-)ordering will happen in another patch. The hole is already there today and there are others to be considered/fixed too. Please consider this patch as a preparation for the other one. After this patch, pahole gives for this struct: /* size: 1272, cachelines: 20, members: 40 */ /* sum members: 1250, holes: 7, sum holes: 18 */ /* bit holes: 1, sum bit holes: 9 bits */ /* padding: 4 */ /* paddings: 1, sum paddings: 2 */ /* last cacheline: 56 bytes */ Quite some work todo :-) > > It might be better if these fields didn't use the __ prefix. > > Indeed, this isn't in a UAPI file so __ prefixed types really aren't > appropriate. The whole file is using __ prefixed types. I can remove them in another patch instead if that's okay with you. It's also present in other files not under UAPI: $ git grep -wl '\(__u32\|__u16\)' include/net/sctp/ include/net/sctp/auth.h include/net/sctp/checksum.h include/net/sctp/command.h include/net/sctp/constants.h include/net/sctp/sctp.h include/net/sctp/sm.h include/net/sctp/structs.h include/net/sctp/tsnmap.h include/net/sctp/ulpevent.h include/net/sctp/ulpqueue.h We can start changing it progressively but I think it would be better if we replace them all at once. Marcelo
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 6df1ce7a411c548bda4163840a90578b6e1b4cfe..1a6a626904bba4223b7921bbb4be41c2550271a7 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -210,14 +210,14 @@ struct sctp_sock { int user_frag; __u32 autoclose; - __u8 nodelay; - __u8 disable_fragments; - __u8 v4mapped; - __u8 frag_interleave; __u32 adaptation_ind; __u32 pd_point; - __u8 recvrcvinfo; - __u8 recvnxtinfo; + __u16 nodelay:1, + disable_fragments:1, + v4mapped:1, + frag_interleave:1, + recvrcvinfo:1, + recvnxtinfo:1; atomic_t pd_mode; /* Receive to here while partial delivery is in effect. */
It wastes space and gets worse as we add new flags, so convert bit-wide flags to a bitfield. Currently it already saves 4 bytes in sctp_sock, which are left as holes in it for now. The whole struct needs packing, which should be done in another patch. Note that do_auto_asconf cannot be merged, as explained in the comment before it. Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> --- include/net/sctp/structs.h | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)