On 23/08/2017 03:20, Joe Stringer wrote:
On 22 August 2017 at 13:31, Paul Blakey <[email protected]> wrote:
On 22/08/2017 23:07, Paul Blakey wrote:
On 21/08/2017 20:45, Joe Stringer wrote:
On 20 August 2017 at 01:50, Paul Blakey <[email protected]> wrote:
On 18/08/2017 00:52, Joe Stringer wrote:
On 17 August 2017 at 02:18, Paul Blakey <[email protected]> wrote:
On 15/08/2017 20:04, Joe Stringer wrote:
On 15 August 2017 at 01:19, Paul Blakey <[email protected]> wrote:
On 15/08/2017 00:56, Joe Stringer wrote:
On 8 August 2017 at 07:21, Roi Dayan <[email protected]> wrote:
From: Paul Blakey <[email protected]>
@@ -117,6 +118,17 @@ struct tc_flower {
uint64_t lastused;
struct {
+ bool rewrite;
+ uint8_t pad1[3];
+ struct tc_flower_key key;
+ uint8_t pad2[3];
+ struct tc_flower_key mask;
+ uint8_t pad3[3];
+ } rewrite;
Now that I get why the pads are here.. ;)
Is there an existing macro we can use to ensure that these pad out to
32-bit boundaries?
I'm not sure if that's possible, the size is a minimum of extra
24bits,
so
its can't overflow with writing on any address below it. The compiler
might add some extra padding but that shouldn't matter.
My thought was that the main concern here is to align the fields to
32-bit boundaries, so if it already does this then I wouldn't expect
to need any padding at all? For instance, on my system with these
patches I see with pahole that 'struct tc_flower_key' has size 84, and
the 'pad2' and 'pad3' appear to be unnecessary:
struct {
_Bool rewrite; /* 216
1 */
uint8_t pad1[0]; /* 217
0 */
struct tc_flower_key key; /* 220
84 */
/* --- cacheline 1 boundary (64 bytes) was 21 bytes
ago
--- */
uint8_t pad2[0]; /* 304
0 */
struct tc_flower_key mask; /* 308
84 */
/* --- cacheline 2 boundary (128 bytes) was 41 bytes
ago
--- */
uint8_t pad3[0]; /* 392
0 */
} rewrite; /* 216
180 */
However, if in future someone adds a 1-byte member to tc_flower_key
then I'm not sure that this alignment will hold. On my system, it
seems like that would end up padding tc_flower_key out to 88B so these
extra padding would still be unnecessary (although pahole says that it
has a brain fart, so I'm not sure exactly how much confidence I should
have in this).
What I was thinking about was whether we could use something like
PADDED_MEMBERS() in this case.
Are you talking about alignment in regards to performance?
Because the padding is there for overflowing.
If someone adds a new member to struct key/mask that is smaller than a
32bits to the end of the struct, setting it via a pointer might overflow
the
struct. I don't think PADDED_MEMBERS will work for this type of padding.
We do mask the write to the write size, and it should still be in memory
of
owned by struct flower and since the compiler can't reorder the struct
we
actually only need the last padding to cover the above case.
Maybe we can add alignment when we measure the performance of the code?
Ah. I wasn't concerned about performance, I was just wondering if this
forces us to add a few extra unnecessary bytes to 'rewrite' regardless
of the size of 'struct tc_flower'. For instance, if 'struct tc_flower'
is 63B long because someone adds a small field to the end of the
structure, then I'd expect to need only one extra byte of padding. If
it were a multiple of 32 bits, I wouldn't expect any need for padding
at all.
I see. You're right but keep in mind that this struct is on the stack and
a couple of bytes shouldn't matter much. I'm not sure how to define a macro
that adds padding based on the last member in the struct unless I do define
it like PADDED_MEMBERS but with the last member isolated from
the rest (e.g PAD_TO_LAST(unit, members, last_member)) so I can test it's
size. A build time assert would be the same.
Since we mask the writes/reads, Maybe we can assert whether reading 32bit
(or actually 24bit) past the struct rewrite is within struct flower bounds.
Basically that no one moved it to the end of struct flower.
pseudo code: BUILD_DECL_ASSERT(member_offsetof(flower.rewrite) +
sizeof(uint32_t) < sizeof(struct flower))
BUILD_DECL_ASSERT(member_offsetof(flower.rewrite) + sizeof(flower.rewrite) +
sizeof(uint32_t) -1 < sizeof(struct flower))
Could we just check that (member_offsetof(flower.rewrite) +
sizeof(flower.rewrite)) % sizeof(uint32_t) == 0 ?
Also for the flower.rewrite.key and flower.rewrite.mask ?
Maybe I'm missing something but that doesn't check that we wouldn't
spill over to outside of flower if someone defines rewrite (moves the
rewrite struct) and flower like this:
struct flower_key {
uint32_t other_members[5]
uint8_t other_members[3]
uint8_t some_member;
}
struct rewrite {
struct flower_key key;
struct flower_key mask;
}
struct flower {
uint32_t other_members[10]
struct rewrite;
}
member_offsetof(flower.rewrite) = 40
sizeof(flower_key) = 24
sizeof(rewrite) = 48
sizeof(flower) = 88
here sizeof(rewrite) % 4 == 0 and member_offsetof(flower.rewrite) % 4 ==
0 so it won't fail. but writing uint32_t to
offsetof(rewrite.mask.some_member) will overflow by 3 bytes.
This BUILD_DECL_ASSERT(member_offsetof(flower.rewrite) +
sizeof(flower.rewrite) + sizeof(uint32_t) - 2 < sizeof(struct flower))
will catch this (40 + 48 + 4 - 2 = 90 < 88)
moving rewrite back to a safe position will pass:
struct flower {
uint32_t other_members[10]
struct rewrite;
uint8_t other_members[3]
}
40 + 48 + 4 - 2 = 90 < 91
any less padding from other_members and we will fail.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
