Hi. Thanks for the patch! One general comment for the subject line: 'netdev' doesn't seem to be a correct 'area' for the change. It should be 'ofproto-dpif-xlate' instead. And, please, add the space after the ':'. 'summary' should preferably start with a capital letter and end with a period.
More details on how to format the patch here: Documentation/internals/contributing/submitting-patches.rst See some more comments inline. On 4/5/22 01:01, Thilak Raj Surendra Babu wrote: > when a packet is received over an access port that needs to be sent over a > vxlan tunnel, > the access port VLAN id is used in the lookup leading to a wrong packet being > crafted and > sent over the tunnel.Clear out the flow 's VLAN field as it should not be > used while performing > mac lookup for the outer tunnel and also at this point the VLAN action > related to inner flow is > already committed. Please, limit the line length for a commit message. 72 characters is the most common limit for it. > Please, add a 'Fixes' tag. > Signed-off-by: Thilak Raj Surendra Babu <[email protected]> > --- While sending new versions of a patch, please, add here a brief explanation of what changed between versions. E.g.: Version 2: - Fixed line length warning from checkpatch. - Dropped unrelated changes. > ofproto/ofproto-dpif-xlate.c | 3 ++ > tests/system-traffic.at | 54 ++++++++++++++++++++++++++++++++++++ I see you added a system test. That is OK, good to have one. However, the issue doesn't require any actual network to be reproduced. So, we should have a unit test for it. I know that Rosemarie worked on the same problem over the past few weeks, and she already has a working unit test. So, I think it would be great if you can cooperate and include the unit test into the patch. Rosemarie, could you share what you have? > 2 files changed, 57 insertions(+) > > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index bfd4960dd..34f74aade 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -3541,6 +3541,9 @@ propagate_tunnel_data_to_flow__(struct flow *dst_flow, > { > dst_flow->dl_dst = dmac; > dst_flow->dl_src = smac; An empty line here would be nice. > + /* Clear VLAN entries which do not apply for tunnel flows */ Comments should end with a period. > + memset (dst_flow->vlans, 0, No need for extra space between the function name and the '('. > + sizeof(union flow_vlan_hdr) * FLOW_MAX_VLAN_HEADERS); Please, avoid use of sizeof with the type as an argument. Use the expression (actual object) as an argument instead. For more details see the coding-style guide: Documentation/internals/contributing/coding-style.rst > > dst_flow->packet_type = htonl(PT_ETH); > dst_flow->nw_dst = src_flow->tunnel.ip_dst; > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 4a7fa49fc..b0003c2e5 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -259,6 +259,60 @@ NS_CHECK_EXEC([at_ns0], [ping -s 3200 -q -c 3 -i 0.3 -w > 2 10.1.1.100 | FORMAT_PI > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > + This extra space is unnecessary. > +AT_SETUP([datapath - ping vlan over vxlan tunnel]) > +OVS_CHECK_TUNNEL_TSO() > +OVS_CHECK_VXLAN() > + > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-underlay]) > + > +AT_CHECK([ovs-vsctl -- add-port br0 patch0 -- set interface patch0 > type=patch options:peer=patch1 -- add-port br-underlay patch1 -- set > interface patch1 type=patch options:peer=patch0]) > + > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > +AT_CHECK([ovs-ofctl add-flow br-underlay "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0) > + > +dnl Set up underlay link from host into the namespace using veth pair. > +ADD_VETH(p0, at_ns0, br-underlay, "172.31.1.1/24") > +AT_CHECK([ip addr add dev br-underlay "172.31.1.100/24"]) > +AT_CHECK([ip link set dev br-underlay up]) > + > + > +dnl Set up tunnel endpoints on OVS outside the namespace and with a native > +dnl linux device inside the namespace. > + > +ADD_NATIVE_TUNNEL([vxlan], [at_vxlan1], [at_ns0], [172.31.1.100], > [10.1.1.1/24], > + [id 0 dstport 4789]) > + > +ADD_OVS_TUNNEL([vxlan], [br-underlay], [at_vxlan0], [172.31.1.1], > [10.1.1.100/24]) > + > +NS_EXEC([at_ns0], [ip link add link at_vxlan1 name at_vxlan1.100 type vlan > id 100]) > + > +NS_EXEC([at_ns0], [ip addr flush dev at_vxlan1]) > +NS_EXEC([at_ns0], [ip addr add dev at_vxlan1.100 "10.1.1.30/24"]) > +NS_EXEC([at_ns0], [ip link set dev at_vxlan1.100 up]) > + > +ADD_NAMESPACES(at_ns1) > +ADD_VETH(p1, at_ns1, br0, "10.1.1.10/24") > + > +AT_CHECK([ovs-vsctl set port ovs-p1 tag=100]) > + > +dnl First, check the underlay > +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 172.31.1.100 | > FORMAT_PING], [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +NS_CHECK_EXEC([at_ns1], [ping -q -c 3 -i 0.3 -w 2 10.1.1.30 | FORMAT_PING], > [0], [dnl > +3 packets transmitted, 3 received, 0% packet loss, time 0ms > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP(["/ofproto_dpif_xlate(revalidator.*)|WARN|over max > translation depth 64.*/d"]) Looks like you have a loop in the test topology. That is not good. Please, remove the loop and you'll not need to filter out warnings. > +AT_CLEANUP > + > + > + One empty line is enough. > AT_SETUP([datapath - ping over vxlan6 tunnel]) > OVS_CHECK_TUNNEL_TSO() > OVS_CHECK_VXLAN_UDP6ZEROCSUM() Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
