On Wed, Jun 19, 2019 at 09:50:58PM +0800, Qu Wenruo wrote:
>
>
> On 2019/6/19 下午9:39, David Sterba wrote:
> > On Wed, Jun 19, 2019 at 09:37:39AM +0800, Qu Wenruo wrote:
> >>> struct btrfs_key {
> >>> __u64 objectid;
> >>> - __u8 type;
> >>> __u64 offset;
> >>> + __u8 type;
> >>> } __attribute__ ((__packed__));
> >>
> >> And why not remove the packed attribute?
> >
> > Because of this (stack usage changes):
>
> That's expected as long as we're using btrfs_key on stack.
And that's in many functions.
> But if we're using btrfs_key on stack and follow the packed feature,
> then adjacent on stack memory is not accessed aligned, which could cause
> (unobvious) performance drop.
No, the placement of local variables is up to the compiler and does not
necessarily follow the same order as teh definition, some of the
variables can be completely optimized out etc. And the types must be
accessed according to the type alignments, so one packed structure does
not cause a disaster.
> If the unaligned memory access is really causing some performance even
> on stack memory, then I'd say the bump in stack memory usage is acceptable.
Unaligned access overhead depends on architecture, and there's almost
none on intel, so the packed key makes no difference regarding speed.
I consider the bump in stack consumption high enough to stick to the
packing, given that it's still ok from the performance POV.
> If not, then the idea of default -Waddress-of-packed-member makes no sense.
Yeah, I start to think that it's causing more harm than good.
