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

Joe Stringer joe at ovn.org
Mon Aug 21 17:45:08 UTC 2017


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.


More information about the dev mailing list