On 22 August 2017 at 13:31, Paul Blakey <pa...@mellanox.com> 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 <pa...@mellanox.com> wrote:
>>>>
>>>>
>>>>
>>>> On 18/08/2017 00:52, Joe Stringer wrote:
>>>>>
>>>>>
>>>>> On 17 August 2017 at 02:18, Paul Blakey <pa...@mellanox.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 15/08/2017 20:04, Joe Stringer wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 15 August 2017 at 01:19, Paul Blakey <pa...@mellanox.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 15/08/2017 00:56, Joe Stringer wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 8 August 2017 at 07:21, Roi Dayan <r...@mellanox.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> From: Paul Blakey <pa...@mellanox.com>
>>>>>>>>>>
>>>>>>>>>> @@ -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 ?
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
