From patchwork Tue Nov 24 04:54:33 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jarno Rajahalme X-Patchwork-Id: 547817 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (unknown [IPv6:2600:3c00::f03c:91ff:fe6e:bdf7]) by ozlabs.org (Postfix) with ESMTP id 672451402A9 for ; Tue, 24 Nov 2015 15:55:59 +1100 (AEDT) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id EC01910763; Mon, 23 Nov 2015 20:55:54 -0800 (PST) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx3v3.cudamail.com (mx3.cudamail.com [64.34.241.5]) by archives.nicira.com (Postfix) with ESMTPS id 05ED3106DC for ; Mon, 23 Nov 2015 20:55:52 -0800 (PST) Received: from bar4.cudamail.com (localhost [127.0.0.1]) by mx3v3.cudamail.com (Postfix) with ESMTPS id 8ED5E1616D4 for ; Mon, 23 Nov 2015 21:55:51 -0700 (MST) X-ASG-Debug-ID: 1448340951-03dc217b9d238620001-byXFYA Received: from mx3-pf3.cudamail.com ([192.168.14.3]) by bar4.cudamail.com with ESMTP id 6pfrA6adUutLx6pt (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 23 Nov 2015 21:55:51 -0700 (MST) X-Barracuda-Envelope-From: jarno@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.3 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx3-pf3.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 24 Nov 2015 04:58:20 -0000 Received-SPF: pass (mx3-pf3.cudamail.com: SPF record at ovn.org designates 217.70.183.196 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.196 X-Barracuda-RBL-IP: 217.70.183.196 Received: from mfilter36-d.gandi.net (mfilter36-d.gandi.net [217.70.178.167]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id 157071720B3; Tue, 24 Nov 2015 05:55:49 +0100 (CET) X-Virus-Scanned: Debian amavisd-new at mfilter36-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter36-d.gandi.net (mfilter36-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id unqfxjEQ9BAe; Tue, 24 Nov 2015 05:55:47 +0100 (CET) X-Originating-IP: 208.91.1.34 Received: from sc9-mailhost1.vmware.com (unknown [208.91.1.34]) (Authenticated sender: jarno@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 46E0A1720AD; Tue, 24 Nov 2015 05:55:46 +0100 (CET) X-CudaMail-Envelope-Sender: jarno@ovn.org From: Jarno Rajahalme To: dev@openvswitch.org X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-V3-1122072205 X-CudaMail-DTE: 112315 X-CudaMail-Originating-IP: 217.70.183.196 Date: Mon, 23 Nov 2015 20:54:33 -0800 X-ASG-Orig-Subj: [##CM-V3-1122072205##][PATCH v3 2/4] odp-execute: Support dropping packets. Message-Id: <1448340875-2070-3-git-send-email-jarno@ovn.org> X-Mailer: git-send-email 2.1.4 In-Reply-To: <1448340875-2070-1-git-send-email-jarno@ovn.org> References: <1448340875-2070-1-git-send-email-jarno@ovn.org> X-Barracuda-Connect: UNKNOWN[192.168.14.3] X-Barracuda-Start-Time: 1448340951 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Subject: [ovs-dev] [PATCH v3 2/4] odp-execute: Support dropping packets. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dev-bounces@openvswitch.org Sender: "dev" Meter action can drop or modify packets, so the execution framework is changed to allow for this. Also, a meter action can appear alone (e.g., to measure traffic that is to be dropped), so the existing drop implementation is not sufficient any more. The action execution framework is changed in three ways: 1. Action execution can shrink the number of packets in a batch to be further processed by the remaining actions. 2. Whenever a packet is 'stolen' (e.g., for output), the corresponding packet pointer is changed to NULL. NULLed pointers must be excluded from further processing by using the change #1 above. Sometimes this means that the packet pointers are shuffled so that remaining action processing never needs to check for the NULL pointers explicitly. 3. Packet deletion is centralized to the odp_execute_actions() function. This make memory leaks less likely to appear as new kinds of action are added later. Signed-off-by: Jarno Rajahalme --- lib/dp-packet.h | 13 ++++++++-- lib/dpif-netdev.c | 53 +++++++++++++++++++++----------------- lib/dpif.c | 6 +++-- lib/netdev-bsd.c | 8 +----- lib/netdev-dpdk.c | 55 +++++++++++++--------------------------- lib/netdev-dummy.c | 8 +----- lib/netdev-linux.c | 9 +------ lib/netdev-provider.h | 4 +++ lib/odp-execute.c | 70 ++++++++++++++++++++++++++++----------------------- lib/odp-execute.h | 12 +++++---- lib/packets.c | 14 ++++------- 11 files changed, 120 insertions(+), 132 deletions(-) diff --git a/lib/dp-packet.h b/lib/dp-packet.h index 5044ce0..8899bc8 100644 --- a/lib/dp-packet.h +++ b/lib/dp-packet.h @@ -108,7 +108,16 @@ struct dp_packet *dp_packet_clone_with_headroom(const struct dp_packet *, struct dp_packet *dp_packet_clone_data(const void *, size_t); struct dp_packet *dp_packet_clone_data_with_headroom(const void *, size_t, size_t headroom); -static inline void dp_packet_delete(struct dp_packet *); +static inline void dp_packet_delete__(struct dp_packet *); + +/* NULL the packet pointer to mark the packet as deleted. */ +#define dp_packet_delete(PKT) \ + do { \ + struct dp_packet **pkt__ = &(PKT); \ + \ + dp_packet_delete__(*pkt__); \ + *pkt__ = NULL; \ + } while (0) static inline void *dp_packet_at(const struct dp_packet *, size_t offset, size_t size); @@ -147,7 +156,7 @@ static inline bool dp_packet_equal(const struct dp_packet *, /* Frees memory that 'b' points to, as well as 'b' itself. */ static inline void -dp_packet_delete(struct dp_packet *b) +dp_packet_delete__(struct dp_packet *b) { if (b) { if (b->source == DPBUF_DPDK) { diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 10d94e9..9dde5f0 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -3478,14 +3478,12 @@ dpif_netdev_register_upcall_cb(struct dpif *dpif, upcall_callback *cb, } static void -dp_netdev_drop_packets(struct dp_packet **packets, int cnt, bool may_steal) +dp_netdev_drop_packets(struct dp_packet **packets, int cnt) { - if (may_steal) { - int i; + int i; - for (i = 0; i < cnt; i++) { - dp_packet_delete(packets[i]); - } + for (i = 0; i < cnt; i++) { + dp_packet_delete(packets[i]); } } @@ -3519,7 +3517,8 @@ dp_netdev_clone_pkt_batch(struct dp_packet **dst_pkts, } } -static void +/* Mark stolen packets by setting the corresponding packet pointer to NULL. */ +static int dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, const struct nlattr *a, bool may_steal) OVS_NO_THREAD_SAFETY_ANALYSIS @@ -3537,7 +3536,6 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, p = dp_netdev_lookup_port(dp, u32_to_odp(nl_attr_get_u32(a))); if (OVS_LIKELY(p)) { netdev_send(p->netdev, pmd->tx_qid, packets, cnt, may_steal); - return; } break; @@ -3556,10 +3554,11 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, (*depth)++; dp_netdev_input(pmd, packets, cnt); (*depth)--; - } else { - dp_netdev_drop_packets(tnl_pkt, cnt, !may_steal); + goto stolen; + } else if (!may_steal) { + /* Drop the cloned packets. */ + dp_netdev_drop_packets(tnl_pkt, cnt); } - return; } break; @@ -3579,7 +3578,6 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, err = netdev_pop_header(p->netdev, packets, cnt); if (!err) { - for (i = 0; i < cnt; i++) { packets[i]->md.in_port.odp_port = portno; } @@ -3587,10 +3585,11 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, (*depth)++; dp_netdev_input(pmd, packets, cnt); (*depth)--; - } else { - dp_netdev_drop_packets(tnl_pkt, cnt, !may_steal); + goto stolen; + } else if (!may_steal) { + /* Drop the cloned packets. */ + dp_netdev_drop_packets(tnl_pkt, cnt); } - return; } } break; @@ -3625,7 +3624,6 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, ofpbuf_uninit(&actions); fat_rwlock_unlock(&dp->upcall_rwlock); - return; } break; @@ -3634,8 +3632,8 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, struct dp_packet *recirc_pkts[NETDEV_MAX_BURST]; if (!may_steal) { - dp_netdev_clone_pkt_batch(recirc_pkts, packets, cnt); - packets = recirc_pkts; + dp_netdev_clone_pkt_batch(recirc_pkts, packets, cnt); + packets = recirc_pkts; } for (i = 0; i < cnt; i++) { @@ -3645,11 +3643,10 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, (*depth)++; dp_netdev_input(pmd, packets, cnt); (*depth)--; - - return; + goto stolen; + } else { + VLOG_WARN("Packet dropped. Max recirculation depth exceeded."); } - - VLOG_WARN("Packet dropped. Max recirculation depth exceeded."); break; case OVS_ACTION_ATTR_CT: @@ -3660,6 +3657,8 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, break; case OVS_ACTION_ATTR_METER: + break; /* Never drop. */ + case OVS_ACTION_ATTR_PUSH_VLAN: case OVS_ACTION_ATTR_POP_VLAN: case OVS_ACTION_ATTR_PUSH_MPLS: @@ -3673,7 +3672,15 @@ dp_execute_cb(void *aux_, struct dp_packet **packets, int cnt, OVS_NOT_REACHED(); } - dp_netdev_drop_packets(packets, cnt, may_steal); + return cnt; +stolen: + /* Mark stolen packets. */ + if (may_steal) { + for (i = 0; i < cnt; i++) { + packets[i] = NULL; + } + } + return cnt; } static void diff --git a/lib/dpif.c b/lib/dpif.c index 06669d3..7389c8f 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1089,7 +1089,7 @@ struct dpif_execute_helper_aux { /* This is called for actions that need the context of the datapath to be * meaningful. */ -static void +static int dpif_execute_helper_cb(void *aux_, struct dp_packet **packets, int cnt, const struct nlattr *action, bool may_steal OVS_UNUSED) { @@ -1136,7 +1136,8 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet **packets, int cnt, if (md->tunnel.ip_dst) { ofpbuf_uninit(&execute_actions); } - break; + + return (aux->error == 0) ? cnt : 0; } case OVS_ACTION_ATTR_HASH: @@ -1152,6 +1153,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet **packets, int cnt, case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } + return 0; } /* Executes 'execute' by performing most of the actions in userspace and diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c index 60e5615..a241c04 100644 --- a/lib/netdev-bsd.c +++ b/lib/netdev-bsd.c @@ -684,7 +684,7 @@ netdev_bsd_rxq_drain(struct netdev_rxq *rxq_) */ static int netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED, - struct dp_packet **pkts, int cnt, bool may_steal) + struct dp_packet **pkts, int cnt, bool may_steal OVS_UNUSED) { struct netdev_bsd *dev = netdev_bsd_cast(netdev_); const char *name = netdev_get_name(netdev_); @@ -731,12 +731,6 @@ netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED, } ovs_mutex_unlock(&dev->mutex); - if (may_steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(pkts[i]); - } - } - return error; } diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index e3a0771..5512901 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1061,7 +1061,7 @@ netdev_dpdk_vhost_update_tx_counters(struct netdev_stats *stats, static void __netdev_dpdk_vhost_send(struct netdev *netdev, struct dp_packet **pkts, - int cnt, bool may_steal) + int cnt) { struct netdev_dpdk *vhost_dev = netdev_dpdk_cast(netdev); struct virtio_net *virtio_dev = netdev_dpdk_get_virtio(vhost_dev); @@ -1071,9 +1071,9 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, struct dp_packet **pkts, if (OVS_UNLIKELY(!is_vhost_running(virtio_dev))) { rte_spinlock_lock(&vhost_dev->stats_lock); - vhost_dev->stats.tx_dropped+= cnt; + vhost_dev->stats.tx_dropped += cnt; rte_spinlock_unlock(&vhost_dev->stats_lock); - goto out; + return; } /* There is vHost TX single queue, So we need to lock it for TX. */ @@ -1119,20 +1119,13 @@ __netdev_dpdk_vhost_send(struct netdev *netdev, struct dp_packet **pkts, netdev_dpdk_vhost_update_tx_counters(&vhost_dev->stats, pkts, total_pkts, cnt); rte_spinlock_unlock(&vhost_dev->stats_lock); - -out: - if (may_steal) { - int i; - - for (i = 0; i < total_pkts; i++) { - dp_packet_delete(pkts[i]); - } - } } +/* Queue 'pkts' for transmission. The ownership of the referred memory is + * taken. */ inline static void dpdk_queue_pkts(struct netdev_dpdk *dev, int qid, - struct rte_mbuf **pkts, int cnt) + struct rte_mbuf **pkts, int cnt) { struct dpdk_tx_queue *txq = &dev->tx_q[qid]; uint64_t diff_tsc; @@ -1157,6 +1150,9 @@ dpdk_queue_pkts(struct netdev_dpdk *dev, int qid, dpdk_queue_flush__(dev, qid); } } + + /* Clear the transferred pointers to mark them as 'taken'. */ + memset(pkts, 0, cnt * sizeof *pkts); } /* Tx function. Transmit packets indefinitely */ @@ -1230,20 +1226,13 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct dp_packet **pkts, } static int -netdev_dpdk_vhost_send(struct netdev *netdev, int qid OVS_UNUSED, struct dp_packet **pkts, - int cnt, bool may_steal) +netdev_dpdk_vhost_send(struct netdev *netdev, int qid OVS_UNUSED, + struct dp_packet **pkts, int cnt) { if (OVS_UNLIKELY(pkts[0]->source != DPBUF_DPDK)) { - int i; - dpdk_do_tx_copy(netdev, qid, pkts, cnt); - if (may_steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(pkts[i]); - } - } } else { - __netdev_dpdk_vhost_send(netdev, pkts, cnt, may_steal); + __netdev_dpdk_vhost_send(netdev, pkts, cnt); } return 0; } @@ -1259,17 +1248,8 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid, rte_spinlock_lock(&dev->tx_q[qid].tx_lock); } - if (OVS_UNLIKELY(!may_steal || - pkts[0]->source != DPBUF_DPDK)) { - struct netdev *netdev = &dev->up; - - dpdk_do_tx_copy(netdev, qid, pkts, cnt); - - if (may_steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(pkts[i]); - } - } + if (OVS_UNLIKELY(!may_steal || pkts[0]->source != DPBUF_DPDK)) { + dpdk_do_tx_copy(&dev->up, qid, pkts, cnt); } else { int next_tx_idx = 0; int dropped = 0; @@ -1281,21 +1261,20 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid, if (next_tx_idx != i) { dpdk_queue_pkts(dev, qid, (struct rte_mbuf **)&pkts[next_tx_idx], - i-next_tx_idx); + i - next_tx_idx); } VLOG_WARN_RL(&rl, "Too big size %d max_packet_len %d", (int)size , dev->max_packet_len); - dp_packet_delete(pkts[i]); dropped++; next_tx_idx = i + 1; } } if (next_tx_idx != cnt) { - dpdk_queue_pkts(dev, qid, + dpdk_queue_pkts(dev, qid, (struct rte_mbuf **)&pkts[next_tx_idx], - cnt-next_tx_idx); + cnt - next_tx_idx); } if (OVS_UNLIKELY(dropped)) { diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c index 76815c2..528ae9e 100644 --- a/lib/netdev-dummy.c +++ b/lib/netdev-dummy.c @@ -896,7 +896,7 @@ netdev_dummy_rxq_drain(struct netdev_rxq *rxq_) static int netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, - struct dp_packet **pkts, int cnt, bool may_steal) + struct dp_packet **pkts, int cnt, bool may_steal OVS_UNUSED) { struct netdev_dummy *dev = netdev_dummy_cast(netdev); int error = 0; @@ -960,12 +960,6 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED, ovs_mutex_unlock(&dev->mutex); } - if (may_steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(pkts[i]); - } - } - return error; } diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index e047be5..a308815 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -1120,7 +1120,7 @@ netdev_linux_rxq_drain(struct netdev_rxq *rxq_) * expected to do additional queuing of packets. */ static int netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED, - struct dp_packet **pkts, int cnt, bool may_steal) + struct dp_packet **pkts, int cnt, bool may_steal OVS_UNUSED) { int i; int error = 0; @@ -1200,19 +1200,12 @@ netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED, i++; } - if (may_steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(pkts[i]); - } - } - if (error && error != EAGAIN) { VLOG_WARN_RL(&rl, "error sending Ethernet packet on %s: %s", netdev_get_name(netdev_), ovs_strerror(error)); } return error; - } /* Registers with the poll loop to wake up from the next call to poll_block() diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h index a33bb3b..5428688 100644 --- a/lib/netdev-provider.h +++ b/lib/netdev-provider.h @@ -306,6 +306,10 @@ struct netdev_class { * been sent anyway. * * To retain ownership of 'buffers' caller can set may_steal to false. + * When 'may_steal' is set to 'true', the network device will set the + * pointer of each packet in 'buffers' whose ownership was taken ('stolen') + * to NULL. The caller retains ownership of all packets whose pointer is + * non-NULL on return. * * The network device is expected to maintain one or more packet * transmission queues, so that the caller does not ordinarily have to diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 0d56354..62ced0c 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -26,11 +26,11 @@ #include "dp-packet.h" #include "dpif.h" +#include "flow.h" #include "netlink.h" #include "odp-netlink.h" #include "odp-util.h" #include "packets.h" -#include "flow.h" #include "unaligned.h" #include "util.h" @@ -444,9 +444,14 @@ odp_execute_masked_set_action(struct dp_packet *packet, } static void -odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, +odp_execute_actions__(void *dp, struct dp_packet **packets, int cnt, + const struct nlattr *actions, size_t actions_len, + odp_execute_cb dp_execute_action, bool may_steal); + +static void +odp_execute_sample(void *dp, struct dp_packet **packet_p, const struct nlattr *action, - odp_execute_cb dp_execute_action) + odp_execute_cb dp_execute_action, bool may_steal) { const struct nlattr *subactions = NULL; const struct nlattr *a; @@ -458,8 +463,8 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, switch ((enum ovs_sample_attr) type) { case OVS_SAMPLE_ATTR_PROBABILITY: if (random_uint32() >= nl_attr_get_u32(a)) { - if (steal) { - dp_packet_delete(packet); + if (may_steal) { + dp_packet_delete(*packet_p); } return; } @@ -476,8 +481,9 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, } } - odp_execute_actions(dp, &packet, 1, steal, nl_attr_get(subactions), - nl_attr_get_size(subactions), dp_execute_action); + odp_execute_actions__(dp, packet_p, 1, nl_attr_get(subactions), + nl_attr_get_size(subactions), dp_execute_action, + may_steal); } static bool @@ -514,10 +520,10 @@ requires_datapath_assistance(const struct nlattr *a) return false; } -void -odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, - const struct nlattr *actions, size_t actions_len, - odp_execute_cb dp_execute_action) +static void +odp_execute_actions__(void *dp, struct dp_packet **packets, int cnt, + const struct nlattr *actions, size_t actions_len, + odp_execute_cb dp_execute_action, bool may_steal) { const struct nlattr *a; unsigned int left; @@ -530,16 +536,13 @@ odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, if (requires_datapath_assistance(a)) { if (dp_execute_action) { /* Allow 'dp_execute_action' to steal the packet data if we do - * not need it any more. */ - bool may_steal = steal && last_action; - - dp_execute_action(dp, packets, cnt, a, may_steal); - - if (last_action) { - /* We do not need to free the packets. dp_execute_actions() - * has stolen them */ - return; - } + * not need it any more. As a precaution, packets actually + * stolen have their pointers set to NULL, so that we can + * catch bugs where the stolen packet is referenced afterwards. + * 'dp_execute_action' may also effectively drop packets by + * returning a count smaller than 'cnt' parameter. */ + cnt = dp_execute_action(dp, packets, cnt, a, + may_steal && last_action); } continue; } @@ -613,14 +616,8 @@ odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, case OVS_ACTION_ATTR_SAMPLE: for (i = 0; i < cnt; i++) { - odp_execute_sample(dp, packets[i], steal && last_action, a, - dp_execute_action); - } - - if (last_action) { - /* We do not need to free the packets. odp_execute_sample() has - * stolen them*/ - return; + odp_execute_sample(dp, &packets[i], a, dp_execute_action, + may_steal && last_action); } break; @@ -639,10 +636,21 @@ odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, OVS_NOT_REACHED(); } } +} +void +odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, + const struct nlattr *actions, size_t actions_len, + odp_execute_cb dp_execute_action) +{ + odp_execute_actions__(dp, packets, cnt, actions, actions_len, + dp_execute_action, steal); if (steal) { - for (i = 0; i < cnt; i++) { - dp_packet_delete(packets[i]); + for (int i = 0; i < cnt; i++) { + /* Packets may already have been taken, e.g. by output actions. */ + if (packets[i]) { + dp_packet_delete(packets[i]); + } } } } diff --git a/lib/odp-execute.h b/lib/odp-execute.h index c602bb4..c9cb43f 100644 --- a/lib/odp-execute.h +++ b/lib/odp-execute.h @@ -27,13 +27,15 @@ struct nlattr; struct dp_packet; struct pkt_metadata; -typedef void (*odp_execute_cb)(void *dp, struct dp_packet **packets, int cnt, - const struct nlattr *action, bool may_steal); +/* Remaining packets are dropped if the callback returns a value < 'cnt'. */ +typedef int (*odp_execute_cb)(void *dp, struct dp_packet **packets, int cnt, + const struct nlattr *action, bool may_steal); /* Actions that need to be executed in the context of a datapath are handed - * to 'dp_execute_action', if non-NULL. Currently this is called only for - * actions OVS_ACTION_ATTR_OUTPUT and OVS_ACTION_ATTR_USERSPACE so - * 'dp_execute_action' needs to handle only these. */ + * to 'dp_execute_action', if non-NULL. + * The caller relinquishes ownership of the 'packets' if 'steal' is 'true'. + * In this case the packet pointers in 'packets' will be set to NULL to + * enforce the ownership transfer. */ void odp_execute_actions(void *dp, struct dp_packet **packets, int cnt, bool steal, const struct nlattr *actions, size_t actions_len, diff --git a/lib/packets.c b/lib/packets.c index 701a5ec..2481589 100644 --- a/lib/packets.c +++ b/lib/packets.c @@ -350,20 +350,16 @@ pop_mpls(struct dp_packet *packet, ovs_be16 ethtype) const char * eth_from_hex(const char *hex, struct dp_packet **packetp) { - struct dp_packet *packet; - /* Use 2 bytes of headroom to 32-bit align the L3 header. */ - packet = *packetp = dp_packet_new_with_headroom(strlen(hex) / 2, 2); + *packetp = dp_packet_new_with_headroom(strlen(hex) / 2, 2); - if (dp_packet_put_hex(packet, hex, NULL)[0] != '\0') { - dp_packet_delete(packet); - *packetp = NULL; + if (dp_packet_put_hex(*packetp, hex, NULL)[0] != '\0') { + dp_packet_delete(*packetp); return "Trailing garbage in packet data"; } - if (dp_packet_size(packet) < ETH_HEADER_LEN) { - dp_packet_delete(packet); - *packetp = NULL; + if (dp_packet_size(*packetp) < ETH_HEADER_LEN) { + dp_packet_delete(*packetp); return "Packet data too short for Ethernet"; }