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

Jarno Rajahalme jrajahalme at nicira.com
Wed Aug 5 19:44:21 UTC 2015


> On Aug 3, 2015, at 6:04 PM, Jesse Gross <jesse at nicira.com> wrote:
> 
> 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.

sizeof is the argument of DIV_ROUND_UP, so it should be indented after the opening parenthesis of “DIV_ROUND_UP(“

> 
>>> @@ -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.

Ok, I think you mentioned this somehow in the commit message, but maybe you could add explicitly that also the unknown options are now shown by dump-flows?

  Jarno





More information about the dev mailing list