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

Paul Blakey paulb at mellanox.com
Tue Aug 22 20:07:43 UTC 2017



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))


need to be careful that rewrite struct will always be inside flower.




More information about the dev mailing list