This needs a rebase due to the recent lib/ofp-actions.c changes. I’ll post a v2
in a moment.
Jarno
On Aug 5, 2014, at 4:38 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> 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 <jrajaha...@nicira.com>
> ---
> lib/dpif-netdev.c | 31 ++++++++++-----------
> lib/dpif.c | 44 +++++++++++++++++++++++------
> lib/netdev-bsd.c | 5 ----
> lib/netdev-dpdk.c | 14 ++++------
> lib/netdev-dummy.c | 8 +-----
> lib/netdev-linux.c | 8 +-----
> lib/netdev-provider.h | 4 +++
> lib/odp-execute.c | 63 +++++++++++++++++++-----------------------
> lib/odp-execute.h | 14 ++++++----
> lib/ofp-actions.c | 1 +
> lib/ofp-actions.h | 1 +
> ofproto/ofproto-dpif-xlate.c | 11 +++++++-
> ofproto/ofproto-dpif.c | 5 ++--
> ofproto/ofproto-provider.h | 3 +-
> ofproto/ofproto.c | 47 +++++++++++++++++--------------
> 15 files changed, 140 insertions(+), 119 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c662260..1e0f05f 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1924,6 +1924,7 @@ dp_netdev_input(struct dp_netdev *dp, struct
> dpif_packet **packets, int cnt,
> for (i = 0; i < cnt; i++) {
> if (OVS_UNLIKELY(ofpbuf_size(&packets[i]->ofpbuf) < ETH_HEADER_LEN)) {
> dpif_packet_delete(packets[i]);
> + packets[i] = NULL;
> mfs[i] = NULL;
> continue;
> }
> @@ -1952,6 +1953,7 @@ dp_netdev_input(struct dp_netdev *dp, struct
> dpif_packet **packets, int cnt,
> dp_netdev_queue_userspace_packet(&q, buf, DPIF_UC_MISS,
> mfs[i], NULL);
> dpif_packet_delete(packets[i]);
> + packets[i] = NULL;
> continue;
> }
>
> @@ -2080,7 +2082,7 @@ dpif_netdev_register_upcall_cb(struct dpif *dpif,
> exec_upcall_cb *cb)
> dp->upcall_cb = cb;
> }
>
> -static void
> +static int
> dp_execute_cb(void *aux_, struct dpif_packet **packets, int cnt,
> struct pkt_metadata *md,
> const struct nlattr *a, bool may_steal)
> @@ -2097,10 +2099,6 @@ dp_execute_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> p = dp_netdev_lookup_port(aux->dp, u32_to_odp(nl_attr_get_u32(a)));
> if (OVS_LIKELY(p)) {
> netdev_send(p->netdev, packets, cnt, may_steal);
> - } else if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(packets[i]);
> - }
> }
> break;
>
> @@ -2123,9 +2121,6 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets,
> int cnt,
> dp_netdev_queue_userspace_packet(&q, packet,
> DPIF_UC_ACTION, &key.flow,
> userdata);
> - if (may_steal) {
> - dpif_packet_delete(packets[i]);
> - }
> }
>
> if (q.packet_count) {
> @@ -2178,13 +2173,16 @@ dp_execute_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> struct dpif_packet *recirc_pkt;
> struct pkt_metadata recirc_md = *md;
>
> - recirc_pkt = (may_steal) ? packets[i]
> - : dpif_packet_clone(packets[i]);
> -
> + if (may_steal) {
> + recirc_pkt = packets[i];
> + packets[i] = NULL; /* Mark as taken. */
> + } else {
> + recirc_pkt = dpif_packet_clone(packets[i]);
> + }
> recirc_md.recirc_id = nl_attr_get_u32(a);
>
> /* Hash is private to each packet */
> - recirc_md.dp_hash = packets[i]->dp_hash;
> + recirc_md.dp_hash = recirc_pkt->dp_hash;
>
> dp_netdev_input(aux->dp, &recirc_pkt, 1, &recirc_md);
> }
> @@ -2193,15 +2191,12 @@ dp_execute_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> break;
> } else {
> VLOG_WARN("Packet dropped. Max recirculation depth exceeded.");
> - if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(packets[i]);
> - }
> - }
> }
> 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:
> @@ -2212,6 +2207,8 @@ dp_execute_cb(void *aux_, struct dpif_packet **packets,
> int cnt,
> case __OVS_ACTION_ATTR_MAX:
> OVS_NOT_REACHED();
> }
> +
> + return cnt;
> }
>
> static void
> diff --git a/lib/dpif.c b/lib/dpif.c
> index ce941e0..d0476c2 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -1068,11 +1068,12 @@ dpif_flow_dump_next(struct dpif_flow_dump_thread
> *thread,
> struct dpif_execute_helper_aux {
> struct dpif *dpif;
> int error;
> + const struct nlattr *meter_action; /* Non-NULL, if have a meter action.
> */
> };
>
> /* 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 dpif_packet **packets, int cnt,
> struct pkt_metadata *md,
> const struct nlattr *action, bool may_steal OVS_UNUSED)
> @@ -1084,6 +1085,13 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> ovs_assert(cnt == 1);
>
> switch ((enum ovs_action_attr)type) {
> + case OVS_ACTION_ATTR_METER:
> + /* Maintain a pointer to the first meter action seen. */
> + if (!aux->meter_action) {
> + aux->meter_action = action;
> + }
> + return cnt;
> +
> case OVS_ACTION_ATTR_OUTPUT:
> case OVS_ACTION_ATTR_USERSPACE:
> case OVS_ACTION_ATTR_RECIRC: {
> @@ -1091,12 +1099,28 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> struct ofpbuf execute_actions;
> uint64_t stub[256 / 8];
>
> - if (md->tunnel.ip_dst) {
> + if (md->tunnel.ip_dst || aux->meter_action) {
> + ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
> +
> + if (aux->meter_action) {
> + const struct nlattr *a = aux->meter_action;
> +
> + do {
> + ofpbuf_put(&execute_actions, a, NLA_ALIGN(a->nla_len));
> + /* Find next meter action before 'action', if any. */
> + do {
> + a = nl_attr_next(a);
> + } while (a != action &&
> + nl_attr_type(a) != OVS_ACTION_ATTR_METER);
> + } while (a != action);
> + }
> +
> /* The Linux kernel datapath throws away the tunnel information
> * that we supply as metadata. We have to use a "set" action to
> * supply it. */
> - ofpbuf_use_stub(&execute_actions, stub, sizeof stub);
> - odp_put_tunnel_action(&md->tunnel, &execute_actions);
> + if (md->tunnel.ip_dst) {
> + odp_put_tunnel_action(&md->tunnel, &execute_actions);
> + }
> ofpbuf_put(&execute_actions, action, NLA_ALIGN(action->nla_len));
>
> execute.actions = ofpbuf_data(&execute_actions);
> @@ -1113,14 +1137,17 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
>
> log_execute_message(aux->dpif, &execute, true, aux->error);
>
> - if (md->tunnel.ip_dst) {
> + if (md->tunnel.ip_dst || aux->meter_action) {
> ofpbuf_uninit(&execute_actions);
> +
> + /* Do not re-use the same meters for later output actions. */
> + aux->meter_action = NULL;
> }
> - break;
> +
> + return (aux->error == 0) ? cnt : 0;
> }
>
> case OVS_ACTION_ATTR_HASH:
> - case OVS_ACTION_ATTR_METER:
> case OVS_ACTION_ATTR_PUSH_VLAN:
> case OVS_ACTION_ATTR_POP_VLAN:
> case OVS_ACTION_ATTR_PUSH_MPLS:
> @@ -1131,6 +1158,7 @@ dpif_execute_helper_cb(void *aux_, struct dpif_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
> @@ -1141,7 +1169,7 @@ dpif_execute_helper_cb(void *aux_, struct dpif_packet
> **packets, int cnt,
> static int
> dpif_execute_with_help(struct dpif *dpif, struct dpif_execute *execute)
> {
> - struct dpif_execute_helper_aux aux = {dpif, 0};
> + struct dpif_execute_helper_aux aux = {dpif, 0, NULL};
> struct dpif_packet packet, *pp;
>
> COVERAGE_INC(dpif_execute_with_help);
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 16efc3d..35fb6a6 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -734,11 +734,6 @@ netdev_bsd_send(struct netdev *netdev_, struct
> dpif_packet **pkts, int cnt,
> }
>
> ovs_mutex_unlock(&dev->mutex);
> - if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(pkts[i]);
> - }
> - }
>
> return error;
> }
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 6ee9803..5433861 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -703,6 +703,9 @@ dpdk_queue_pkts(struct netdev_dpdk *dev, int qid,
> }
> }
> rte_spinlock_unlock(&txq->tx_lock);
> +
> + /* Clear the transferred pointers to mark them as 'taken'. */
> + memset(pkts, 0, cnt * sizeof *pkts);
> }
>
> /* Tx function. Transmit packets indefinitely */
> @@ -774,12 +777,6 @@ netdev_dpdk_send(struct netdev *netdev, struct
> dpif_packet **pkts, int cnt,
>
> if (!may_steal || pkts[0]->ofpbuf.source != OFPBUF_DPDK) {
> dpdk_do_tx_copy(netdev, pkts, cnt);
> -
> - if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(pkts[i]);
> - }
> - }
> } else {
> int qid;
> int next_tx_idx = 0;
> @@ -793,19 +790,18 @@ netdev_dpdk_send(struct netdev *netdev, struct
> dpif_packet **pkts, int cnt,
> 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);
>
> - dpif_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);
> }
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 8d1c298..acd25bc 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -847,7 +847,7 @@ netdev_dummy_rxq_drain(struct netdev_rxq *rxq_)
>
> static int
> netdev_dummy_send(struct netdev *netdev, struct dpif_packet **pkts, int cnt,
> - bool may_steal)
> + bool may_steal OVS_UNUSED)
> {
> struct netdev_dummy *dev = netdev_dummy_cast(netdev);
> int error = 0;
> @@ -894,12 +894,6 @@ netdev_dummy_send(struct netdev *netdev, struct
> dpif_packet **pkts, int cnt,
> ovs_mutex_unlock(&dev->mutex);
> }
>
> - if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(pkts[i]);
> - }
> - }
> -
> return error;
> }
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index e50392a..6a1eeee 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1058,7 +1058,7 @@ netdev_linux_rxq_drain(struct netdev_rxq *rxq_)
> * expected to do additional queuing of packets. */
> static int
> netdev_linux_send(struct netdev *netdev_, struct dpif_packet **pkts, int cnt,
> - bool may_steal)
> + bool may_steal OVS_UNUSED)
> {
> int i;
> int error = 0;
> @@ -1138,12 +1138,6 @@ netdev_linux_send(struct netdev *netdev_, struct
> dpif_packet **pkts, int cnt,
> i++;
> }
>
> - if (may_steal) {
> - for (i = 0; i < cnt; i++) {
> - dpif_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));
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index 56244ad..5f36374 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -260,6 +260,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 a packet transmission queue,
> * so that the caller does not ordinarily have to do additional queuing of
> diff --git a/lib/odp-execute.c b/lib/odp-execute.c
> index fd6644d..179f6f6 100644
> --- a/lib/odp-execute.c
> +++ b/lib/odp-execute.c
> @@ -21,13 +21,13 @@
> #include <string.h>
>
> #include "dpif.h"
> +#include "flow.h"
> #include "netlink.h"
> #include "ofpbuf.h"
> #include "odp-netlink.h"
> #include "odp-util.h"
> #include "packet-dpif.h"
> #include "packets.h"
> -#include "flow.h"
> #include "unaligned.h"
> #include "util.h"
>
> @@ -160,14 +160,14 @@ odp_execute_set_action(struct dpif_packet *packet,
> const struct nlattr *a,
>
> static void
> odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
> - bool steal, struct pkt_metadata *,
> + struct pkt_metadata *,
> const struct nlattr *actions, size_t actions_len,
> - odp_execute_cb dp_execute_action, bool more_actions);
> + odp_execute_cb dp_execute_action, bool may_steal);
>
> static void
> -odp_execute_sample(void *dp, struct dpif_packet *packet, bool steal,
> +odp_execute_sample(void *dp, struct dpif_packet **packet_p,
> struct pkt_metadata *md, const struct nlattr *action,
> - odp_execute_cb dp_execute_action, bool more_actions)
> + odp_execute_cb dp_execute_action, bool may_steal)
> {
> const struct nlattr *subactions = NULL;
> const struct nlattr *a;
> @@ -194,20 +194,19 @@ odp_execute_sample(void *dp, struct dpif_packet
> *packet, bool steal,
> }
> }
>
> - odp_execute_actions__(dp, &packet, 1, steal, md, nl_attr_get(subactions),
> + odp_execute_actions__(dp, packet_p, 1, md, nl_attr_get(subactions),
> nl_attr_get_size(subactions), dp_execute_action,
> - more_actions);
> + may_steal);
> }
>
> static void
> odp_execute_actions__(void *dp, struct dpif_packet **packets, int cnt,
> - bool steal, struct pkt_metadata *md,
> + struct pkt_metadata *md,
> const struct nlattr *actions, size_t actions_len,
> - odp_execute_cb dp_execute_action, bool more_actions)
> + odp_execute_cb dp_execute_action, bool may_steal)
> {
> const struct nlattr *a;
> unsigned int left;
> -
> int i;
>
> NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) {
> @@ -215,21 +214,20 @@ odp_execute_actions__(void *dp, struct dpif_packet
> **packets, int cnt,
>
> switch ((enum ovs_action_attr) type) {
>
> - case OVS_ACTION_ATTR_METER:
> - /* Not implemented yet. */
> - break;
> -
> /* These only make sense in the context of a datapath. */
> + case OVS_ACTION_ATTR_METER:
> case OVS_ACTION_ATTR_OUTPUT:
> case OVS_ACTION_ATTR_USERSPACE:
> case OVS_ACTION_ATTR_RECIRC:
> 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 && (!more_actions
> - && left <= NLA_ALIGN(a->nla_len)
> - && type !=
> OVS_ACTION_ATTR_RECIRC);
> - dp_execute_action(dp, packets, cnt, md, a, may_steal);
> + * 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, md, a, may_steal
> + && left <= NLA_ALIGN(a->nla_len));
> }
> break;
>
> @@ -305,17 +303,14 @@ odp_execute_actions__(void *dp, struct dpif_packet
> **packets, int cnt,
>
> case OVS_ACTION_ATTR_SET:
> for (i = 0; i < cnt; i++) {
> - odp_execute_set_action(packets[i], nl_attr_get(a),
> - md);
> + odp_execute_set_action(packets[i], nl_attr_get(a), md);
> }
> break;
>
> case OVS_ACTION_ATTR_SAMPLE:
> for (i = 0; i < cnt; i++) {
> - odp_execute_sample(dp, packets[i], steal, md, a,
> - dp_execute_action,
> - more_actions ||
> - left > NLA_ALIGN(a->nla_len));
> + odp_execute_sample(dp, &packets[i], md, a, dp_execute_action,
> + may_steal && left <=
> NLA_ALIGN(a->nla_len));
> }
> break;
>
> @@ -332,15 +327,15 @@ odp_execute_actions(void *dp, struct dpif_packet
> **packets, int cnt,
> const struct nlattr *actions, size_t actions_len,
> odp_execute_cb dp_execute_action)
> {
> - odp_execute_actions__(dp, packets, cnt, steal, md, actions, actions_len,
> - dp_execute_action, false);
> -
> - if (!actions_len && steal) {
> - /* Drop action. */
> - int i;
> -
> - for (i = 0; i < cnt; i++) {
> - dpif_packet_delete(packets[i]);
> + odp_execute_actions__(dp, packets, cnt, md, actions, actions_len,
> + dp_execute_action, steal);
> + if (steal) {
> + for (int i = 0; i < cnt; i++) {
> + /* Packets may already have been taken, e.g. by output actions.
> */
> + if (packets[i]) {
> + dpif_packet_delete(packets[i]);
> + packets[i] = NULL;
> + }
> }
> }
> }
> diff --git a/lib/odp-execute.h b/lib/odp-execute.h
> index 23dc219..9f4ca7d 100644
> --- a/lib/odp-execute.h
> +++ b/lib/odp-execute.h
> @@ -27,14 +27,16 @@ struct nlattr;
> struct dpif_packet;
> struct pkt_metadata;
>
> -typedef void (*odp_execute_cb)(void *dp, struct dpif_packet **packets, int
> cnt,
> - struct pkt_metadata *,
> - 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 dpif_packet **packets, int
> cnt,
> + struct pkt_metadata *,
> + 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 dpif_packet **packets, int cnt,
> bool steal, struct pkt_metadata *,
> const struct nlattr *actions, size_t actions_len,
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index e8fd922..258ae85 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -1785,6 +1785,7 @@ ofpacts_pull_openflow_instructions(struct ofpbuf
> *openflow,
>
> om = ofpact_put_METER(ofpacts);
> om->meter_id = ntohl(oim->meter_id);
> + om->provider_meter_id = UINT32_MAX; /* No provider meter ID. */
> }
> if (insts[OVSINST_OFPIT11_APPLY_ACTIONS]) {
> const union ofp_action *actions;
> diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
> index 8cb8862..040c7b9 100644
> --- a/lib/ofp-actions.h
> +++ b/lib/ofp-actions.h
> @@ -429,6 +429,7 @@ struct ofpact_metadata {
> struct ofpact_meter {
> struct ofpact ofpact;
> uint32_t meter_id;
> + uint32_t provider_meter_id;
> };
>
> /* OFPACT_WRITE_ACTIONS.
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 4aedb59..e958f9e 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3363,6 +3363,15 @@ xlate_sample_action(struct xlate_ctx *ctx,
> probability, &cookie, sizeof cookie.flow_sample);
> }
>
> +static void
> +xlate_meter_action(struct xlate_ctx *ctx, const struct ofpact_meter *meter)
> +{
> + if (meter->provider_meter_id != UINT32_MAX) {
> + nl_msg_put_u32(&ctx->xout->odp_actions, OVS_ACTION_ATTR_METER,
> + meter->provider_meter_id);
> + }
> +}
> +
> static bool
> may_receive(const struct xport *xport, struct xlate_ctx *ctx)
> {
> @@ -3756,7 +3765,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> break;
>
> case OFPACT_METER:
> - /* Not implemented yet. */
> + xlate_meter_action(ctx, ofpact_get_METER(a));
> break;
>
> case OFPACT_GOTO_TABLE: {
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index bc56d08..408c119 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3221,9 +3221,8 @@ rule_dpif_reduce_timeouts(struct rule_dpif *rule,
> uint16_t idle_timeout,
> ofproto_rule_reduce_timeouts(&rule->up, idle_timeout, hard_timeout);
> }
>
> -/* Returns 'rule''s actions. The caller owns a reference on the returned
> - * actions and must eventually release it (with rule_actions_unref()) to
> avoid
> - * a memory leak. */
> +/* Returns 'rule''s actions. The actions are managed via RCU and may be used
> + * until the calling thread quiesces. */
> const struct rule_actions *
> rule_dpif_get_actions(const struct rule_dpif *rule)
> {
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 8ce7b68..b0ca28f 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1646,8 +1646,7 @@ void ofproto_delete_flow(struct ofproto *,
> OVS_EXCLUDED(ofproto_mutex);
> void ofproto_flush_flows(struct ofproto *);
>
> -enum ofperr ofproto_check_ofpacts(struct ofproto *,
> - const struct ofpact ofpacts[],
> +enum ofperr ofproto_check_ofpacts(struct ofproto *, struct ofpact ofpacts[],
> size_t ofpacts_len);
>
> static inline const struct rule_actions *
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 0aaae31..4d09c9b 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2562,8 +2562,8 @@ ofproto_group_unref(struct ofgroup *group)
> }
> }
>
> -static uint32_t get_provider_meter_id(const struct ofproto *,
> - uint32_t of_meter_id);
> +static bool ofproto_fix_meter_action(const struct ofproto *,
> + struct ofpact_meter *);
>
> /* Creates and returns a new 'struct rule_actions', whose actions are a copy
> * of from the 'ofpacts_len' bytes of 'ofpacts'. */
> @@ -2905,23 +2905,23 @@ reject_slave_controller(struct ofconn *ofconn)
> * for 'ofproto':
> *
> * - If they use a meter, then 'ofproto' has that meter configured.
> + * Updates the meter action with ofproto's datapath's provider_meter_id.
> *
> * - If they use any groups, then 'ofproto' has that group configured.
> *
> * Returns 0 if successful, otherwise an OpenFlow error. */
> enum ofperr
> ofproto_check_ofpacts(struct ofproto *ofproto,
> - const struct ofpact ofpacts[], size_t ofpacts_len)
> + struct ofpact ofpacts[], size_t ofpacts_len)
> {
> - const struct ofpact *a;
> - uint32_t mid;
> -
> - mid = ofpacts_get_meter(ofpacts, ofpacts_len);
> - if (mid && get_provider_meter_id(ofproto, mid) == UINT32_MAX) {
> - return OFPERR_OFPMMFC_INVALID_METER;
> - }
> + struct ofpact *a;
>
> OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
> + if (a->type == OFPACT_METER &&
> + !ofproto_fix_meter_action(ofproto, ofpact_get_METER(a))) {
> + return OFPERR_OFPMMFC_INVALID_METER;
> + }
> +
> if (a->type == OFPACT_GROUP
> && !ofproto_group_exists(ofproto, ofpact_get_GROUP(a)->group_id))
> {
> return OFPERR_OFPBAC_BAD_OUT_GROUP;
> @@ -4964,20 +4964,27 @@ struct meter {
> };
>
> /*
> - * This is used in instruction validation at flow set-up time,
> - * as flows may not use non-existing meters.
> - * Return value of UINT32_MAX signifies an invalid meter.
> + * This is used in instruction validation at flow set-up time, to map
> + * the OpenFlow meter ID to the corresponding datapath provider meter
> + * ID. If either does not exist, returns false. Otherwise updates
> + * the meter action and returns true.
> */
> -static uint32_t
> -get_provider_meter_id(const struct ofproto *ofproto, uint32_t of_meter_id)
> +static bool
> +ofproto_fix_meter_action(const struct ofproto *ofproto,
> + struct ofpact_meter *ma)
> {
> - if (of_meter_id && of_meter_id <= ofproto->meter_features.max_meters) {
> - const struct meter *meter = ofproto->meters[of_meter_id];
> - if (meter) {
> - return meter->provider_meter_id.uint32;
> + if (ma->meter_id && ma->meter_id <= ofproto->meter_features.max_meters) {
> + const struct meter *meter = ofproto->meters[ma->meter_id];
> +
> + if (meter && meter->provider_meter_id.uint32 != UINT32_MAX) {
> + /* Update the action with the provider's meter ID, so that we
> + * do not need any synchronization between ofproto_dpif_xlate
> + * and ofproto for meter table access. */
> + ma->provider_meter_id = meter->provider_meter_id.uint32;
> + return true;
> }
> }
> - return UINT32_MAX;
> + return false;
> }
>
> /* Finds the meter invoked by 'rule''s actions and adds 'rule' to the meter's
> --
> 1.7.10.4
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
