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

[Jarno Rajahalme]

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