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
