[ovs-dev] [PATCH 5/6] lib/odp, ofproto xlate: Meter execution support.

Jarno Rajahalme jrajahalme at nicira.com
Mon Aug 11 21:35:17 UTC 2014


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 <jrajahalme at 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 <jrajahalme at 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
> 




More information about the dev mailing list