[ovs-dev] [PATCH 2/2] miniflow: Use 64-bit data.

[Madhu Challa]

Thanks for the helpful explanation.

Thanks.

On Wed, Dec 17, 2014 at 12:45 PM, Jarno Rajahalme <jrajahalme at nicira.com>
wrote:
>
>
> On Dec 17, 2014, at 12:07 PM, Madhu Challa <challa at noironetworks.com>
> wrote:
>
> Jarno,
>
> I did some simple tests increasing flow_tnl size and trying to match on it
> looks good. Thanks for solving this. I have a question inline around
> miniflow_extract. Other than that the diff looks good to me.
>
> Thanks.
>
> On Wed, Dec 17, 2014 at 10:30 AM, Jarno Rajahalme <jrajahalme at nicira.com>
> wrote:
>>
>> (snip)
>
>
> +
>> +#define miniflow_push_be32_(MF, OFS, VALUE)                     \
>> +    miniflow_push_uint32_(MF, OFS, (OVS_FORCE uint32_t)(VALUE))
>> +
>> +#define miniflow_push_uint16_(MF, OFS, VALUE)                           \
>> +{                                                                       \
>> +    MINIFLOW_ASSERT(MF.data < MF.end &&                                 \
>> +                    (((OFS) % 8 == 0 && !(MF.map & (UINT64_MAX << (OFS)
>> / 8))) \
>> +                     || ((OFS) % 2 == 0 && MF.map & (UINT64_C(1) <<
>> (OFS) / 8) \
>> +                         && !(MF.map & (UINT64_MAX << ((OFS) / 8 +
>> 1)))))); \
>>                                                                          \
>> -    if ((OFS) % 4 == 0) {                                               \
>> +    if ((OFS) % 8 == 0) {                                               \
>>          *(uint16_t *)MF.data = VALUE;                                   \
>> -        MF.map |= UINT64_C(1) << (OFS) / 4;                             \
>> -    } else if ((OFS) % 4 == 2) {                                        \
>> +        MF.map |= UINT64_C(1) << (OFS) / 8;                             \
>> +    } else if ((OFS) % 8 == 2) {                                        \
>>          *((uint16_t *)MF.data + 1) = VALUE;                             \
>> +    } else if ((OFS) % 8 == 4) {                                        \
>> +        *((uint16_t *)MF.data + 2) = VALUE;                             \
>> +    } else if ((OFS) % 8 == 6) {                                        \
>> +        *((uint16_t *)MF.data + 3) = VALUE;                             \
>>          MF.data++;                                                      \
>>      }                                                                   \
>>  }
>>
>
> So I am assuming these calls should come in groups of 4 for the increment
> to work ? This does not show up in the diff but was wondering how this
> would get incremented and map set for say the ethernet header where we have
>
>  468         vlan_tci = parse_vlan(&data, &size);
>  469         dl_type = parse_ethertype(&data, &size);
>  470         miniflow_push_be16(mf, dl_type, dl_type);
>  471         miniflow_push_be16(mf, vlan_tci, vlan_tci);
>  472     }
>
>
> This was maybe the most involving change, and I don’t claim the “push”
> macros to be very general. In this case, a prior call to
> miniflow_push_macs() sets the map bits, but leaves the data pointer to
> point to the second, still incomplete 64-bit word, which can then be
> completed with other push types or padding. In general, a push starting a
> new 64-bit word sets the map bit, and the a push on the last bits of the
> 64-bit word increments the data pointer. This way we can mix and match
> different data types within the 64 bits.
>
> So the miniflow_push_macs() currently only works when the macs fields
> start 64-bit aligned. We can make it more general later if needed, but this
> should be good enough for now.
>
>   Jarno
>
>