git sha: [email protected]
Author: David Marchand <[email protected]>
One line subject: dp-packet-gso: Refactor software segmentation code.
This commit refactors the software segmentation code in dp_packet_gso() by
extracting two helper functions and optimizing the first segment handling.
The refactoring changes dp_packet_gso() from returning bool to void and
removes the netdev_soft_seg_drops counter.
> diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c
> index fe7186ddf4..6b8da8b370 100644
> --- a/lib/dp-packet-gso.c
> +++ b/lib/dp-packet-gso.c
> @@ -29,19 +29,19 @@ VLOG_DEFINE_THIS_MODULE(dp_packet_gso);
> *
> * The new packet is initialized with 'hdr_len' bytes from the
> * start of packet 'p' and then appended with 'data_len' bytes
> - * from the 'data' buffer.
> + * from the packet 'p' at offset 'data_off'.
> *
> * Note: The packet headers are not updated. */
> static struct dp_packet *
> dp_packet_gso_seg_new(const struct dp_packet *p, size_t hdr_len,
> - const char *data, size_t data_len)
> + size_t data_off, size_t data_len)
> {
> struct dp_packet *seg = dp_packet_new_with_headroom(hdr_len + data_len,
>
> dp_packet_headroom(p));
>
> /* Append the original packet headers and then the payload. */
> dp_packet_put(seg, dp_packet_data(p), hdr_len);
> - dp_packet_put(seg, data, data_len);
> + dp_packet_put(seg, (char *) dp_packet_data(p) + data_off, data_len);
[ ... ]
> -/* Perform software segmentation on packet 'p'.
> - *
> - * Segments packet 'p' into the array of preallocated batches in 'batches',
> - * updating the 'batches' pointer as needed and returns true.
> - *
> - * Returns false if the packet cannot be segmented. */
> -bool
> +/* Perform software segmentation on packet 'p'.
> + *
> + * Segments packet 'p' into the array of preallocated batches in 'batches',
> + * updating the 'batches' pointer as needed. */
> +void
> dp_packet_gso(struct dp_packet *p, struct dp_packet_batch **batches)
> {
> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> struct dp_packet_batch *curr_batch = *batches;
> [ ... ]
> struct dp_packet *seg;
> uint16_t tso_segsz;
> - uint16_t inner_ip_id = 0;
> - uint16_t outer_ip_id = 0;
> - struct dp_packet *seg;
> - uint16_t tcp_offset;
> - uint32_t tcp_seq;
> - bool outer_ipv4;
> - int hdr_len;
> - int seg_len;
> - bool udp_tnl = dp_packet_tunnel_vxlan(p)
> - || dp_packet_tunnel_geneve(p);
> - bool gre_tnl = dp_packet_tunnel_gre(p);
> + size_t data_len;
> + size_t hdr_len;
> + int n_segs;
>
> tso_segsz = dp_packet_get_tso_segsz(p);
> - if (!tso_segsz) {
> - VLOG_WARN_RL(&rl, "GSO packet with len %d with no segment size.",
> - dp_packet_size(p));
> - return false;
> - }
> + ovs_assert(tso_segsz);
Does this code risk hitting the assert when processing legitimate packets?
The original function returned false for packets without tso_segsz, but now
it would abort. Are there any conditions where packets might reach this
function without tso_segsz being set?
> + n_segs = dp_packet_gso_nr_segs(p);
> +
> + /* Put back the first segment in the batch, it will be trimmed after
> + * all segments have been copied. */
> + if (dp_packet_batch_is_full(curr_batch)) {
> + curr_batch++;
> + }
> + dp_packet_batch_add(curr_batch, p);
[ ... ]
> + for (int i = 1; i < n_segs - 1; i++) {
> + seg = dp_packet_gso_seg_new(p, hdr_len, hdr_len + i * tso_segsz,
> + tso_segsz);
Does this arithmetic for the data_off parameter handle edge cases correctly?
When i=1, data_off becomes hdr_len + tso_segsz, which should point to the
second segment's payload start.
> + dp_packet_gso_update_segment(seg, i, n_segs, tso_segsz);
> +
> + if (dp_packet_batch_is_full(curr_batch)) {
> + curr_batch++;
> + }
> + dp_packet_batch_add(curr_batch, seg);
> + }
> +
> + /* Create the last segment. */
> + seg = dp_packet_gso_seg_new(p, hdr_len, hdr_len + (n_segs - 1) *
> tso_segsz,
> + data_len - (n_segs - 1) * tso_segsz);
Can this arithmetic for the last segment's data_len result in negative
values or underflow? If data_len is smaller than (n_segs - 1) * tso_segsz,
the subtraction could wrap around since data_len is size_t.
> + dp_packet_gso_update_segment(seg, n_segs - 1, n_segs, tso_segsz);
> +
> + if (dp_packet_batch_is_full(curr_batch)) {
> + curr_batch++;
> + }
> + dp_packet_batch_add(curr_batch, seg);
> +
> + /* Trim the first segment and reset TSO. */
> + dp_packet_set_size(p, hdr_len + tso_segsz);
> + dp_packet_set_tso_segsz(p, 0);
> + dp_packet_gso_update_segment(p, 0, n_segs, tso_segsz);
[ ... ]
> diff --git a/lib/netdev.c b/lib/netdev.c
> index 6a05e9a7e5..f493dab005 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -71,7 +71,6 @@ COVERAGE_DEFINE(netdev_add_router);
> COVERAGE_DEFINE(netdev_get_stats);
> COVERAGE_DEFINE(netdev_push_header_drops);
> COVERAGE_DEFINE(netdev_soft_seg_good);
> -COVERAGE_DEFINE(netdev_soft_seg_drops);
[ ... ]
> @@ -843,17 +842,12 @@ netdev_send_tso(struct netdev *netdev, int qid,
> curr_batch = batches;
> DP_PACKET_BATCH_REFILL_FOR_EACH (k, size, packet, batch) {
> if (dp_packet_get_tso_segsz(packet)) {
> - if (dp_packet_gso(packet, &curr_batch)) {
> - COVERAGE_INC(netdev_soft_seg_good);
> - } else {
> - COVERAGE_INC(netdev_soft_seg_drops);
> - }
> - dp_packet_delete(packet);
> + dp_packet_gso(packet, &curr_batch);
> + COVERAGE_INC(netdev_soft_seg_good);
Does this code leak the original packet? The previous version called
dp_packet_delete(packet) after dp_packet_gso(), but now the packet p is
added to curr_batch and reused as the first segment. Is this intentional
behavior change properly handled?
> } else {
> if (dp_packet_batch_is_full(curr_batch)) {
> curr_batch++;
> }
> -
> dp_packet_batch_add(curr_batch, packet);
> }
> }
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
