On Fri, Dec 15, 2023 at 8:05 PM Ilya Maximets <[email protected]> wrote:
>
> On 12/11/23 16:39, Mike Pattrick wrote:
> > Test that netdev-dummy is able to send and recieve segment offloaded
> > packets.
> >
> > Signed-off-by: Mike Pattrick <[email protected]>
> >
> > ---
> > v2: Fix clang build error: mutex needed to access netdev_dummy members
> >
> > Signed-off-by: Mike Pattrick <[email protected]>
> > ---
> > lib/netdev-dummy.c | 32 +++++++++++++++++++++++++++-
> > tests/dpif-netdev.at | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 81 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> > index 8c6e6d448..9d9a28892 100644
> > --- a/lib/netdev-dummy.c
> > +++ b/lib/netdev-dummy.c
> > @@ -44,6 +44,7 @@
> > #include "unaligned.h"
> > #include "timeval.h"
> > #include "unixctl.h"
> > +#include "userspace-tso.h"
> > #include "reconnect.h"
> >
> > VLOG_DEFINE_THIS_MODULE(netdev_dummy);
> > @@ -152,6 +153,8 @@ struct netdev_dummy {
> > bool ol_ip_csum OVS_GUARDED;
> > /* Flag RX packet with good csum. */
> > bool ol_ip_csum_set_good OVS_GUARDED;
> > + /* Set the segment size for netdev TSO support. */
> > + int ol_tso_segsz OVS_GUARDED;
> > };
> >
> > /* Max 'recv_queue_len' in struct netdev_dummy. */
> > @@ -806,6 +809,10 @@ netdev_dummy_get_config(const struct netdev *dev,
> > struct smap *args)
> > smap_add_format(args, "ol_ip_csum_set_good", "%s", "true");
> > }
> >
> > + if (netdev->ol_tso_segsz && userspace_tso_enabled()) {
> > + smap_add_format(args, "ol_tso_segsz", "%d", netdev->ol_tso_segsz);
> > + }
> > +
> > /* 'dummy-pmd' specific config. */
> > if (!netdev_is_pmd(dev)) {
> > goto exit;
> > @@ -937,6 +944,14 @@ netdev_dummy_set_config(struct netdev *netdev_, const
> > struct smap *args,
> > netdev_->ol_flags |= NETDEV_TX_OFFLOAD_IPV4_CKSUM;
> > }
> >
> > + if (userspace_tso_enabled()) {
> > + netdev->ol_tso_segsz = smap_get_int(args, "ol_tso_segsz", 0);
> > + if (netdev->ol_tso_segsz) {
> > + netdev_->ol_flags |= (NETDEV_TX_OFFLOAD_TCP_TSO
> > + | NETDEV_TX_OFFLOAD_TCP_CKSUM);
> > + }
> > + }
> > +
> > netdev_change_seq_changed(netdev_);
> >
> > /* 'dummy-pmd' specific config. */
> > @@ -1119,6 +1134,15 @@ netdev_dummy_rxq_recv(struct netdev_rxq *rxq_,
> > struct dp_packet_batch *batch,
> > /* The netdev hardware sets the flag when the packet has good
> > csum. */
> > dp_packet_ol_set_ip_csum_good(packet);
> > }
> > +
> > + if (userspace_tso_enabled() && netdev->ol_tso_segsz) {
> > + dp_packet_set_tso_segsz(packet, netdev->ol_tso_segsz);
> > + dp_packet_hwol_set_tcp_seg(packet);
> > + dp_packet_hwol_set_tx_ip_csum(packet);
> > + dp_packet_hwol_set_tx_ipv4(packet);
> > + dp_packet_hwol_set_csum_tcp(packet);
> > + }
> > +
> > ovs_mutex_unlock(&netdev->mutex);
> >
> > dp_packet_batch_init_packet(batch, packet);
> > @@ -1174,6 +1198,12 @@ netdev_dummy_send(struct netdev *netdev, int qid,
> > DP_PACKET_BATCH_FOR_EACH(i, packet, batch) {
> > const void *buffer = dp_packet_data(packet);
> > size_t size = dp_packet_size(packet);
> > + bool is_tso;
> > +
> > + ovs_mutex_lock(&dev->mutex);
> > + is_tso = userspace_tso_enabled() && dev->ol_tso_segsz &&
> > + dp_packet_hwol_is_tso(packet);
> > + ovs_mutex_unlock(&dev->mutex);
> >
> > if (!dp_packet_is_eth(packet)) {
> > error = EPFNOSUPPORT;
> > @@ -1194,7 +1224,7 @@ netdev_dummy_send(struct netdev *netdev, int qid,
> > if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
> > max_size += VLAN_HEADER_LEN;
> > }
> > - if (size > max_size) {
> > + if (size > max_size && !is_tso) {
> > error = EMSGSIZE;
> > break;
> > }
> > diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> > index d0359b5ea..2d9b8eb5f 100644
> > --- a/tests/dpif-netdev.at
> > +++ b/tests/dpif-netdev.at
> > @@ -810,6 +810,56 @@ AT_CHECK_UNQUOTED([tail -n 1 p2.pcap.txt], [0],
> > [${good_expected}
> > OVS_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > +AT_SETUP([userspace offload - tso])
>
> Hi, Mike. I didn't review the code, but see a few comments
> for the test below.
>
> > +OVS_VSWITCHD_START(
> > + [set Open_vSwitch . other_config:userspace-tso-enable=true -- \
> > + add-br br1 -- set bridge br1 datapath-type=dummy -- \
> > + add-port br1 p1 -- \
> > + set Interface p1 type=dummy -- \
> > + add-port br1 p2 -- \
> > + set Interface p2 type=dummy --])
> > +
> > +dnl Modify the ip_dst addr to force changing the IP csum.
>
> This comment doesn't match the flow.
>
> > +AT_CHECK([ovs-ofctl add-flow br1 in_port=p1,actions=output:p2])
> > +
> > +flow_s="\
> > + eth_src=8a:bf:7e:2f:05:84,eth_dst=0a:8f:39:4f:e0:73,dl_type=0x0800,\
> > +
> > nw_src=192.168.123.2,nw_dst=192.168.123.1,nw_proto=6,nw_ttl=64,nw_frag=no,\
> > + tp_src=54392,tp_dst=5201,tcp_flags=ack"
> > +
> > +payload=$(dd if=/dev/zero bs=2000 count=1 | hexdump -ve '1/1 "%02x"')
>
> This seems unnecessary, you may use printf instead, since it's just
> an all-zero payload, but also...
>
> > +
> > +big_frame=$(ovs-ofctl compose-packet --bare "${flow_s}" "${payload}")
>
> You're not using this hex string anywhere beside passing to receive
> appctl. You may just provide an odp-formatted flow to it instead
> and netdev-dummy/receive supports --len option, so there is no need
> to generate the payload either.
>
> > +
> > +dnl Send from tso to no-tso.
> > +AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2.pcap])
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum=true])
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_ip_csum_set_good=false])
> > +AT_CHECK([ovs-vsctl set Interface p1 options:ol_tso_segsz=500])
>
> Can be set in a single command?
>
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${big_frame}])
> > +AT_CHECK([tcpdump -vvnnr p2.pcap > p2.pcap.txt 2>&1])
>
> Please, don't use tcpdump is "unit" tests. Use ovs-pcap instead.
> It's not that pretty, but more reliable aand also always available.
> Tests also have to check for tcpdump before using it. Same goes
> for nc and other non-trivial utilities.
Good to know, I'll update the test to this standard.
-M
>
> > +
> > +dnl Send from tso to tso.
> > +dnl AT_CHECK([ovs-vsctl set Interface p2 options:tx_pcap=p2_tso.pcap])
> > +AT_CHECK([ovs-vsctl set Interface p2 options:ol_ip_csum=true])
> > +AT_CHECK([ovs-vsctl set Interface p2 options:ol_ip_csum_set_good=false])
> > +AT_CHECK([ovs-vsctl set Interface p2 options:ol_tso_segsz=500])
> > +AT_CHECK([ovs-appctl netdev-dummy/receive p1 ${big_frame}])
> > +AT_CHECK([tcpdump -vvnnr p2.pcap > p2.pcap.txt 2>&1])
> > +
> > +dnl Verify 5 packets with good checksums, 4 500 bytes and one 2000 bytes.
> > +AT_CHECK([grep -c "length 2000" p2.pcap.txt], [0], [1
> > +])
> > +AT_CHECK([grep -c "length 500" p2.pcap.txt], [0], [4
> > +])
> > +AT_CHECK([grep -c "bad cksum" p2.pcap.txt], [1], [0
> > +])
> > +AT_CHECK([grep -c "incorrect" p2.pcap.txt], [1], [0
> > +])
>
> This is going to be less pretty without tcpdump, but tcpdump is
> not a given in these tests. Segment sizez may be reduced to make
> the packets shorter, I guess. Trailing zeroes may be cut off.
>
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> > +
> > AT_SETUP([dpif-netdev - revalidators handle dp modification fail
> > correctly])
> > OVS_VSWITCHD_START(
> > [add-port br0 p1 \
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
