Hi Aaron, Thanks for the detailed review!
> Weird that this is here. All other tests would also not work if there > were no netns or OVS module. Why were these added? You're right, the modprobe and netns pre-checks are unnecessary. run_test already gates on the module, and ovs_add_netns_and_veths will fail naturally if netns isn't available. I'll remove both pre-flight blocks in v5. > We don't like to make extra files when not requested. For example, > this is making some pcap file but do we need it? Agreed. The pcap capture was overly cautious, ping success/failure already tells us whether the VLAN tag was correctly stripped (or not). > We also have a specific spawn function `ovs_netns_spawn_daemon` that > should take care of cleaning up at least the PID. Understood. I'll use existing helpers rather than rolling custom infrastructure. > Do we really need these tcpdump checks? The ping commands above > should already capture when we are expecting tagged / untagged > traffic. Makes sense. In v5 I'll drop the start_capture/stop_capture helpers entirely and rely solely on ping results: - Baseline: ping without VLAN flow succeeds (ARP resolution works) - Negative: ping with tagged traffic and no pop_vlan fails - Positive: ping with tagged traffic and pop_vlan succeeds This will significantly simplify the test. I was previously working as QE, so my instinct was to add defensive pre-checks and packet-level capture for every test. I'm still adjusting to the kernel selftest convention of keeping tests minimal and relying on existing infrastructure. Appreciate the guidance. Combined with the depth-check removal from patch 1/2 feedback, v5 will be a much leaner series. B,R Minxi Aaron Conole <[email protected]> 于2026年5月4日周一 23:51写道： > > Hi Minxi, > > Thanks for the test (and the support infra in 1/2). Questions below. > > Minxi Hou <[email protected]> writes: > > > Add a test for the OVS datapath POP_VLAN action. The test uses > > the VLAN sub-interface of a veth pair to generate 802.1Q tagged > > frames and verifies that pop_vlan correctly strips the VLAN tag > > before delivery via pcap frame capture. > > > > The test has two verifications: > > - Negative: forward tagged frames without pop_vlan, verify the > > VLAN tag is still present on the egress interface. > > - Positive: apply pop_vlan, verify the tag is removed and an > > untagged ICMP echo request arrives at the egress interface. > > > > Static ARP entries avoid the complexity of VLAN-tagged ARP > > resolution (the egress side has no VLAN sub-interface and cannot > > process tagged ARP). > > > > Signed-off-by: Minxi Hou <[email protected]> > > --- > > v1 -> v2: rebase to latest net-next/main, drop --base=auto > > > > .../selftests/net/openvswitch/openvswitch.sh | 132 ++++++++++++++++++ > > 1 file changed, 132 insertions(+) > > > > diff --git a/tools/testing/selftests/net/openvswitch/openvswitch.sh > > b/tools/testing/selftests/net/openvswitch/openvswitch.sh > > index b327d3061ed5..cd614ff7b740 100755 > > --- a/tools/testing/selftests/net/openvswitch/openvswitch.sh > > +++ b/tools/testing/selftests/net/openvswitch/openvswitch.sh > > @@ -27,6 +27,7 @@ tests=" > > upcall_interfaces ovs: test the upcall > > interfaces > > tunnel_metadata ovs: test extraction of > > tunnel metadata > > drop_reason drop: test drop reasons are > > emitted > > + pop_vlan vlan-pop: POP_VLAN > > action strips 802.1Q tag > > nit: The whitespace alignment above is slightly off. > > > psample psample: Sampling packets > > with psample" > > > > info() { > > @@ -830,6 +831,137 @@ test_tunnel_metadata() { > > return 0 > > } > > > > +test_pop_vlan() { > > + modprobe -q openvswitch 2>/dev/null || true > > + [ -d /sys/module/openvswitch ] || return $ksft_skip > > + ip netns add __test_pop_vlan_netns__ 2>/dev/null || \ > > + { info "CONFIG_NET_NS missing"; return $ksft_skip; } > > + ip netns del __test_pop_vlan_netns__ 2>/dev/null > > Weird that this is here. All other tests would also not work if there > were no netns or OVS module. Why were these added? > > > + modprobe -q 8021q 2>/dev/null || true > > + [ -d /sys/module/8021q ] || { info "CONFIG_VLAN_8021Q missing"; > > return $ksft_skip; } > > + > > + local sbx="test_pop_vlan" > > + sbx_add "$sbx" || return $ksft_skip > > + ovs_add_dp "$sbx" vlandp || return 1 > > + > > + # --- baseline: untagged forwarding --- > > + ovs_add_netns_and_veths "$sbx" vlandp ns1 veth1 ns1veth 192.0.2.1/24 > > || return 1 > > + ovs_add_netns_and_veths "$sbx" vlandp ns2 veth2 ns2veth 192.0.2.2/24 > > || return 1 > > + > > + # ARP + IPv4 bidirectional (all untagged) > > + ovs_add_flow "$sbx" vlandp \ > > + 'in_port(1),eth(),eth_type(0x0806),arp()' '2' || return 1 > > + ovs_add_flow "$sbx" vlandp \ > > + 'in_port(2),eth(),eth_type(0x0806),arp()' '1' || return 1 > > + ovs_add_flow "$sbx" vlandp \ > > + 'in_port(1),eth(),eth_type(0x0800),ipv4()' '2' || return 1 > > + ovs_add_flow "$sbx" vlandp \ > > + 'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1 > > + ip netns exec ns1 ping -c 3 -W 2 192.0.2.2 || return 1 > > + > > + # --- POP_VLAN test --- > > + # Register cleanup before creating resources (safe on failure paths) > > + on_exit "ip -n ns1 link del ns1veth.10 2>/dev/null || true > > + ip -n ns2 addr del 198.51.100.2/24 dev ns2veth 2>/dev/null > > || true" > > + > > + # ns1: VLAN sub-interface generates tagged frames > > + ip -n ns1 link add link ns1veth name ns1veth.10 type vlan id 10 > > + ip -n ns1 addr add 198.51.100.1/24 dev ns1veth.10 > > + ip -n ns1 link set ns1veth.10 up > > + > > + # ns2: no VLAN sub-interface. POP delivers untagged frames to ns2veth > > + ip -n ns2 addr add 198.51.100.2/24 dev ns2veth > > + > > + # veth disable VLAN offload + GRO (ensure kernel software tag > > processing) > > + if command -v ethtool >/dev/null 2>&1; then > > + ip netns exec ns1 ethtool -k ns1veth 2>/dev/null | grep -q > > vlan-offload && \ > > + ip netns exec ns1 ethtool -K ns1veth rx-vlan-offload > > off \ > > + tx-vlan-offload off gro off 2>/dev/null || > > true > > + ip netns exec ns2 ethtool -k ns2veth 2>/dev/null | grep -q > > vlan-offload && \ > > + ip netns exec ns2 ethtool -K ns2veth rx-vlan-offload > > off \ > > + tx-vlan-offload off gro off 2>/dev/null || > > true > > + fi > > + > > + ovs_del_flows "$sbx" vlandp > > + > > + # Static ARP avoids VLAN-tagged ARP complexity (ns2 has no VLAN > > + # sub-interface, so tagged ARP would be invisible to ns2). > > + local ns1veth10mac ns2mac > > + ns1veth10mac=$(ip -n ns1 link show ns1veth.10 | \ > > + awk '/link\/ether/ {print $2}') > > + ns2mac=$(ip -n ns2 link show ns2veth | \ > > + awk '/link\/ether/ {print $2}') > > + [ -n "$ns1veth10mac" ] && echo "$ns1veth10mac" | \ > > + grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1 > > + [ -n "$ns2mac" ] && echo "$ns2mac" | \ > > + grep -qE "^([0-9a-fA-F]{2}:){5}[0-9a-fA-F]{2}$" || return 1 > > + ip -n ns1 neigh replace 198.51.100.2 lladdr "$ns2mac" \ > > + dev ns1veth.10 nud permanent || return 1 > > + ip -n ns2 neigh replace 198.51.100.1 lladdr "$ns1veth10mac" \ > > + dev ns2veth nud permanent || return 1 > > + > > + # --- Negative check: fwd without pop_vlan, VLAN tag stays --- > > + ovs_add_flow "$sbx" vlandp \ > > + > > 'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' > > \ > > + '2' || return 1 > > + > > + local pcap_no_pop > > + pcap_no_pop=$(mktemp --suffix=.pcap) > > + on_exit "rm -f $pcap_no_pop" > > + ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap_no_pop" -U & > > + local tpid_no_pop=$! > > + on_exit "kill $tpid_no_pop 2>/dev/null || true" > > + sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5)) > > + > > We don't like to make extra files when not requested. For example, this > is making some pcap file but do we need it? We also have a specific > spawn function `ovs_netns_spawn_daemon` that should take care of > cleaning up at least the PID. > > > + ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \ > > + >/dev/null 2>&1 || true > > + kill $tpid_no_pop 2>/dev/null || true; wait $tpid_no_pop 2>/dev/null > > || true > > + > > + # assert: VLAN tag still present (no pop_vlan in action) > > + tcpdump -nr "$pcap_no_pop" 'vlan' 2>/dev/null | grep -q . || { > > + info "FAIL: negative check: no VLAN tag (expected tag > > present)"; return 1 > > + } > > + > > + ovs_del_flows "$sbx" vlandp > > + > > + # --- Positive: pop_vlan strips tag --- > > + ovs_add_flow "$sbx" vlandp \ > > + > > 'in_port(1),eth(),eth_type(0x8100),vlan(vid=10),encap(eth_type(0x0800),ipv4(src=198.51.100.1,proto=1),icmp())' > > \ > > + 'pop_vlan,2' || return 1 > > + ovs_add_flow "$sbx" vlandp \ > > + 'in_port(2),eth(),eth_type(0x0800),ipv4()' '1' || return 1 > > + > > + local pcap > > + pcap=$(mktemp --suffix=.pcap) > > + on_exit "rm -f $pcap" > > + ip netns exec ns2 tcpdump -nei ns2veth -w "$pcap" -U & > > + local tpid=$! > > + on_exit "kill $tpid 2>/dev/null || true" > > + sleep $((WAIT_TIMEOUT / 5 < 2 ? 2 : WAIT_TIMEOUT / 5)) > > + > > + # ping reply unreachable: ns1veth.10 only accepts tagged frames, > > + # ns2 sends untagged reply -> dropped by ns1veth.10. > > + local ping_rc=0 > > + ip netns exec ns1 ping -I ns1veth.10 -c 3 -W 1 198.51.100.2 \ > > + >/dev/null 2>&1 || ping_rc=$? > > + kill $tpid 2>/dev/null || true; wait $tpid 2>/dev/null || true > > + > > + # ping failure is expected (reply path asymmetric) > > + [ "$ping_rc" -ne 0 ] || \ > > + info "NOTE: ping succeeded unexpectedly (reply reached > > ns1veth.10)" > > + > > + # assert: no VLAN tag (POP succeeded), untagged ICMP echo request > > arrived > > + tcpdump -nr "$pcap" 'vlan' 2>/dev/null | grep -q . && { > > + info "FAIL: POP_VLAN: VLAN tag still present"; return 1 > > + } > > + tcpdump -nr "$pcap" 'icmp and icmp[icmptype]=8' \ > > + 2>/dev/null | grep -q . || { > > + info "FAIL: POP_VLAN: no untagged ICMP echo request"; return 1 > > + } > > Do we really need these tcpdump checks? The ping commands above should > already capture when we are expecting tagged / untagged traffic. > > > + return 0 > > +} > > + > > run_test() { > > ( > > tname="$1" > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
