> On Aug 3, 2015, at 6:04 PM, Jesse Gross <je...@nicira.com> wrote:
>
> On Mon, Aug 3, 2015 at 3:45 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
>> With few suggestions and questions below:
>>
>> Acked-by: Jarno Rajahalme <jrajaha...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
