On Fri, 5 Dec 2025 at 18:25, Kevin Traynor <[email protected]> wrote:
>
> On 12/11/2025 17:04, David Marchand via dev wrote:
> > Refactor software segmentation dp_packet_gso() using two helpers:
> > - dp_packet_gso_seg_new: create a segment from the original packet,
> > copying headers and relevant payload,
> > - dp_packet_gso_update_segment: update the headers,
> >
> > For dp_packet_gso_seg_new, prefer working on data offset instead of
> > passing a pointer to the packet data.
> > This simplifies calling this helper: segments data are either of size
> > tso_segsz, or the remaining data for the last segment.
> >
> > For dp_packet_gso_update_segment, (inner and outer) ip_id is based
> > on the value copied from the original packet + the segment index.
> > The first and last segments handling are separated from the other
> > segments:
> > - rather than copying data for the first segment, we can simply resize the
> > original packet (once all segments have been copied), this small
> > optimisation saves one tso_segsz copy,
> > - the last segment needs special handling wrt its length and TCP flags,
> >
> > Finally, replace the check on tso_segsz presence with an ovs_assert() as
> > this code should only be invoked when segmenting.
> > Thanks to this, dp_packet_gso() can be declared void, and the
> > netdev_soft_seg_drops counter has no reason to exist anymore.
> >
> > The code is easier to read this way, and it will be beneficial to the next
> > commit.
> >
>
> > Note: this patch is easier to read if ignoring whitespace changes.
> >
>
> Can drop this from commit msg
Yes, will do.
[snip]
> > +static void
> > +dp_packet_gso_update_segment(struct dp_packet *seg, int seg_no, int n_segs,
> > + uint16_t tso_segsz)
> > {
> > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > - struct dp_packet_batch *curr_batch = *batches;
>
> > + bool udp_tnl = dp_packet_tunnel_vxlan(seg)
> > + || dp_packet_tunnel_geneve(seg);
> > + bool gre_tnl = dp_packet_tunnel_gre(seg);
>
> Only disadvantage I can see from change is that these have to be read
> for every update. I guess it shouldn't make any real difference and
> adding them to the API doesn't seem very nice either
I agree, reading the current segment properties is unneeded, because
the properties of the original packet are known from the start.
This is an internal API, I don't mind updating it: we are already
passing segment numbers which are not that elegant to me :-).
Maybe a cleaner API would be to pass the original packet, and let the
compiler notice we always refer to the same info (idem with nr_segs
and tso_segsz) for all segments?
--
David Marchand
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
