[ovs-dev] [PATCH 2/2] dpif-netdev: Translate Geneve options per-flow, not per-packet.

Jesse Gross jesse at nicira.com
Tue Aug 4 01:04:37 UTC 2015


On Mon, Aug 3, 2015 at 3:45 PM, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
> With few suggestions and questions below:
>
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

Thanks for the comments - I had one follow up question below.

>> @@ -3014,11 +3014,25 @@ dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_,
>>                  struct ofpbuf *actions, struct ofpbuf *put_actions)
>> {
>>     struct dp_netdev *dp = pmd->dp;
>> +    struct flow_tnl orig_tunnel;
>> +    int err;
>>
>>     if (OVS_UNLIKELY(!dp->upcall_cb)) {
>>         return ENODEV;
>>     }
>>
>> +    orig_tunnel.flags = flow->tunnel.flags;
>> +    if (flow->tunnel.flags & FLOW_TNL_F_UDPIF) {
>> +        orig_tunnel.metadata.present.len = flow->tunnel.metadata.present.len;
>> +        memcpy(orig_tunnel.metadata.opts.gnv, flow->tunnel.metadata.opts.gnv,
>> +               flow->tunnel.metadata.present.len);
>> +        err = tun_metadata_from_geneve_udpif(&orig_tunnel, &orig_tunnel,
>> +                                             &flow->tunnel);
>> +        if (err) {
>> +            return err;
>> +        }
>> +    }
>> +
>
> All these new blocks in dp_netdev_upcall() would benefit from short comments. I.e., “Upcall processing expects the geneve options to be in the translated format, but we need to retain the raw format for datapath use.”

This is a good idea. I incorporated all of your suggestions and added
a little more as well.

>> static inline uint32_t
>> diff --git a/lib/flow.c b/lib/flow.c
>> index 352e9b8..d3d25e4 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -462,9 +462,22 @@ miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
>>         miniflow_push_words(mf, tunnel, &md->tunnel,
>>                             offsetof(struct flow_tnl, metadata) /
>>                             sizeof(uint64_t));
>> -        if (md->tunnel.metadata.opt_map) {
>> -            miniflow_push_words(mf, tunnel.metadata, &md->tunnel.metadata,
>> -                                 sizeof md->tunnel.metadata / sizeof(uint64_t));
>> +
>> +        if (!(md->tunnel.flags & FLOW_TNL_F_UDPIF)) {
>> +            if (md->tunnel.metadata.present.map) {
>> +                miniflow_push_words(mf, tunnel.metadata, &md->tunnel.metadata,
>> +                                    sizeof md->tunnel.metadata /
>> +                                    sizeof(uint64_t));
>> +            }
>> +        } else {
>> +            if (md->tunnel.metadata.present.len) {
>> +                miniflow_push_words(mf, tunnel.metadata.present,
>> +                                    &md->tunnel.metadata.present, 1);
>> +                miniflow_push_words(mf, tunnel.metadata.opts.gnv,
>> +                                    md->tunnel.metadata.opts.gnv,
>> +                                    DIV_ROUND_UP(md->tunnel.metadata.present.len,
>> +                                    sizeof(uint64_t)));
>
> Wrong indentation here.

Sorry, which part? It's not jumping out at me.

>> @@ -1253,9 +1273,17 @@ flow_wc_map(const struct flow *flow, struct miniflow *map)
>>
>>     map->tnl_map = 0;
>>     if (flow->tunnel.ip_dst) {
>> -        map->tnl_map = MINIFLOW_TNL_MAP(tunnel);
>> -        if (!flow->tunnel.metadata.opt_map) {
>> -            map->tnl_map &= ~MINIFLOW_TNL_MAP(tunnel.metadata);
>> +        map->tnl_map |= MINIFLOW_TNL_MAP__(tunnel,
>> +                                          offsetof(struct flow_tnl, metadata));
>> +        if (!(flow->tunnel.flags & FLOW_TNL_F_UDPIF)) {
>> +            if (flow->tunnel.metadata.present.map) {
>> +                map->tnl_map |= MINIFLOW_TNL_MAP__(tunnel.metadata,
>> +                                                 sizeof(flow->tunnel.metadata));
>
> This could be MINIFLOW_TNL_MAP() instead?

You're right, thanks.

>> diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
>> index bd95c8e..0f1724a 100644
>> --- a/tests/tunnel-push-pop.at
>> +++ b/tests/tunnel-push-pop.at
>> @@ -132,7 +132,7 @@ AT_CHECK([ovs-ofctl dump-ports int-br | grep 'port  5'], [0], [dnl
>>   port  5: rx pkts=1, bytes=98, drop=0, errs=0, frame=0, over=0, crc=0
>> ])
>> AT_CHECK([ovs-appctl dpif/dump-flows int-br], [0], [dnl
>> -tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,ttl=64,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}),flags(-df-csum+key)),skb_mark(0),recirc_id(0),in_port(6081),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:drop
>> +tunnel(tun_id=0x7b,src=1.1.2.92,dst=1.1.2.88,ttl=64,geneve({class=0xffff,type=0x80,len=4,0xa/0xf}{class=0xffff,type=0,len=4}),flags(-df-csum+key)),skb_mark(0),recirc_id(0),in_port(6081),eth_type(0x0800),ipv4(frag=no), packets:0, bytes:0, used:never, actions:drop
>
> Why this changed?

This is actually a nice side benefit of the change. The input packet
actually has two options, the second of which was unknown. Previously,
the unknown one didn't show up in the installed flow since it was
skipped over by the userspace packet parsing code. Now that we always
extract the full set of options, the unknown option is visible to the
unit test and we can validation the processing associated with them.



More information about the dev mailing list