On Mon, Jul 8, 2024 at 8:23 AM David Marchand <[email protected]> wrote: > > 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?
I've been thinking about changing the trunc member to a flags bit array lately in other contexts as well. It would be useful to mark if a batch has fragmented packets, if there is mixed memory between mbuf's and other types of packet source memory, there should be other uses as well. Having batch flags would remove a few instances of iterating over the whole batch. I think this is a seperate patch though. > > > > 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
