Let's allow growing a dp_packet_batch. This will help for software GSO as we can define a helper that will segment all packets and return them in the input batch.
To limit heap allocations, keep an embedded array of packets in the batch structure, and switch to heap allocated packets array when reaching the NETDEV_MAX_BURST previous limit. The refill API becomes simpler, as the only operation needed now is to reset the count of packets when starting a refill loop. ipf and GSO software helper are left untouched (keeping the behavior of accumulating up to NETDEV_MAX_BURST packets) and are updated in next commits. At this point in time, no part of OVS should grow batches. Add a check in OVS_VSWITCHD_STOP, with a macro to simplify testing later commits. Signed-off-by: David Marchand <[email protected]> --- Changes since v1: - introduced dp_packet_batch_capacity helper earlier in the series, Changes since RFC v1: - renamed field as "capacity", --- lib/dp-packet-gso.c | 6 ++-- lib/dp-packet.c | 25 +++++++++++++++ lib/dp-packet.h | 67 +++++++++++++++++------------------------ lib/dpif-netdev.c | 6 ++-- lib/ipf.c | 14 ++++----- lib/netdev-dpdk.c | 4 +-- lib/netdev.c | 6 ++-- lib/odp-execute.c | 2 +- tests/ofproto-macros.at | 10 ++++++ tests/test-conntrack.c | 4 +-- 10 files changed, 84 insertions(+), 60 deletions(-) diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c index bceb851fb9..22e8a6cd18 100644 --- a/lib/dp-packet-gso.c +++ b/lib/dp-packet-gso.c @@ -196,7 +196,7 @@ dp_packet_gso__(struct dp_packet *p, struct dp_packet_batch **batches, /* 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)) { + if (dp_packet_batch_size(curr_batch) == NETDEV_MAX_BURST) { curr_batch++; } dp_packet_batch_add(curr_batch, p); @@ -228,7 +228,7 @@ dp_packet_gso__(struct dp_packet *p, struct dp_packet_batch **batches, dp_packet_gso_update_segment(seg, i, n_segs, tso_segsz, udp_tnl, gre_tnl); - if (dp_packet_batch_is_full(curr_batch)) { + if (dp_packet_batch_size(curr_batch) == NETDEV_MAX_BURST) { curr_batch++; } dp_packet_batch_add(curr_batch, seg); @@ -241,7 +241,7 @@ last_seg: dp_packet_gso_update_segment(seg, n_segs - 1, n_segs, tso_segsz, udp_tnl, gre_tnl); - if (dp_packet_batch_is_full(curr_batch)) { + if (dp_packet_batch_size(curr_batch) == NETDEV_MAX_BURST) { curr_batch++; } dp_packet_batch_add(curr_batch, seg); diff --git a/lib/dp-packet.c b/lib/dp-packet.c index ac8fbe3b97..fb52fec686 100644 --- a/lib/dp-packet.c +++ b/lib/dp-packet.c @@ -18,6 +18,7 @@ #include <stdlib.h> #include <string.h> +#include "coverage.h" #include "dp-packet.h" #include "netdev-afxdp.h" #include "netdev-dpdk.h" @@ -25,6 +26,8 @@ #include "openvswitch/dynamic-string.h" #include "util.h" +COVERAGE_DEFINE(dp_packet_batch_grow); + static void dp_packet_init__(struct dp_packet *b, size_t allocated, enum dp_packet_source source) { @@ -665,3 +668,25 @@ dp_packet_ol_send_prepare(struct dp_packet *p, uint64_t flags) } } } + +void +dp_packet_batch_grow(struct dp_packet_batch *batch, size_t count) +{ + struct dp_packet **old_packets; + + COVERAGE_INC(dp_packet_batch_grow); + + if (batch->packets == batch->embedded) { + old_packets = NULL; + } else { + old_packets = batch->packets; + } + + batch->packets = xrealloc(old_packets, + (batch->capacity + count) + * sizeof *batch->packets); + if (!old_packets) { + memcpy(batch->packets, batch->embedded, sizeof batch->embedded); + } + batch->capacity += count; +} diff --git a/lib/dp-packet.h b/lib/dp-packet.h index e144cf8d75..fbb2320b3c 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -862,7 +862,9 @@ enum { NETDEV_MAX_BURST = 32 }; /* Maximum number packets in a batch. */ struct dp_packet_batch { size_t count; bool trunc; /* true if the batch needs truncate. */ - struct dp_packet *packets[NETDEV_MAX_BURST]; + struct dp_packet **packets; + size_t capacity; + struct dp_packet *embedded[NETDEV_MAX_BURST]; }; static inline void @@ -876,40 +878,31 @@ static inline void dp_packet_batch_init(struct dp_packet_batch *batch) { dp_packet_batch_reset(batch); + batch->capacity = ARRAY_SIZE(batch->embedded); + batch->packets = batch->embedded; } -static inline void -dp_packet_batch_add__(struct dp_packet_batch *batch, - struct dp_packet *packet, size_t limit) -{ - if (batch->count < limit) { - batch->packets[batch->count++] = packet; - } else { - dp_packet_delete(packet); - } -} +void dp_packet_batch_grow(struct dp_packet_batch *batch, size_t count); -/* When the batch is full, 'packet' will be dropped and freed. */ static inline void dp_packet_batch_add(struct dp_packet_batch *batch, struct dp_packet *packet) { - dp_packet_batch_add__(batch, packet, NETDEV_MAX_BURST); + if (OVS_UNLIKELY(batch->count == batch->capacity)) { + dp_packet_batch_grow(batch, NETDEV_MAX_BURST); + } + batch->packets[batch->count++] = packet; } static inline void dp_packet_batch_add_array(struct dp_packet_batch *batch, struct dp_packet *packets[], size_t n) { - size_t count = MIN(n, NETDEV_MAX_BURST - batch->count); - - if (count) { - memcpy(&batch->packets[batch->count], packets, - count * sizeof packets[0]); - batch->count += count; - } - for (size_t i = count; i < n; i++) { - dp_packet_delete(packets[i]); + if (OVS_UNLIKELY(batch->count + n > batch->capacity)) { + size_t delta = batch->count + n - batch->capacity; + dp_packet_batch_grow(batch, ROUND_UP(delta, NETDEV_MAX_BURST)); } + memcpy(&batch->packets[batch->count], packets, n * sizeof packets[0]); + batch->count += n; } static inline size_t @@ -918,21 +911,19 @@ dp_packet_batch_size(const struct dp_packet_batch *batch) return batch->count; } -/* Clear 'batch' for refill. Use dp_packet_batch_refill() to add - * packets back into the 'batch'. */ +static inline size_t +dp_packet_batch_capacity(const struct dp_packet_batch *batch) +{ + return batch->capacity; +} + +/* Clear 'batch' for refill. */ static inline void dp_packet_batch_refill_init(struct dp_packet_batch *batch) { batch->count = 0; }; -static inline void -dp_packet_batch_refill(struct dp_packet_batch *batch, - struct dp_packet *packet, size_t idx) -{ - dp_packet_batch_add__(batch, packet, MIN(NETDEV_MAX_BURST, idx + 1)); -} - static inline void dp_packet_batch_init_packet(struct dp_packet_batch *batch, struct dp_packet *p) { @@ -947,12 +938,6 @@ dp_packet_batch_is_empty(const struct dp_packet_batch *batch) return !dp_packet_batch_size(batch); } -static inline bool -dp_packet_batch_is_full(const struct dp_packet_batch *batch) -{ - return dp_packet_batch_size(batch) == NETDEV_MAX_BURST; -} - #define DP_PACKET_BATCH_FOR_EACH(IDX, PACKET, BATCH) \ for (size_t IDX = 0; IDX < dp_packet_batch_size(BATCH); IDX++) \ if (PACKET = (BATCH)->packets[IDX], true) @@ -961,7 +946,7 @@ dp_packet_batch_is_full(const struct dp_packet_batch *batch) * dropped after going through each packet in the 'BATCH'. * * For packets to stay in the 'BATCH', they need to be refilled back - * into the 'BATCH' by calling dp_packet_batch_refill(). Caller owns + * into the 'BATCH' by calling dp_packet_batch_add(). Caller owns * the packets that are not refilled. * * Caller needs to supply 'SIZE', that stores the current number of @@ -994,8 +979,12 @@ dp_packet_batch_clone(struct dp_packet_batch *dst, } static inline void -dp_packet_batch_destroy(struct dp_packet_batch *batch OVS_UNUSED) +dp_packet_batch_destroy(struct dp_packet_batch *batch) { + if (batch->packets != batch->embedded) { + free(batch->packets); + } + batch->packets = NULL; } static inline void diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 7d46d747fd..0e4d4b7866 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -6424,7 +6424,7 @@ dp_netdev_run_meter(struct dp_netdev *dp, struct dp_packet_batch *packets_, dp_packet_delete(packet); } else { /* Meter accepts packet. */ - dp_packet_batch_refill(packets_, packet, j); + dp_packet_batch_add(packets_, packet); } } @@ -7209,7 +7209,7 @@ smc_lookup_batch(struct dp_netdev_pmd_thread *pmd, /* SMC missed. Group missed packets together at * the beginning of the 'packets' array. */ - dp_packet_batch_refill(packets_, packet, i); + dp_packet_batch_add(packets_, packet); /* Preserve the order of packet for flow batching. */ index_map[n_missed] = recv_idx; @@ -7424,7 +7424,7 @@ dfc_processing(struct dp_netdev_pmd_thread *pmd, } else { /* Exact match cache missed. Group missed packets together at * the beginning of the 'packets' array. */ - dp_packet_batch_refill(packets_, packet, i); + dp_packet_batch_add(packets_, packet); /* Preserve the order of packet for flow batching. */ index_map[n_missed] = map_cnt; diff --git a/lib/ipf.c b/lib/ipf.c index 3f60ed81cc..7f9faf4f1f 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -973,16 +973,16 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb, ovs_mutex_lock(&ipf->ipf_lock); if (!ipf_handle_frag(ipf, pkt, dl_type, zone, now, hash_basis, &rp)) { - dp_packet_batch_refill(pb, pkt, pb_idx); + dp_packet_batch_add(pb, pkt); } else { - if (rp && !dp_packet_batch_is_full(pb)) { - dp_packet_batch_refill(pb, rp->pkt, pb_idx); + if (rp && dp_packet_batch_size(pb) != NETDEV_MAX_BURST) { + dp_packet_batch_add(pb, rp->pkt); rp->list->reass_execute_ctx = rp->pkt; } } ovs_mutex_unlock(&ipf->ipf_lock); } else { - dp_packet_batch_refill(pb, pkt, pb_idx); + dp_packet_batch_add(pb, pkt); } } } @@ -1033,7 +1033,7 @@ ipf_send_frags_in_list(struct ipf *ipf, struct ipf_list *ipf_list, while (ipf_list->last_sent_idx < ipf_list->last_inuse_idx) { struct dp_packet *pkt = ipf_list->frag_list[ipf_list->last_sent_idx + 1].pkt; - if (!dp_packet_batch_is_full(pb)) { + if (dp_packet_batch_size(pb) != NETDEV_MAX_BURST) { dp_packet_batch_add(pb, pkt); ipf_list->last_sent_idx++; atomic_count_dec(&ipf->nfrag); @@ -1148,7 +1148,7 @@ ipf_execute_reass_pkts(struct ipf *ipf, struct dp_packet_batch *pb, LIST_FOR_EACH_SAFE (rp, rp_list_node, &ipf->reassembled_pkt_list) { if (!rp->list->reass_execute_ctx && rp->list->key.dl_type == dl_type && - !dp_packet_batch_is_full(pb)) { + dp_packet_batch_size(pb) != NETDEV_MAX_BURST) { dp_packet_batch_add(pb, rp->pkt); rp->list->reass_execute_ctx = rp->pkt; } @@ -1241,7 +1241,7 @@ ipf_post_execute_reass_pkts(struct ipf *ipf, free(rp); rp = NULL; } else { - dp_packet_batch_refill(pb, pkt, pb_idx); + dp_packet_batch_add(pb, pkt); } } } diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index cab69f00db..9912c0e279 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -3245,13 +3245,13 @@ dpdk_copy_batch_to_mbuf(struct netdev *netdev, struct dp_packet_batch *batch) DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) { if (OVS_UNLIKELY(packet->source == DPBUF_DPDK)) { - dp_packet_batch_refill(batch, packet, i); + dp_packet_batch_add(batch, packet); } else { struct dp_packet *pktcopy; pktcopy = dpdk_copy_dp_packet_to_mbuf(dev->dpdk_mp->mp, packet); if (pktcopy) { - dp_packet_batch_refill(batch, pktcopy, i); + dp_packet_batch_add(batch, pktcopy); } dp_packet_delete(packet); diff --git a/lib/netdev.c b/lib/netdev.c index 26669ff4da..38c83e7395 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -843,7 +843,7 @@ netdev_send_tso(struct netdev *netdev, int qid, COVERAGE_INC(netdev_soft_seg_good); } } else { - if (dp_packet_batch_is_full(curr_batch)) { + if (dp_packet_batch_size(curr_batch) == NETDEV_MAX_BURST) { curr_batch++; } dp_packet_batch_add(curr_batch, packet); @@ -960,7 +960,7 @@ netdev_pop_header(struct netdev *netdev, struct dp_packet_batch *batch) * recirculated.*/ dp_packet_reset_offload(packet); pkt_metadata_init_conn(&packet->md); - dp_packet_batch_refill(batch, packet, i); + dp_packet_batch_add(batch, packet); } } } @@ -1036,7 +1036,7 @@ netdev_push_header(const struct netdev *netdev, data); pkt_metadata_init(&packet->md, data->out_port); - dp_packet_batch_refill(batch, packet, i); + dp_packet_batch_add(batch, packet); } } diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 7ece235605..618fb5ac81 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -1121,7 +1121,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, DP_PACKET_BATCH_REFILL_FOR_EACH (i, num, packet, batch) { if (pop_nsh(packet)) { - dp_packet_batch_refill(batch, packet, i); + dp_packet_batch_add(batch, packet); } else { COVERAGE_INC(datapath_drop_nsh_decap_error); dp_packet_delete(packet); diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at index 59f3b07ca1..4d6adbb788 100644 --- a/tests/ofproto-macros.at +++ b/tests/ofproto-macros.at @@ -185,6 +185,15 @@ m4_divert_pop([PREPARE_TESTS]) m4_define([TESTABLE_LOG], [-vPATTERN:ANY:'%c|%p|%m']) +m4_define([OVS_CHECK_BATCH_GROW], + [m4_if([$1], [], + [AT_CHECK([test -f exp_batch_grow || echo 0 > exp_batch_grow]) + AT_CHECK([cat exp_batch_grow > expout])], + [AT_CHECK([echo $1 > expout])]) + AT_CHECK([ovs-appctl coverage/read-counter dp_packet_batch_grow], [0], [expout]) + AT_CHECK([mv -f expout exp_batch_grow]) +]) + # _OVS_VSWITCHD_START([vswitchd-aux-args] [dbinit-aux-args]) # # Creates an empty database and starts ovsdb-server. @@ -382,6 +391,7 @@ m4_divert_pop([PREPARE_TESTS]) # OVS_VSWITCHD_STOP(["/expected error/d"]) m4_define([OVS_VSWITCHD_STOP], [AT_CHECK([check_logs $1]) + OVS_CHECK_BATCH_GROW() OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) OVS_APP_EXIT_AND_WAIT([ovsdb-server])]) diff --git a/tests/test-conntrack.c b/tests/test-conntrack.c index c8afe4812f..2babe989c4 100644 --- a/tests/test-conntrack.c +++ b/tests/test-conntrack.c @@ -154,9 +154,9 @@ prepare_packets(size_t n, bool change, unsigned tid, ovs_be16 *dl_type) struct dp_packet_batch *pkt_batch = xzalloc(sizeof *pkt_batch); size_t i; - ovs_assert(n <= ARRAY_SIZE(pkt_batch->packets)); - dp_packet_batch_init(pkt_batch); + ovs_assert(n <= dp_packet_batch_capacity(pkt_batch)); + for (i = 0; i < n; i++) { uint16_t udp_dst = change ? 2+1 : 2; struct dp_packet *pkt = build_packet(1 + tid, udp_dst, dl_type); -- 2.54.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
