On 24 August 2017 at 00:03, Paul Blakey <[email protected]> wrote: > > > 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.
OK, thanks for the thorough explanation. I was under the assumption that the writes would only ever access on 32-bit boundaries. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
