On Fri, Jul 5, 2024 at 10:44 PM Mike Pattrick <[email protected]> wrote:
> diff --git a/lib/netdev.c b/lib/netdev.c
> index f2d921ed6..866dbf3b7 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -69,8 +69,6 @@ COVERAGE_DEFINE(netdev_received);
> COVERAGE_DEFINE(netdev_sent);
> COVERAGE_DEFINE(netdev_add_router);
> COVERAGE_DEFINE(netdev_get_stats);
> -COVERAGE_DEFINE(netdev_vxlan_tso_drops);
> -COVERAGE_DEFINE(netdev_geneve_tso_drops);
> COVERAGE_DEFINE(netdev_push_header_drops);
> COVERAGE_DEFINE(netdev_soft_seg_good);
> COVERAGE_DEFINE(netdev_soft_seg_drops);
> @@ -910,28 +908,30 @@ netdev_send(struct netdev *netdev, int qid, struct
> dp_packet_batch *batch,
> struct dp_packet *packet;
> int error;
>
> - if (userspace_tso_enabled() &&
> - !(netdev_flags & NETDEV_TX_OFFLOAD_TCP_TSO)) {
> - DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> - if (dp_packet_hwol_is_tso(packet)) {
> - if (dp_packet_hwol_is_tunnel_vxlan(packet)
> - && !(netdev_flags & NETDEV_TX_VXLAN_TNL_TSO)) {
> - VLOG_WARN_RL(&rl, "%s: No VXLAN TSO support",
> - netdev_get_name(netdev));
> - COVERAGE_INC(netdev_vxlan_tso_drops);
> - dp_packet_delete_batch(batch, true);
> - return false;
> + if (userspace_tso_enabled()) {
> + if (!(netdev_flags & NETDEV_TX_OFFLOAD_TCP_TSO)) {
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + if (dp_packet_hwol_is_tso(packet)) {
> + return netdev_send_tso(netdev, qid, batch,
> concurrent_txq);
> }
> -
> - if (dp_packet_hwol_is_tunnel_geneve(packet)
> - && !(netdev_flags & NETDEV_TX_GENEVE_TNL_TSO)) {
> - VLOG_WARN_RL(&rl, "%s: No GENEVE TSO support",
> - netdev_get_name(netdev));
> - COVERAGE_INC(netdev_geneve_tso_drops);
> - dp_packet_delete_batch(batch, true);
> - return false;
> + }
> + } else if (!(netdev_flags & NETDEV_TX_OFFLOAD_OUTER_UDP_CKSUM)) {
Just checking this feature is not enough at this point.
Imagine a driver that has TSO support, but neither outer udp checksum
nor any tunnel tso support (the one I have in mind is net/ixgbe).
IIUC, a batch of (tunneled) tso packets with no request for outer udp
csum would not be segmented in sw (and it would probably be dropped by
hw).
I think this block should be moved after checking the presence of
*tnl_tso flags.
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + if (dp_packet_hwol_is_tso(packet) &&
> + (dp_packet_hwol_is_tunnel_vxlan(packet) ||
> + dp_packet_hwol_is_tunnel_geneve(packet)) &&
> + dp_packet_hwol_is_outer_udp_cksum(packet)) {
> + return netdev_send_tso(netdev, qid, batch,
> concurrent_txq);
> + }
> + }
> + } else if (!(netdev_flags & (NETDEV_TX_VXLAN_TNL_TSO |
> + NETDEV_TX_GENEVE_TNL_TSO))) {
> + DP_PACKET_BATCH_FOR_EACH (i, packet, batch) {
> + if (dp_packet_hwol_is_tso(packet) &&
> + (dp_packet_hwol_is_tunnel_vxlan(packet) ||
> + dp_packet_hwol_is_tunnel_geneve(packet))) {
Note: you can imagine a netdev supporting geneve tso, but not vxlan tso.
A batch containing only tso requests through a geneve tunnel would end
up being segmented in sw.
DPDK drivers either support both or none. And only those drivers
expose these netdev features in OVS.
So I don't think it is a real problem and I am ok with this hunk.
> + return netdev_send_tso(netdev, qid, batch,
> concurrent_txq);
> }
> - return netdev_send_tso(netdev, qid, batch, concurrent_txq);
> }
> }
> }
A final note, I suspect all those checks negatively impact non tso
packets processing when OVS tso is enabled.
Would it be feasible to mark that tso (or a tx offload) has been
requested at the "batch" level?
This is more an optimisation... maybe in the future?
Overall the patch lgtm.
I'll be off soon, so with the issue I mentionned for net/ixgbe fixed,
feel free to add my review tag.
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
