netdev - RE: [PATCH net-next 2/3] net: sctp: Optimise the way 'sctp_arg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <063D6719AE5E284EB5DD2968C1650D6D1726AA12@AcuExch.aculab.com>
Date:	Tue, 1 Jul 2014 13:04:34 +0000
From:	David Laight <David.Laight@...LAB.COM>
To:	'Neil Horman' <nhorman@...driver.com>
CC:	Network Development <netdev@...r.kernel.org>,
	"linux-sctp@...r.kernel.org" <linux-sctp@...r.kernel.org>,
	"David S. Miller" <davem@...emloft.net>
Subject: RE: [PATCH net-next 2/3] net: sctp: Optimise the way 'sctp_arg_t'
 values are initialised

From: Neil Horman [mailto:nhorman@...driver.com]
> On Tue, Jul 01, 2014 at 12:34:26PM +0000, David Laight wrote:
> > From: Neil Horman [mailto:nhorman@...driver.com]
> > > On Mon, Jun 30, 2014 at 01:01:07PM +0000, David Laight wrote:
> > > > Even if memset() is inlined (as on x86) using it to zero the union
> > > > generates a memory word write of zero, followed by a write of the
> > > > smaller field, and then a read of the word.
> > > > As well as being a lot of instructions the sequence is unlikely to
> > > > be optimised by the store-load forward hardware so will be slow.
> > > >
> > > > Instead allocate a field of the union that is the same size as the
> > > > entire union and write a zero value to it. The compiler will then
> > > > generate the required value in a register.
> > > >
> > > > Signed-off-by: David Laight <david.laight@...lab.com>
> > > > ---
> > > >  include/net/sctp/command.h | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/net/sctp/command.h b/include/net/sctp/command.h
> > > > index 020eb95..5fcd1a7 100644
> > > > --- a/include/net/sctp/command.h
> > > > +++ b/include/net/sctp/command.h
> > > > @@ -118,6 +118,7 @@ typedef enum {
> > > >  #define SCTP_MAX_NUM_COMMANDS 18
> > > >
> > > >  typedef union {
> > > > +	void *zero_all;	/* Set to NULL to clear the entire union */
> > > >  	__s32 i32;
> > > >  	__u32 u32;
> > > >  	__be32 be32;
> > > > @@ -154,7 +155,7 @@ typedef union {
> > > >  static inline sctp_arg_t	\
> > > >  SCTP_## name (type arg)		\
> > > >  { sctp_arg_t retval;\
> > > > -  memset(&retval, 0, sizeof(sctp_arg_t));\
> > > > +  retval.zero_all = NULL;\
> > > >    retval.elt = arg;\
> > > >    return retval;\
> > > >  }
> > > > @@ -191,7 +192,7 @@ static inline sctp_arg_t SCTP_NOFORCE(void)
> > > >  static inline sctp_arg_t SCTP_NULL(void)
> > > >  {
> > > >  	sctp_arg_t retval;
> > > > -	memset(&retval, 0, sizeof(sctp_arg_t));
> > > > +	retval.zero_all = NULL;
> > > >  	return retval;
> > > >  }
> > > >
> > > > @@ -212,7 +213,6 @@ typedef struct {
> > > >   */
> > > >  static inline int sctp_init_cmd_seq(sctp_cmd_seq_t *seq)
> > > >  {
> > > > -	memset(seq, 0, sizeof(sctp_cmd_seq_t));
> > > >  	return 1;		/* We always succeed.  */
> > > >  }
> > > >
> > > > --
> > > > 1.8.1.2
> > > >
> > > I get the advantage here, but this seems a bit rickety to me, in that it relies
> > > on the size of the union never being more than sizeof(void *) bytes long.  I
> > > understand thats not likely to happen, but its not the sort of thing we're
> > > likely to notice if it does.
> > >
> > > That said, I'd almost make the argument that we don't need to do this zeroing at
> > > all.  A while back I fixed a few outlier cases where we accidentally read a
> > > union member which we didn't set, so now we shouldn't have any type punning
> > > going on here.  It should be sufficient to just set the field you want, and
> > > leave it at that.
> >
> > On a 'big endian' system reading a wrong-sized member won't give the correct
> > value anyway.
> >
> > One benefit of zeroing the union is that diagnostic traces (added for debug)
> > will give sane values regardless of the member printed.
> > OTOH no such traces actually exist.
> >
> > With the patch, the generated code on amd64 is probably almost exactly
> > the same as that without the zeroing. The same is probable true for most LE
> > systems.
> > I haven't looked at how gcc handles the union on BE systems - but it is
> > likely to be a lot better if it isn't zeroed at all (or if all the fields
> > are the size of the union).
> >
> > 	David
> >
> Is it worth considering making a slab cache out of this?  We could amortize the
> cost of the zeroing then by doing it per slab, and then we wouldn't have to
> worry about making sure we always zero the largest element.

Probably not, it us currently an on-stack data area.
The cost of zeroing the entire structure is probably measureable
however it is done.
Remember, most of the time only a few entries are used - but the code
paths are used for every rx chunk and every tx request (and probably
elsewhere).

I actually thought about changing the 'array of struct' into a 'struct
of array' in order to reduce the on-stack memory footprint.
(The struct has two fields, one is 8 bits, the other a union whose
largest field is a pointer.)
The real optimisation is to realise that it is pointless to defer (most of)
the actions - it is only likely to lead to actions being performed on stale
pointers. However simple inlining changes the order - which might have
its own bugs.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html