On Wed, 7 Jan 2026 at 13:41, Ilya Maximets <[email protected]> wrote:
>
> On 12/15/25 2:57 PM, David Marchand wrote:
> > Define macros to limit copy/paste (those will be reused later).
> >
> > To ease test debugging, store composed and expected good/bad packets
> > in the unit test working directory, and pass the file names.
> >
> > Example:
> > $ make -C build check TESTSUITEFLAGS="1098 -d -v"
> > ...
> > ../../tests/dpif-netdev.at:808:
> > ovs-vsctl set interface p1 options:ol_ip_rx_csum_set_good=true
> > ../../tests/dpif-netdev.at:808:
> > ovs-appctl netdev-dummy/receive p1 $(cat good_frame)
> > ../../tests/dpif-netdev.at:808:
> > ovs-pcap p2.pcap > p2.pcap.txt 2>&1
> > ../../tests/dpif-netdev.at:808:
> > tail -n 1 p2.pcap.txt
> > ../../tests/dpif-netdev.at:808:
> > ovs-vsctl set interface p1 options:ol_ip_rx_csum_set_good=false
> > ...
> >
> > Signed-off-by: David Marchand <[email protected]>
> > Acked-by: Kevin Traynor <[email protected]>
> > ---
> > tests/dpif-netdev.at | 970 ++++++++-----------------------------------
> > 1 file changed, 175 insertions(+), 795 deletions(-)
> >
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index 90d7497664..bbce1ccee4 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -736,6 +736,43 @@ AT_CHECK([test `ovs-vsctl get Interface p2
> > statistics:tx_q0_packets` -gt 0 -a dn
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +m4_define([CHECK_FWD_PACKET],
> > + [test -z "$3" || AT_CHECK([ovs-vsctl set interface $1 options:$3=true])
> > + AT_CHECK([ovs-appctl netdev-dummy/receive $1 $(cat $4)])
> > + test "$5" = 'none' || {
> > + AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1])
> > + AT_CHECK_UNQUOTED([tail -n 1 $2.pcap.txt], [0], [$(cat $5)
> > +])
> > + }
> > + test -z "$3" || AT_CHECK([ovs-vsctl remove interface $1 options $3])
> > +])
>
> We shouldnt mix the shell and macro checks here. It makes autotest generate
> unnecessary code and makes it harder to userstand which variables are checked
> at runtime and which at compile time. All the arguments here are macro
> arguments that are known at compilation time and so the m4_if macro should
> be used to check them. And we should probbaly use expout instead of the
> unquoted check, i.e.:
>
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index c41c1e18d..955b0a51c 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -737,14 +737,14 @@ OVS_VSWITCHD_STOP
> AT_CLEANUP
>
> m4_define([CHECK_FWD_PACKET],
> - [test -z "$3" || AT_CHECK([ovs-vsctl set interface $1 options:$3=true])
> + [m4_if([$3], [], [], [AT_CHECK([ovs-vsctl set interface $1
> options:$3=true])])
> AT_CHECK([ovs-appctl netdev-dummy/receive $1 $(cat $4)])
> - test "$5" = 'none' || {
> - AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1])
> - AT_CHECK_UNQUOTED([tail -n 1 $2.pcap.txt], [0], [$(cat $5)
> -])
> - }
> - test -z "$3" || AT_CHECK([ovs-vsctl remove interface $1 options $3])
> + m4_if([$5], [none], [], [
> + AT_CHECK([ovs-pcap $2.pcap > $2.pcap.txt 2>&1])
> + AT_CHECK([awk 1 $5 > expout]) dnl Also normalizes the line endings.
> + AT_CHECK([tail -n 1 $2.pcap.txt], [0], [expout])
> + ])
> + m4_if([$3], [], [], [AT_CHECK([ovs-vsctl remove interface $1 options
> $3])])
> ])
>
> dnl CHECK_IP_CHECKSUMS rx_port tx_port good_pkt bad_pkt good_exp bad_exp
It is nicer with m4_if yes.
I don't really see the difference with expout (and there are other
similar constructs that use AT_CHECK_UNQUOTED, in this file).
I don't mind if you update it though.
> ---
>
> > +
> > +dnl CHECK_IP_CHECKSUMS rx_port tx_port good_pkt bad_pkt good_exp bad_exp
> > +dnl
> > +dnl Test combinations of Rx IP checksum flags for a good or bad packet
> > +dnl received on rx_port, and sent over tx_port.
> > +m4_define([CHECK_IP_CHECKSUMS],
> > + [dnl Checks for good packet.
> > + dnl No Rx flag
> > + CHECK_FWD_PACKET($1, $2, , $3, $5)
> > + dnl Rx good
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_good, $3, $5)
> > + dnl Rx bad
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_bad, $3, $5)
> > + dnl Rx partial
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_partial, $3, $5)
> > +
> > + dnl Checks for bad packet.
> > + dnl No Rx flag
> > + CHECK_FWD_PACKET($1, $2, , $4, $6)
> > + dnl Rx good
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_good, $4, $5)
> > + dnl Rx bad
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_bad, $4, $6)
> > + dnl Rx partial
> > + CHECK_FWD_PACKET($1, $2, ol_ip_rx_csum_set_partial, $4, $5)
> > +])
> > +
> > AT_SETUP([dpif-netdev - ip csum offload])
> > AT_KEYWORDS([userspace offload])
> > OVS_VSWITCHD_START(
> > @@ -747,6 +784,8 @@ OVS_VSWITCHD_START(
> >
> > AT_CHECK([ovs-appctl vlog/set netdev_dummy:file:dbg])
> >
> > +AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
> > +
> > dnl Modify the ip_dst addr to force changing the IP csum.
> > AT_CHECK([ovs-ofctl add-flow br1
> > in_port=p1,actions=mod_nw_dst:192.168.1.1,output:p2])
> >
> > @@ -755,160 +794,33 @@ flow_s="\
> > tcp,ip_src=192.168.123.2,ip_dst=192.168.123.1,ip_frag=no,\
> > tcp_src=54392,tcp_dst=5201,tcp_flags=ack"
> >
> > -good_frame=$(ovs-ofctl compose-packet --bare "${flow_s}")
> > +ovs-ofctl compose-packet --bare "${flow_s}" > good_frame
>
> It wasn't possible to check the command before, but now it should be
> wrapped into AT_CHECK, so we have a trace of it in the logs and we're
> sure that it doesn't fail silently.
>
> > -bad_frame=$(echo $good_frame | sed -e "s/6b72/dead/")
> > +cat good_frame | sed -e "s/6b72/dead/" > bad_frame
> > dnl 0x6b72 + (5201-2222) == 0x7715
> > dnl 0xdead + (5201-2222) == 0xea50
> > -bad_expected=$(echo $good_expected | sed -e "s/7715/ea50/")
> > +cat good_expected | sed -e "s/7715/ea50/" > bad_expected
> These cat / sed commands should also be checked. We can't check all of
> them easily, but we should check ones that we can.
>
> Both checks can be easily added with a simple substitution in vim:
>
> :%s/^ovs-ofctl.*/AT_CHECK([\0])/gc
> :%s/^cat .*/AT_CHECK([\0])/gc
Ok for me.
> Since all that mentioned above are just minor test changes, If you agree,
> I can make them on commit. WDYT?
I don't mind if you do those updates (and the comment on patch 6) yourself.
Note that I had some nits from Kevin on the v3 revision, if you could
update them as well, then the series would be good to merge.
Here is what I had locally:
$ git diff tso_csum_v3
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 9bcf14c272..c04d608be6 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -558,7 +558,7 @@ dp_packet_ol_send_prepare(struct dp_packet *p,
uint64_t flags)
if (!dp_packet_ip_checksum_partial(p)
&& !dp_packet_l4_checksum_partial(p)) {
- /* Only checksumming needs actions. */
+ /* No checksumming needed. */
return;
}
diff --git a/lib/packets.c b/lib/packets.c
index d1254676f9..c604416c1b 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -2129,8 +2129,8 @@ packet_udp_tunnel_csum(struct dp_packet *p)
return false;
}
} else {
- /* This optimisation applies only to inner TCP/UDP. */
- return false;
+ /* This optimisation applies only to inner TCP/UDP. */
+ return false;
}
if (!dp_packet_inner_l4_checksum_valid(p)) {
@@ -2156,8 +2156,8 @@ packet_udp_tunnel_csum(struct dp_packet *p)
/* Important: for inner UDP, a null inner_l4_csum here should in theory be
* replaced with 0xffff. However, since the only use of inner_l4_csum is
* for the final outer checksum with a csum_add16() below, we can skip this
- * entirely because adding 0xffff after reducing as the same impact than
- * adding 0. */
+ * entirely because adding 0xffff will have the same effect as adding 0x0
+ * after reducing in csum_finish. */
udp->udp_csum = 0;
if (IP_VER(ip->ip_ihl_ver) == 4) {
Thanks Ilya.
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
