| Message ID | 20231201210523.3085560-1-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 6b172358852e8aa265e2078249406ea46c9bcf36 |
| Headers | show |
| Series | [ovs-dev] tunnel: Do not carry source port from a previous tunnel. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
Hi Ilya, thanks for the patch! I’ve validated it against my setup with geneve tunnels hairpin. With [0] applied, tc offloading works as expected: no warnings/errors in ovs-vswitchd.log, and only first packets of a flow reach system/CPU/are visible in tcpdump. Also, ovs-appctl dpctl/dump-flows type=offloaded outputs related flows. I’ve got some more test results, were offloading is not working (snat/dnat_and_snat usecases), but I’ll start a new thread for this discussion. Tested-by: Vladislav Odintsov <odivlad@gmail.com> > On 2 Dec 2023, at 00:04, Ilya Maximets <i.maximets@ovn.org> wrote: > > If a packet is received from a UDP tunnel, it has a source port > populated in the tunnel metadata. This field cannot be read or > changed with OpenFlow or the tunnel configuration. However, while > sending this packet to a different tunnel, the value remains in > the metadata and is being sent to the datapath to use as a source > port for this new tunnel. Tunnel implementations largely ignore > this value, and it is a random value from a different tunnel > anyway. > > Clear it while sending to a different tunnel, so the unnecessary > information is not being passed to the datapath. This additionally > allows traffic from one tunnel to anther to be offloaded to TC, > as TC doesn't allow setting the source port at all. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > > Vladislav, it would be great if you can test this change on your > setup along with the previous offloading fix: > https://patchwork.ozlabs.org/project/openvswitch/patch/20231030140031.75157-1-i.maximets@ovn.org/ > > ofproto/tunnel.c | 1 + > tests/tunnel.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 45 insertions(+) > > diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c > index 3455ed233..80ddee78a 100644 > --- a/ofproto/tunnel.c > +++ b/ofproto/tunnel.c > @@ -432,6 +432,7 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, > flow->tunnel.ipv6_dst = in6addr_any; > } > } > + flow->tunnel.tp_src = 0; /* Do not carry from a previous tunnel. */ > flow->tunnel.tp_dst = cfg->dst_port; > if (!cfg->out_key_flow) { > flow->tunnel.tun_id = cfg->out_key; > diff --git a/tests/tunnel.at b/tests/tunnel.at > index 05613bcc3..282651ac7 100644 > --- a/tests/tunnel.at > +++ b/tests/tunnel.at > @@ -333,6 +333,50 @@ set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,flags(df|key))),1 > OVS_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([tunnel - set_tunnel VXLAN]) > +OVS_VSWITCHD_START([dnl > + add-port br0 p1 -- set Interface p1 type=vxlan options:key=flow \ > + options:remote_ip=1.1.1.1 ofport_request=1 \ > + -- add-port br0 p2 -- set Interface p2 type=vxlan options:key=flow \ > + options:remote_ip=2.2.2.2 ofport_request=2 \ > + -- add-port br0 p3 -- set Interface p3 type=vxlan options:key=flow \ > + options:remote_ip=3.3.3.3 ofport_request=3 \ > + -- add-port br0 p4 -- set Interface p4 type=vxlan options:key=flow \ > + options:remote_ip=4.4.4.4 ofport_request=4]) > +AT_DATA([flows.txt], [dnl > +actions=set_tunnel:1,output:1,set_tunnel:2,output:2,set_tunnel:3,output:3,set_tunnel:5,output:4 > +]) > + > +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP > +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > + > +AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl > + br0 65534/100: (dummy-internal) > + p1 1/4789: (vxlan: key=flow, remote_ip=1.1.1.1) > + p2 2/4789: (vxlan: key=flow, remote_ip=2.2.2.2) > + p3 3/4789: (vxlan: key=flow, remote_ip=3.3.3.3) > + p4 4/4789: (vxlan: key=flow, remote_ip=4.4.4.4) > +]) > + > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], [Datapath actions: dnl > +set(tunnel(tun_id=0x1,dst=1.1.1.1,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl > +set(tunnel(tun_id=0x2,dst=2.2.2.2,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl > +set(tunnel(tun_id=0x3,dst=3.3.3.3,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl > +set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,tp_dst=4789,flags(df|key))),4789 > +]) > + > +dnl With pre-existing tunnel metadata. > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(tun_id=0x1,src=1.1.1.1,dst=5.5.5.5,tp_src=12345,tp_dst=4789,ttl=64,flags(key)),in_port(4789),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) > +AT_CHECK([tail -1 stdout], [0], [Datapath actions: dnl > +set(tunnel(tun_id=0x2,dst=2.2.2.2,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl > +set(tunnel(tun_id=0x3,dst=3.3.3.3,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl > +set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,tp_dst=4789,flags(df|key))),4789 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > + > AT_SETUP([tunnel - key]) > OVS_VSWITCHD_START([dnl > add-port br0 p1 -- set Interface p1 type=gre options:key=1 \ > -- > 2.43.0 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Regards, Vladislav Odintsov
On 1 Dec 2023, at 22:04, Ilya Maximets wrote: > If a packet is received from a UDP tunnel, it has a source port > populated in the tunnel metadata. This field cannot be read or > changed with OpenFlow or the tunnel configuration. However, while > sending this packet to a different tunnel, the value remains in > the metadata and is being sent to the datapath to use as a source > port for this new tunnel. Tunnel implementations largely ignore > this value, and it is a random value from a different tunnel > anyway. > > Clear it while sending to a different tunnel, so the unnecessary > information is not being passed to the datapath. This additionally > allows traffic from one tunnel to anther to be offloaded to TC, > as TC doesn't allow setting the source port at all. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- I tested this patch on multiple datapath configurations, and it all seems to pass. Thanks for looking into this, and adding the test case. //Eelco Acked-by: Eelco Chaudron <echaudro@redhat.com>
On 12/4/23 16:24, Eelco Chaudron wrote: > > > On 1 Dec 2023, at 22:04, Ilya Maximets wrote: > >> If a packet is received from a UDP tunnel, it has a source port >> populated in the tunnel metadata. This field cannot be read or >> changed with OpenFlow or the tunnel configuration. However, while >> sending this packet to a different tunnel, the value remains in >> the metadata and is being sent to the datapath to use as a source >> port for this new tunnel. Tunnel implementations largely ignore >> this value, and it is a random value from a different tunnel >> anyway. >> >> Clear it while sending to a different tunnel, so the unnecessary >> information is not being passed to the datapath. This additionally >> allows traffic from one tunnel to anther to be offloaded to TC, >> as TC doesn't allow setting the source port at all. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- > > I tested this patch on multiple datapath configurations, and it all seems to pass. > > Thanks for looking into this, and adding the test case. > > //Eelco > > Acked-by: Eelco Chaudron <echaudro@redhat.com> > Thanks, Eelco and Valdislav! Applied. This one is on the edge between a bug fix and enhancement, but it seems to be low risk, so I backported it down to 2.17 as well. Best regards, Ilya Maximets.
diff --git a/ofproto/tunnel.c b/ofproto/tunnel.c index 3455ed233..80ddee78a 100644 --- a/ofproto/tunnel.c +++ b/ofproto/tunnel.c @@ -432,6 +432,7 @@ tnl_port_send(const struct ofport_dpif *ofport, struct flow *flow, flow->tunnel.ipv6_dst = in6addr_any; } } + flow->tunnel.tp_src = 0; /* Do not carry from a previous tunnel. */ flow->tunnel.tp_dst = cfg->dst_port; if (!cfg->out_key_flow) { flow->tunnel.tun_id = cfg->out_key; diff --git a/tests/tunnel.at b/tests/tunnel.at index 05613bcc3..282651ac7 100644 --- a/tests/tunnel.at +++ b/tests/tunnel.at @@ -333,6 +333,50 @@ set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,flags(df|key))),1 OVS_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([tunnel - set_tunnel VXLAN]) +OVS_VSWITCHD_START([dnl + add-port br0 p1 -- set Interface p1 type=vxlan options:key=flow \ + options:remote_ip=1.1.1.1 ofport_request=1 \ + -- add-port br0 p2 -- set Interface p2 type=vxlan options:key=flow \ + options:remote_ip=2.2.2.2 ofport_request=2 \ + -- add-port br0 p3 -- set Interface p3 type=vxlan options:key=flow \ + options:remote_ip=3.3.3.3 ofport_request=3 \ + -- add-port br0 p4 -- set Interface p4 type=vxlan options:key=flow \ + options:remote_ip=4.4.4.4 ofport_request=4]) +AT_DATA([flows.txt], [dnl +actions=set_tunnel:1,output:1,set_tunnel:2,output:2,set_tunnel:3,output:3,set_tunnel:5,output:4 +]) + +OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP +AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) + +AT_CHECK([ovs-appctl dpif/show | tail -n +3], [0], [dnl + br0 65534/100: (dummy-internal) + p1 1/4789: (vxlan: key=flow, remote_ip=1.1.1.1) + p2 2/4789: (vxlan: key=flow, remote_ip=2.2.2.2) + p3 3/4789: (vxlan: key=flow, remote_ip=3.3.3.3) + p4 4/4789: (vxlan: key=flow, remote_ip=4.4.4.4) +]) + +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(100),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], [Datapath actions: dnl +set(tunnel(tun_id=0x1,dst=1.1.1.1,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl +set(tunnel(tun_id=0x2,dst=2.2.2.2,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl +set(tunnel(tun_id=0x3,dst=3.3.3.3,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl +set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,tp_dst=4789,flags(df|key))),4789 +]) + +dnl With pre-existing tunnel metadata. +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'tunnel(tun_id=0x1,src=1.1.1.1,dst=5.5.5.5,tp_src=12345,tp_dst=4789,ttl=64,flags(key)),in_port(4789),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout]) +AT_CHECK([tail -1 stdout], [0], [Datapath actions: dnl +set(tunnel(tun_id=0x2,dst=2.2.2.2,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl +set(tunnel(tun_id=0x3,dst=3.3.3.3,ttl=64,tp_dst=4789,flags(df|key))),4789,dnl +set(tunnel(tun_id=0x5,dst=4.4.4.4,ttl=64,tp_dst=4789,flags(df|key))),4789 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP + AT_SETUP([tunnel - key]) OVS_VSWITCHD_START([dnl add-port br0 p1 -- set Interface p1 type=gre options:key=1 \
If a packet is received from a UDP tunnel, it has a source port populated in the tunnel metadata. This field cannot be read or changed with OpenFlow or the tunnel configuration. However, while sending this packet to a different tunnel, the value remains in the metadata and is being sent to the datapath to use as a source port for this new tunnel. Tunnel implementations largely ignore this value, and it is a random value from a different tunnel anyway. Clear it while sending to a different tunnel, so the unnecessary information is not being passed to the datapath. This additionally allows traffic from one tunnel to anther to be offloaded to TC, as TC doesn't allow setting the source port at all. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- Vladislav, it would be great if you can test this change on your setup along with the previous offloading fix: https://patchwork.ozlabs.org/project/openvswitch/patch/20231030140031.75157-1-i.maximets@ovn.org/ ofproto/tunnel.c | 1 + tests/tunnel.at | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+)