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

[Joe Stringer]

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.