Re: [PATCH net-next v2 2/2] selftests: openvswitch: add pop_vlan test

[侯敏熙]

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 <aconole@xxxxxxxxxx> 于2026年5月4日周一 23:51写道：
>
> Hi Minxi,
>
> Thanks for the test (and the support infra in 1/2).  Questions below.
>
> Minxi Hou <houminxi@xxxxxxxxx> 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 <houminxi@xxxxxxxxx>
> > ---
> > 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"
>