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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
