Proper packed attribute usage?

Proper packed attribute usage?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1085 bytes --]

Hi,

With GCC 9 coming soon, its new default warning,
-Waddress-of-packed-member, is already causing a lot of btrfs-progs
warnings.

It's pretty sure kernel will just suppress this warning, but this makes
me to think about the proper way to use packed attribute.

To my poor understanding, we should only use packed attribute handling:
- On-disk format
  Obviously. And also needs extra handlers to do the endian convert.

- Ioctl parameters
  To make sure the format doesn't change.

- Fixed format packages
  For network packages.

But then this means, we should have two copies of data for every such
structures.
One for the fixed format one, and one for the compiler aligned one, with
enough helper to convert them (along with needed endian convert).

Is that the correct practice?

And for a btrfs specific question, why we have packed attribute for
btrfs_key?
I see no specific reason to make a CPU native structure packed, not to
mention we already have btrfs_disk_key.
And no ioctl structure is using btrfs_key in its parameter.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Proper packed attribute usage?

[David Sterba]

On Mon, Jun 17, 2019 at 05:06:22PM +0800, Qu Wenruo wrote:
> And for a btrfs specific question, why we have packed attribute for
> btrfs_key?
> I see no specific reason to make a CPU native structure packed, not to
> mention we already have btrfs_disk_key.

For that one there's no reason to use packed and we can go further and
reorder the members so that the offset is right after objectid, ie. both
at aligned addresses.  I have a patch for that but I still need to
validate that.

Re: Proper packed attribute usage?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 753 bytes --]



On 2019/6/17 下午9:18, David Sterba wrote:
> On Mon, Jun 17, 2019 at 05:06:22PM +0800, Qu Wenruo wrote:
>> And for a btrfs specific question, why we have packed attribute for
>> btrfs_key?
>> I see no specific reason to make a CPU native structure packed, not to
>> mention we already have btrfs_disk_key.
> 
> For that one there's no reason to use packed and we can go further and
> reorder the members so that the offset is right after objectid, ie. both
> at aligned addresses.

Yep, exactly what I want to do.

>  I have a patch for that but I still need to
> validate that.

A quick grep shows no direct btrfs_key usage in ioctl structures.
Thus I think existing test cases should be enough to validate it.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: Proper packed attribute usage?

[James Bottomley]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Mon, 2019-06-17 at 17:06 +0800, Qu Wenruo wrote:
[...]
> But then this means, we should have two copies of data for every such
> structures. One for the fixed format one, and one for the compiler
> aligned one, with enough helper to convert them (along with needed
> endian convert).

I don't think it does mean this.  The compiler can easily access the
packed data by pointer, the problem on systems requiring strict
alignment is that it has to be done with byte accesses, so instead of a
load word for a pointer to an int, you have to do four load bytes. 
This is mostly a minor slowdown so trying to evolve a whole
infrastructure around copying data for these use cases really wouldn't
be a good use of resources.

James

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Proper packed attribute usage?

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]



On 2019/6/17 下午11:37, James Bottomley wrote:
> On Mon, 2019-06-17 at 17:06 +0800, Qu Wenruo wrote:
> [...]
>> But then this means, we should have two copies of data for every such
>> structures. One for the fixed format one, and one for the compiler
>> aligned one, with enough helper to convert them (along with needed
>> endian convert).
> 
> I don't think it does mean this.  The compiler can easily access the
> packed data by pointer, the problem on systems requiring strict
> alignment is that it has to be done with byte accesses, so instead of a
> load word for a pointer to an int, you have to do four load bytes. 
> This is mostly a minor slowdown so trying to evolve a whole
> infrastructure around copying data for these use cases really wouldn't
> be a good use of resources.

Then I can argue it shouldn't be a default warning for GCC 9.

But anyway, kernel will disable the warning, so I think it shouldn't
cause much problem.

Thanks,
Qu

> 
> James
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]