netdev - RE: [PATCH net-next 7/8] ionic: Use an u16 for rx

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: <610168e14b814c67b2ef63f1a4fb0eca@AcuMS.aculab.com>
Date: Sun, 16 Jun 2024 10:27:52 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Andrew Lunn' <andrew@...n.ch>
CC: 'Shannon Nelson' <shannon.nelson@....com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net>,
	"kuba@...nel.org" <kuba@...nel.org>, "edumazet@...gle.com"
	<edumazet@...gle.com>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"brett.creeley@....com" <brett.creeley@....com>, "drivers@...sando.io"
	<drivers@...sando.io>
Subject: RE: [PATCH net-next 7/8] ionic: Use an u16 for rx_copybreak

From: Andrew Lunn
> Sent: 16 June 2024 02:29
> 
> > > --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > > +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> > > @@ -206,7 +206,7 @@ struct ionic_lif {
> > >  	unsigned int nxqs;
> > >  	unsigned int ntxq_descs;
> > >  	unsigned int nrxq_descs;
> > > -	u32 rx_copybreak;
> > > +	u16 rx_copybreak;
> > >  	u64 rxq_features;
> > >  	u16 rx_mode;
> >
> > There seem to be 6 pad bytes here - why not just use them??
> 
> Or at least move rx_copybreak next to rx_mode so the compiler can pack
> them together.
> 
> It would be good to include some output from pahole in the commit
> message to show the goal of this patch has actually been reached.

And then start asking whether the fields are grouped for cache usage at all.
And whether the structure itself is allocated by kmalloc() or is nested
in something else.

You might worry about structure holes because they make the structure
larger - but that only matters if the allocator rounds it up to a
bigger size. And that only really matters if you allocate lots of them.

So the dominant part of this change is probably the extra code.

When you add the extra flag as 'uint flag:1' you also generate worse
code that just using a 'u8' - so not worth it unless you need to pack
multiple flags into a word.

Of course, slight re-ordering to avoid holes is usually fine.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)