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]> --- lib/dp-packet-gso.c | 6 ++-- lib/dp-packet.c | 24 ++++++++++++++++ lib/dp-packet.h | 61 +++++++++++++++-------------------------- lib/dpif-netdev.c | 6 ++-- lib/ipf.c | 15 +++++----- 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, 77 insertions(+), 61 deletions(-) diff --git a/lib/dp-packet-gso.c b/lib/dp-packet-gso.c index bceb851fb9..83a07c8e31 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 (curr_batch->count == 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 (curr_batch->count == 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 (curr_batch->count == 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 c04d608be6..e4b6878b6d 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) { @@ -646,3 +649,24 @@ 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->size + count) * sizeof *batch->packets); + if (!old_packets) { + memcpy(batch->packets, batch->embedded, sizeof batch->embedded); + } + batch->size += count; +} diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 2540534cc3..03d1eda6a6 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 size; + 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_metadata(batch); + batch->size = 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->size)) { + 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->size)) { + size_t delta = batch->count + n - batch->size; + 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,13 @@ 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'. */ +/* 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 +932,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 +940,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 +973,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 47517ec010..9f13874a76 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -6933,7 +6933,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); } } @@ -7742,7 +7742,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; @@ -7958,7 +7958,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..551261d4e8 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 && pb->count != 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 (pb->count != NETDEV_MAX_BURST) { dp_packet_batch_add(pb, pkt); ipf_list->last_sent_idx++; atomic_count_dec(&ipf->nfrag); @@ -1066,7 +1066,6 @@ ipf_send_completed_frags(struct ipf *ipf, struct dp_packet_batch *pb, struct ipf_list *ipf_list; LIST_FOR_EACH_SAFE (ipf_list, list_node, &ipf->frag_complete_list) { - if ((ipf_list->key.dl_type == htons(ETH_TYPE_IPV6)) != v6) { continue; } @@ -1148,7 +1147,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)) { + pb->count != NETDEV_MAX_BURST) { dp_packet_batch_add(pb, rp->pkt); rp->list->reass_execute_ctx = rp->pkt; } @@ -1241,7 +1240,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 923191da84..ca96269d6e 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -3198,13 +3198,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 8bd4b43db4..1e50d2f007 100644 --- a/lib/netdev.c +++ b/lib/netdev.c @@ -851,7 +851,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 (curr_batch->count == NETDEV_MAX_BURST) { curr_batch++; } dp_packet_batch_add(curr_batch, packet); @@ -968,7 +968,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); } } } @@ -1042,7 +1042,7 @@ netdev_push_header(const struct netdev *netdev, netdev->netdev_class->push_header(netdev, packet, 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 b9446fb25f..5756e60b94 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -1242,7 +1242,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 2afcbb38f4..6aece06216 100644 --- a/tests/test-conntrack.c +++ b/tests/test-conntrack.c @@ -60,9 +60,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 <= pkt_batch->size); + 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.53.0 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
