[ovs-dev] [PATCH 3/4] tc: Add header rewrite using tc pedit action

Joe Stringer joe at ovn.org
Sat Aug 26 00:57:12 UTC 2017


On 24 August 2017 at 00:03, Paul Blakey <paulb at mellanox.com> wrote:
>
>
> On 23/08/2017 03:20, Joe Stringer wrote:
>>
>> On 22 August 2017 at 13:31, Paul Blakey <paulb at 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 <paulb at mellanox.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 18/08/2017 00:52, Joe Stringer wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 17 August 2017 at 02:18, Paul Blakey <paulb at mellanox.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 15/08/2017 20:04, Joe Stringer wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 15 August 2017 at 01:19, Paul Blakey <paulb at mellanox.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 15/08/2017 00:56, Joe Stringer wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 8 August 2017 at 07:21, Roi Dayan <roid at mellanox.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> From: Paul Blakey <paulb at 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 ?
>>
>
> 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.


More information about the dev mailing list