Hi, I sent a new version to dev list. Please ignore this one. I changed the subject, since the problem does not seem to be restricted to tunnel handling.
https://patchwork.ozlabs.org/patch/851949/ Best regards, Zoltan > -----Original Message----- > From: [email protected] > [mailto:[email protected]] On Behalf Of Zoltán Balogh > Sent: Monday, December 11, 2017 4:39 PM > To: Jan Scheurich <[email protected]>; [email protected]; Ben > Pfaff <[email protected]> > Subject: Re: [ovs-dev] [PATCH] tunnel: fix tnl_find() after packet_type > changed > > Hi, > > I've been working on the solution proposed by Jan. I faced a new issue with > patch ports. Let's have a config like in unit test 1025: > "1025: ofproto-dpif - balance-tcp bonding, different recirc flow " > > > +-------------+ > | br-int | p5 (OF 5) > | o--<--- > +---o---------+ > | br1- (patch port: OF 101) > | > +------+ > | > | br1+ (patch port: OF 100) > +----------o--+ > | br1 | > | | > +---o-----o---+ > p3 | | p4 > (OF 3)| | (OF 4) > |bond1| > +--+--+ > | > > In the unit test, a packet is received on port p5. The packet is then output > to > 'br1-' which is a patch port, its pair is 'br1+' on br1. Ports p3 and p4 are > in > bond0, the packet is recirculated in br1, before it is sent out on bond1. > > So, when it's recirculated, ofproto xlate will end up in > xlate_lookup_ofproto_() > at some point in time. The vanilla code does xport_lookup() based on the flow > which is built up based on dp_packet (due to miniflow_extract/expand) which > has > in port 5 (p5). So the flow has in port 5 as well. Furthermore, the returned > ofproto_dpif will be xport->xbridge->ofproto: br-int. > > If we do use the recirc ID extracted from the flow (or from dp_packet) which > is > 2, then the state's uuid will refer to the bridge br1. This is the bridge > where > the recirc action was added to odp_actions. On the other hand the in port > stored in the state is 0xffff (undefined). It's neither 5 (p5) nor 100 (br1+). > > So, If we want to build our code based on the recirc ID extracted from flow > (or > dp_packet) we won't get the same result as the vanilla code in case of using > patch ports and recirculate in the second bridge. > > Does anyone have a idea how to resolve this? > Maybe the vanilla code or patch port concept is broken? > > If we would use the original value of packet_type in tnl_find() we could still > workaround this. > > Best regards, > Zoltan > > > -----Original Message----- > > From: Jan Scheurich > > Sent: Friday, December 08, 2017 5:18 PM > > To: Zoltán Balogh <[email protected]>; [email protected] > > Cc: Ben Pfaff <[email protected]> > > Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed > > > > Hi Zoltan, > > Please find answers below. > > Regards, Jan > > > > > -----Original Message----- > > > From: Zoltán Balogh > > > Sent: Friday, 08 December, 2017 12:56 > > > To: Jan Scheurich <[email protected]>; [email protected] > > > Cc: Ben Pfaff <[email protected]> > > > Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed > > > > > > Hello Jan, > > > > > > Thank you for the review. > > > The recirc_id_node, and thus the frozen_state with the the ofproto_uuid > > > can be retrieved from recirc ID via > > recirc_id_node_find(). So I > > > think, it would be feasible to get the ofproto from the recirc ID without > > > calling tnl_find(). In addition we > would > > need to store in_port in the > > > frozen_state. > > > > Yes, my thinking was to lookup the recirculation context in > > xlate_lookup_ofproto_() if flow->recirc_id != 0 and > get > > bridge and in_port from the frozen data in that case. But I was looking for > > a way to store the recirc context > > pointer so that it need not be looked up once more later during > > xlate_actions(). > > > > BTW: in_port is already stored in frozen_state.metdata.in_port > > > > > > > > However, if we do not fix this in tnl_find(), then we need to make sure, > > > that neither xlate_lookup_ofproto() nor > > xlate_lookup() are called > > > after flow->packet_type has changed during xlate, don't we? For instance, > > > calling revalidate() can lead to call > > xlate_lookup(). > > > > revalidate() is called from revalidator threads to pass an artificial > > packet representing the megaflow through the > > pipeline. The flow constructed from the ukey will contain the recirc_id and > > it should still hit the same > > recirculation context with the frozen_state as a MISS upcall for the > > originally recirculated packet. > > > > > > > > Btw, what about the case, when we have a L2 and L3 port with the same > > > local IP. A packet is received on one of > > them, but the packet_type has changed during xlate and later revalidate > > leads to calling xlate_lookup()? It will > > provide the wrong tunnel port as in_port. Is this a valid case? > > > > I think xlate_lookup() is only executed once at the beginning of each > > upcall or ukey revalidation to initiatilize > > some data. It is never done again later during the xlation process. > > > > > > > > I was thinking about storing the original value of packet_type of > > > received packet in dp_packet or in the flow > > structure. Then use the > > > original value in tnl_find(). > > > > > > Best regards, > > > Zoltan > > > > > > > -----Original Message----- > > > > From: Jan Scheurich > > > > Sent: Friday, December 08, 2017 11:30 AM > > > > To: Zoltán Balogh <[email protected]>; [email protected] > > > > Cc: Ben Pfaff <[email protected]> > > > > Subject: RE: [PATCH] tunnel: fix tnl_find() after packet_type changed > > > > > > > > Hi Zoltan, > > > > > > > > My feeling here is that it is conceptually wrong to do another tunnel > > > > lookup if the packet is recirculated > after > > it > > > > has already been de-tunneled. You are trying to fix the symptoms and I > > > > am not sure that the more liberal check > > on > > > > packet type won't lead to incorrect proper tunnel port lookups. > > > > > > > > The actual bridge is stored in the frozen state of the recirculation > > > > context, and the OF in_port should be > > stored > > > > there as well (if it is not yet). I believe a recirculation should > > > > never change the bridge and the OF in_port. > > @Ben: > > > > Can you confirm? > > > > > > > > I think the order of things is wrong in an upcall. First ofproto should > > > > check if it is a recirculation upcall, > > and > > > > only if it is not, it should derive the bridge and the in_port from the > > > > flow. Today the first recirculation > > check > > > > happens later in upcall_xlate(). > > > > > > > > Can you look into that option and propose a better way to solve the > > > > issue? > > > > > > > > Thanks, Jan > > > > > > > > > -----Original Message----- > > > > > From: Zoltán Balogh > > > > > Sent: Friday, 08 December, 2017 10:56 > > > > > To: [email protected] > > > > > Cc: Zoltán Balogh <[email protected]>; Jan Scheurich > > > > > <[email protected]>; Ben Pfaff > > > > <[email protected]> > > > > > Subject: [PATCH] tunnel: fix tnl_find() after packet_type changed > > > > > > > > > > During xlate, it can happen that tnl_find() is invoked when flow > > > > > packet_type has been already changed. For instance, pop_mpls and > > > > > resubmit actions should be applied to the packet in overlay bridge > > > > > after > > > > > packet was received on a legacy_l3 tunnel port. > > > > > In this case, packet is recirculated after pop_mpls, a new tunnel > > > > > lookup > > > > > is performed in order to find the proper ofproto, however packet_type > > > > > of > > > > > flow is already PT_ETHERNET while the tunnel port mode is > > > > > NETDEV_PT_LEGACY_L3. So, no tunnel port is found and the packet is > > > > > dropped. > > > > > > > > > > This fix does an additional tnl_find() if no port is found. It looks > > > > > for > > > > > L3 tunnel port in case of L2 packet and vice versa. > > > > > > > > > > Signed-off-by: Zoltan Balogh <[email protected]> > > > > > CC: Jan Scheurich <[email protected]> > > > > > CC: Ben Pfaff <[email protected]> > > > > > Fixes: 875ab13020b1 ("userspace: Handling of versatile tunnel ports") > > > > > --- > > > > > ofproto/tunnel.c | 13 +++++++++++++ > > > > > 1 file changed, 13 insertions(+) > > > > > > > > > > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c > > > > > index 1676f4d46..d497f026b 100644 > > > > > --- a/ofproto/tunnel.c > > > > > +++ b/ofproto/tunnel.c > > > > > @@ -585,6 +585,19 @@ tnl_find(const struct flow *flow) > > > > > OVS_REQ_RDLOCK(rwlock) > > > > > if (tnl_port) { > > > > > return tnl_port; > > > > > } > > > > > + > > > > > + /* Maybe flow->packet_type has been already > > > > > changed during > > > > > + * xlate, so let's try to find L2 port for L3 > > > > > packet or > > > > > + * L3 port for L2 packet. */ > > > > > + if (pt_ns(flow->packet_type) == > > > > > OFPHTN_ETHERTYPE) { > > > > > + match.pt_mode = NETDEV_PT_LEGACY_L2; > > > > > + } else { > > > > > + match.pt_mode = NETDEV_PT_LEGACY_L3; > > > > > + } > > > > > + tnl_port = tnl_find_exact(&match, map); > > > > > + if (tnl_port) { > > > > > + return tnl_port; > > > > > + } > > > > > } > > > > > > > > > > i++; > > > > > -- > > > > > 2.14.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
