| Message ID | 1366146438-8815-5-git-send-email-dborkman@redhat.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
> __u32 outstanding_bytes; > > - /* Are we doing fast-rtx on this queue */ > - char fast_rtx; > - > - /* Corked? */ > - char cork; > - > - /* Is this structure empty? */ > - char empty; > + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ > + cork:1, /* Corked? */ > + empty:1; /* Is this structure empty? */ > }; Use of bitfields just makes the code slower. The only real excuse for using them is to reduce the size of a structure that is allocated a lot. In the above you are just increasing the padding from 1 byte to 3 bytes. David -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/17/2013 10:43 AM, David Laight wrote: >> __u32 outstanding_bytes; >> >> - /* Are we doing fast-rtx on this queue */ >> - char fast_rtx; >> - >> - /* Corked? */ >> - char cork; >> - >> - /* Is this structure empty? */ >> - char empty; >> + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ >> + cork:1, /* Corked? */ >> + empty:1; /* Is this structure empty? */ >> }; > > Use of bitfields just makes the code slower. > The only real excuse for using them is to reduce the size > of a structure that is allocated a lot. sctp_outq is _embedded_ into an sctp_association structure, which has [size: 2280, cachelines: 36, members: 76]! A next step would be to try to reorder its elements carefully and see if we can reduce the size by filling some holes. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote: > On 04/17/2013 10:43 AM, David Laight wrote: > >> __u32 outstanding_bytes; > >> > >>- /* Are we doing fast-rtx on this queue */ > >>- char fast_rtx; > >>- > >>- /* Corked? */ > >>- char cork; > >>- > >>- /* Is this structure empty? */ > >>- char empty; > >>+ __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ > >>+ cork:1, /* Corked? */ > >>+ empty:1; /* Is this structure empty? */ > >> }; > > > >Use of bitfields just makes the code slower. > >The only real excuse for using them is to reduce the size > >of a structure that is allocated a lot. > > sctp_outq is _embedded_ into an sctp_association structure, which > has [size: 2280, cachelines: 36, members: 76]! A next step would be > to try to reorder its elements carefully and see if we can reduce > the size by filling some holes. Unrelated note: How did you get pahole to give you that information? For some reason pahole can never glean any useful information about struct sctp_association for me, despite having dwarf info embedded into the cu. Neil > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 16, 2013 at 11:07:13PM +0200, Daniel Borkmann wrote: > sctp_outq structure members fast_rtx, cork and empty are all > of type char. Consolidate them into a single __u8 bitfield since > they either carry 0 or 1. Have the rest 5 bits for future flags. > > Signed-off-by: Daniel Borkmann <dborkman@redhat.com> > --- > include/net/sctp/structs.h | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h > index 73fd5de..3de5985 100644 > --- a/include/net/sctp/structs.h > +++ b/include/net/sctp/structs.h > @@ -1051,14 +1051,9 @@ struct sctp_outq { > /* How many unackd bytes do we have in-flight? */ > __u32 outstanding_bytes; > > - /* Are we doing fast-rtx on this queue */ > - char fast_rtx; > - > - /* Corked? */ > - char cork; > - > - /* Is this structure empty? */ > - char empty; > + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ > + cork:1, /* Corked? */ > + empty:1; /* Is this structure empty? */ > }; > > void sctp_outq_init(struct sctp_association *, struct sctp_outq *); > -- > 1.7.11.7 > Acked-by: Neil Horman <nhorman@tuxdriver.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/17/2013 11:27 AM, Neil Horman wrote: > On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote: >> On 04/17/2013 10:43 AM, David Laight wrote: >>>> __u32 outstanding_bytes; >>>> >>>> - /* Are we doing fast-rtx on this queue */ >>>> - char fast_rtx; >>>> - >>>> - /* Corked? */ >>>> - char cork; >>>> - >>>> - /* Is this structure empty? */ >>>> - char empty; >>>> + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ >>>> + cork:1, /* Corked? */ >>>> + empty:1; /* Is this structure empty? */ >>>> }; >>> >>> Use of bitfields just makes the code slower. >>> The only real excuse for using them is to reduce the size >>> of a structure that is allocated a lot. >> >> sctp_outq is _embedded_ into an sctp_association structure, which >> has [size: 2280, cachelines: 36, members: 76]! A next step would be >> to try to reorder its elements carefully and see if we can reduce >> the size by filling some holes. > Unrelated note: How did you get pahole to give you that information? For some > reason pahole can never glean any useful information about struct > sctp_association for me, despite having dwarf info embedded into the cu. > pahole -C sctp_association <sctp object file> works for me. -vlad > Neil > >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/17/2013 05:27 PM, Neil Horman wrote: > On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote: >> On 04/17/2013 10:43 AM, David Laight wrote: >>>> __u32 outstanding_bytes; >>>> >>>> - /* Are we doing fast-rtx on this queue */ >>>> - char fast_rtx; >>>> - >>>> - /* Corked? */ >>>> - char cork; >>>> - >>>> - /* Is this structure empty? */ >>>> - char empty; >>>> + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ >>>> + cork:1, /* Corked? */ >>>> + empty:1; /* Is this structure empty? */ >>>> }; >>> >>> Use of bitfields just makes the code slower. >>> The only real excuse for using them is to reduce the size >>> of a structure that is allocated a lot. >> >> sctp_outq is _embedded_ into an sctp_association structure, which >> has [size: 2280, cachelines: 36, members: 76]! A next step would be >> to try to reorder its elements carefully and see if we can reduce >> the size by filling some holes. > Unrelated note: How did you get pahole to give you that information? For some > reason pahole can never glean any useful information about struct > sctp_association for me, despite having dwarf info embedded into the cu. I made a ``make net/sctp/'' and run pahole against net/sctp/sctp.o redirecting the output into a file, thus I have all possible headers visible. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 17, 2013 at 11:35:49AM -0400, Vlad Yasevich wrote: > On 04/17/2013 11:27 AM, Neil Horman wrote: > >On Wed, Apr 17, 2013 at 11:26:21AM +0200, Daniel Borkmann wrote: > >>On 04/17/2013 10:43 AM, David Laight wrote: > >>>> __u32 outstanding_bytes; > >>>> > >>>>- /* Are we doing fast-rtx on this queue */ > >>>>- char fast_rtx; > >>>>- > >>>>- /* Corked? */ > >>>>- char cork; > >>>>- > >>>>- /* Is this structure empty? */ > >>>>- char empty; > >>>>+ __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ > >>>>+ cork:1, /* Corked? */ > >>>>+ empty:1; /* Is this structure empty? */ > >>>> }; > >>> > >>>Use of bitfields just makes the code slower. > >>>The only real excuse for using them is to reduce the size > >>>of a structure that is allocated a lot. > >> > >>sctp_outq is _embedded_ into an sctp_association structure, which > >>has [size: 2280, cachelines: 36, members: 76]! A next step would be > >>to try to reorder its elements carefully and see if we can reduce > >>the size by filling some holes. > >Unrelated note: How did you get pahole to give you that information? For some > >reason pahole can never glean any useful information about struct > >sctp_association for me, despite having dwarf info embedded into the cu. > > > > pahole -C sctp_association <sctp object file> works for me. > > -vlad > Strange, that doesn't work for me, and the DW_TAG_structure_type entry is in place in the debug_info section (though interestingly no byte offsets are noted for any members). I'll have to look at that further. Thanks Neil > >Neil > > > >>-- > >>To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > >>the body of a message to majordomo@vger.kernel.org > >>More majordomo info at http://vger.kernel.org/majordomo-info.html > >> > >-- > >To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > >the body of a message to majordomo@vger.kernel.org > >More majordomo info at http://vger.kernel.org/majordomo-info.html > > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/17/2013 05:26 AM, Daniel Borkmann wrote: > On 04/17/2013 10:43 AM, David Laight wrote: >>> __u32 outstanding_bytes; >>> >>> - /* Are we doing fast-rtx on this queue */ >>> - char fast_rtx; >>> - >>> - /* Corked? */ >>> - char cork; >>> - >>> - /* Is this structure empty? */ >>> - char empty; >>> + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ >>> + cork:1, /* Corked? */ >>> + empty:1; /* Is this structure empty? */ >>> }; >> >> Use of bitfields just makes the code slower. >> The only real excuse for using them is to reduce the size >> of a structure that is allocated a lot. > > sctp_outq is _embedded_ into an sctp_association structure, which > has [size: 2280, cachelines: 36, members: 76]! A next step would be > to try to reorder its elements carefully and see if we can reduce > the size by filling some holes. I remember attempting this undertaking that led me down the rabbit hole so fast that my head started to spin. It is a badly needed project, but be prepared to spend a lot of time on it. Not sure if the space saving is worth the bad code generated for bit-fields, especially considering that empty and cork are used on almost every packet. -vlad > -- > To unsubscribe from this list: send the line "unsubscribe linux-sctp" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h index 73fd5de..3de5985 100644 --- a/include/net/sctp/structs.h +++ b/include/net/sctp/structs.h @@ -1051,14 +1051,9 @@ struct sctp_outq { /* How many unackd bytes do we have in-flight? */ __u32 outstanding_bytes; - /* Are we doing fast-rtx on this queue */ - char fast_rtx; - - /* Corked? */ - char cork; - - /* Is this structure empty? */ - char empty; + __u8 fast_rtx:1, /* Are we doing fast-rtx on this queue */ + cork:1, /* Corked? */ + empty:1; /* Is this structure empty? */ }; void sctp_outq_init(struct sctp_association *, struct sctp_outq *);
sctp_outq structure members fast_rtx, cork and empty are all of type char. Consolidate them into a single __u8 bitfield since they either carry 0 or 1. Have the rest 5 bits for future flags. Signed-off-by: Daniel Borkmann <dborkman@redhat.com> --- include/net/sctp/structs.h | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)