Re: [PATCH] arm: Silence gcc warnings about arch ABI drift

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Calvin Owens <jcalvinowens@gmail.com>,
	Dave Martin <Dave.Martin@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	"linux-bcachefs@vger.kernel.org" <linux-bcachefs@vger.kernel.org>
Subject: Re: [PATCH] arm: Silence gcc warnings about arch ABI drift
Date: Mon, 19 Feb 2024 09:52:08 +0000	[thread overview]
Message-ID: <ZdMkyPX/h9s8xht5@shell.armlinux.org.uk> (raw)
In-Reply-To: <xs2splnjhlj257uca5yae64fp4solc4ugbxrkczoyd75n42r66@42boyzdcmyj4>

On Mon, Feb 19, 2024 at 04:40:00AM -0500, Kent Overstreet wrote:
> On Mon, Feb 19, 2024 at 09:26:51AM +0000, Russell King (Oracle) wrote:
> > On Mon, Feb 19, 2024 at 07:21:11AM +0100, Arnd Bergmann wrote:
> > > I think these should be addressed in bcachefs instead.
> > > While it's not the fault of bcachefs that the calling
> > > convention changed between gcc versions, have a look at
> > > the actual structure layout:
> > > 
> > > struct bch_val {
> > >         __u64           __nothing[0];
> > > };
> > > struct bpos {
> > >         /*
> > >          * Word order matches machine byte order - btree code treats a bpos as a
> > >          * single large integer, for search/comparison purposes
> > >          *
> > >          * Note that wherever a bpos is embedded in another on disk data
> > >          * structure, it has to be byte swabbed when reading in metadata that
> > >          * wasn't written in native endian order:
> > >          */
> > > #if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
> > >         __u32           snapshot;
> > >         __u64           offset;
> > >         __u64           inode;
> > > #elif __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
> > >         __u64           inode;
> > >         __u64           offset;         /* Points to end of extent - sectors */
> > >         __u32           snapshot;
> > > #else
> > > #error edit for your odd byteorder.
> > > #endif
> > > } __packed
> > > struct bch_backpointer {
> > >         struct bch_val          v;
> > >         __u8                    btree_id;
> > >         __u8                    level;
> > >         __u8                    data_type;
> > >         __u64                   bucket_offset:40;
> > >         __u32                   bucket_len;
> > >         struct bpos             pos;
> > > } __packed __aligned(8);
> > > 
> > > This is not something that should ever be passed by value
> > > into a function.
> > 
> > +1 - bcachefs definitely needs fixing. Passing all that as an argument
> > not only means that it has to be read into registers, but also when
> > accessing members, it has to be extracted from those registers as well.
> > 
> > Passing that by argument is utterly insane.
> 
> If the compiler people can't figure out a vaguely efficient way to pass
> a small struct by value, that's their problem - from the way you
> describe it, I have to wonder at what insanity is going on there.

It sounds like you have a personal cruisade here.

Passing structures on through function arguments is never efficient.
The entire thing _has_ to be loaded from memory at function call and
either placed onto the stack (creating an effective memcpy() on every
function call) or into registers. Fundamentally. It's not something
compiler people can mess around with.

Sorry but it's bcachefs that's the problem here, and well done to the
compiler people for pointing out stupid code.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel