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))
need to be careful that rewrite struct will always be inside flower.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
