I'm not sure that the change to dpif_assert_class() is necessary.
There's a redundant newline added after dpif_flow_del().
Out of curiosity, where did EXPIRE_MAX_BATCH = 50 come from? Seems
like as good a number as any, just curious if the value has a
significant impact on performance.
In expire_subfacets(), "struct subfacet *batch[50]" should probably be
defined in terms of EXPIRE_MAX_BATCH.
Everything else looks good, thanks.
Ethan
On Mon, Apr 16, 2012 at 17:18, Ben Pfaff <b...@nicira.com> wrote:
> From: Ben Pfaff <b...@hardrock.nicira.com>
>
> Signed-off-by: Ben Pfaff <b...@hardrock.nicira.com>
> ---
> lib/dpif-linux.c | 103
> +++++++++++++++++++++++++++++++++---------------
> lib/dpif-netdev.c | 10 ++---
> lib/dpif-provider.h | 19 ++++-----
> lib/dpif.c | 53 +++++++++++++++++++------
> lib/dpif.h | 13 ++++++-
> ofproto/ofproto-dpif.c | 50 +++++++++++++++++++++++-
> 6 files changed, 186 insertions(+), 62 deletions(-)
>
> diff --git a/lib/dpif-linux.c b/lib/dpif-linux.c
> index 7013bc5..d47ddf1 100644
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira Networks.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -645,26 +645,32 @@ dpif_linux_flow_put(struct dpif *dpif_, const struct
> dpif_flow_put *put)
> return error;
> }
>
> -static int
> -dpif_linux_flow_del(struct dpif *dpif_,
> - const struct nlattr *key, size_t key_len,
> - struct dpif_flow_stats *stats)
> +static void
> +dpif_linux_init_flow_del(struct dpif *dpif_, const struct dpif_flow_del *del,
> + struct dpif_linux_flow *request)
> {
> struct dpif_linux *dpif = dpif_linux_cast(dpif_);
> +
> + dpif_linux_flow_init(request);
> + request->cmd = OVS_FLOW_CMD_DEL;
> + request->dp_ifindex = dpif->dp_ifindex;
> + request->key = del->key;
> + request->key_len = del->key_len;
> +}
> +
> +static int
> +dpif_linux_flow_del(struct dpif *dpif_, const struct dpif_flow_del *del)
> +{
> struct dpif_linux_flow request, reply;
> struct ofpbuf *buf;
> int error;
>
> - dpif_linux_flow_init(&request);
> - request.cmd = OVS_FLOW_CMD_DEL;
> - request.dp_ifindex = dpif->dp_ifindex;
> - request.key = key;
> - request.key_len = key_len;
> + dpif_linux_init_flow_del(dpif_, del, &request);
> error = dpif_linux_flow_transact(&request,
> - stats ? &reply : NULL,
> - stats ? &buf : NULL);
> - if (!error && stats) {
> - dpif_linux_flow_get_stats(&reply, stats);
> + del->stats ? &reply : NULL,
> + del->stats ? &buf : NULL);
> + if (!error && del->stats) {
> + dpif_linux_flow_get_stats(&reply, del->stats);
> ofpbuf_delete(buf);
> }
> return error;
> @@ -818,23 +824,39 @@ dpif_linux_operate(struct dpif *dpif_, struct dpif_op
> **ops, size_t n_ops)
> for (i = 0; i < n_ops; i++) {
> struct nl_transaction *txn = &txns[i];
> struct dpif_op *op = ops[i];
> -
> - if (op->type == DPIF_OP_FLOW_PUT) {
> - struct dpif_flow_put *put = &op->u.flow_put;
> - struct dpif_linux_flow request;
> -
> + struct dpif_flow_put *put;
> + struct dpif_flow_del *del;
> + struct dpif_execute *execute;
> + struct dpif_linux_flow request;
> +
> + switch (op->type) {
> + case DPIF_OP_FLOW_PUT:
> + put = &op->u.flow_put;
> dpif_linux_init_flow_put(dpif_, put, &request);
> if (put->stats) {
> request.nlmsg_flags |= NLM_F_ECHO;
> }
> txn->request = ofpbuf_new(1024);
> dpif_linux_flow_to_ofpbuf(&request, txn->request);
> - } else if (op->type == DPIF_OP_EXECUTE) {
> - struct dpif_execute *execute = &op->u.execute;
> + break;
> +
> + case DPIF_OP_FLOW_DEL:
> + del = &op->u.flow_del;
> + dpif_linux_init_flow_del(dpif_, del, &request);
> + if (del->stats) {
> + request.nlmsg_flags |= NLM_F_ECHO;
> + }
> + txn->request = ofpbuf_new(1024);
> + dpif_linux_flow_to_ofpbuf(&request, txn->request);
> + break;
>
> + case DPIF_OP_EXECUTE:
> + execute = &op->u.execute;
> txn->request = dpif_linux_encode_execute(dpif->dp_ifindex,
> execute);
> - } else {
> + break;
> +
> + default:
> NOT_REACHED();
> }
> }
> @@ -851,23 +873,40 @@ dpif_linux_operate(struct dpif *dpif_, struct dpif_op
> **ops, size_t n_ops)
> for (i = 0; i < n_ops; i++) {
> struct nl_transaction *txn = &txns[i];
> struct dpif_op *op = ops[i];
> + struct dpif_flow_put *put;
> + struct dpif_flow_del *del;
>
> - if (op->type == DPIF_OP_FLOW_PUT) {
> - struct dpif_flow_put *put = &op->u.flow_put;
> - int error = txn->error;
> + op->error = txn->error;
>
> - if (!error && put->stats) {
> + switch (op->type) {
> + case DPIF_OP_FLOW_PUT:
> + put = &op->u.flow_put;
> + if (!op->error && put->stats) {
> struct dpif_linux_flow reply;
>
> - error = dpif_linux_flow_from_ofpbuf(&reply, txn->reply);
> - if (!error) {
> + op->error = dpif_linux_flow_from_ofpbuf(&reply, txn->reply);
> + if (!op->error) {
> dpif_linux_flow_get_stats(&reply, put->stats);
> }
> }
> - op->error = error;
> - } else if (op->type == DPIF_OP_EXECUTE) {
> - op->error = txn->error;
> - } else {
> + break;
> +
> + case DPIF_OP_FLOW_DEL:
> + del = &op->u.flow_del;
> + if (!op->error && del->stats) {
> + struct dpif_linux_flow reply;
> +
> + op->error = dpif_linux_flow_from_ofpbuf(&reply, txn->reply);
> + if (!op->error) {
> + dpif_linux_flow_get_stats(&reply, del->stats);
> + }
> + }
> + break;
> +
> + case DPIF_OP_EXECUTE:
> + break;
> +
> + default:
> NOT_REACHED();
> }
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 1686087..a907722 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -777,24 +777,22 @@ dpif_netdev_flow_put(struct dpif *dpif, const struct
> dpif_flow_put *put)
> }
>
> static int
> -dpif_netdev_flow_del(struct dpif *dpif,
> - const struct nlattr *nl_key, size_t nl_key_len,
> - struct dpif_flow_stats *stats)
> +dpif_netdev_flow_del(struct dpif *dpif, const struct dpif_flow_del *del)
> {
> struct dp_netdev *dp = get_dp_netdev(dpif);
> struct dp_netdev_flow *flow;
> struct flow key;
> int error;
>
> - error = dpif_netdev_flow_from_nlattrs(nl_key, nl_key_len, &key);
> + error = dpif_netdev_flow_from_nlattrs(del->key, del->key_len, &key);
> if (error) {
> return error;
> }
>
> flow = dp_netdev_lookup_flow(dp, &key);
> if (flow) {
> - if (stats) {
> - get_dpif_flow_stats(flow, stats);
> + if (del->stats) {
> + get_dpif_flow_stats(flow, del->stats);
> }
> dp_netdev_free_flow(dp, flow);
> return 0;
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 5ef4c99..d694975 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2009, 2010, 2011 Nicira Networks.
> + * Copyright (c) 2009, 2010, 2011, 2012 Nicira Networks.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -46,8 +46,9 @@ void dpif_init(struct dpif *, const struct dpif_class *,
> const char *name,
> uint8_t netflow_engine_type, uint8_t netflow_engine_id);
> void dpif_uninit(struct dpif *dpif, bool close);
>
> -static inline void dpif_assert_class(const struct dpif *dpif,
> - const struct dpif_class *dpif_class)
> +static inline void dpif_assert_class(
> + const struct dpif *dpif OVS_UNUSED,
> + const struct dpif_class *dpif_class OVS_UNUSED)
> {
> assert(dpif->dpif_class == dpif_class);
> }
> @@ -235,14 +236,12 @@ struct dpif_class {
>
> /* Deletes a flow from 'dpif' and returns 0, or returns ENOENT if 'dpif'
> * does not contain such a flow. The flow is specified by the Netlink
> - * attributes with types OVS_KEY_ATTR_* in the 'key_len' bytes starting
> at
> - * 'key'.
> + * attributes with types OVS_KEY_ATTR_* in the 'del->key_len' bytes
> + * starting at 'del->key'.
> *
> - * If the operation succeeds, then 'stats', if nonnull, must be set to
> the
> - * flow's statistics before its deletion. */
> - int (*flow_del)(struct dpif *dpif,
> - const struct nlattr *key, size_t key_len,
> - struct dpif_flow_stats *stats);
> + * If the operation succeeds, then 'del->stats', if nonnull, must be set
> to
> + * the flow's statistics before its deletion. */
> + int (*flow_del)(struct dpif *dpif, const struct dpif_flow_del *del);
>
> /* Deletes all flows from 'dpif' and clears all of its queues of received
> * packets. */
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 73696e4..0199b49 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -89,6 +89,8 @@ static void log_operation(const struct dpif *, const char
> *operation,
> static bool should_log_flow_message(int error);
> static void log_flow_put_message(struct dpif *, const struct dpif_flow_put *,
> int error);
> +static void log_flow_del_message(struct dpif *, const struct dpif_flow_del *,
> + int error);
> static void log_execute_message(struct dpif *, const struct dpif_execute *,
> int error);
>
> @@ -815,6 +817,21 @@ dpif_flow_put(struct dpif *dpif, enum
> dpif_flow_put_flags flags,
> return dpif_flow_put__(dpif, &put);
> }
>
> +static int
> +dpif_flow_del__(struct dpif *dpif, struct dpif_flow_del *del)
> +{
> + int error;
> +
> + COVERAGE_INC(dpif_flow_del);
> +
> + error = dpif->dpif_class->flow_del(dpif, del);
> + if (error && del->stats) {
> + memset(del->stats, 0, sizeof *del->stats);
> + }
> + log_flow_del_message(dpif, del, error);
> + return error;
> +}
> +
> /* Deletes a flow from 'dpif' and returns 0, or returns ENOENT if 'dpif' does
> * not contain such a flow. The flow is specified by the Netlink attributes
> * with types OVS_KEY_ATTR_* in the 'key_len' bytes starting at 'key'.
> @@ -826,21 +843,15 @@ dpif_flow_del(struct dpif *dpif,
> const struct nlattr *key, size_t key_len,
> struct dpif_flow_stats *stats)
> {
> - int error;
> + struct dpif_flow_del del;
>
> - COVERAGE_INC(dpif_flow_del);
> -
> - error = dpif->dpif_class->flow_del(dpif, key, key_len, stats);
> - if (error && stats) {
> - memset(stats, 0, sizeof *stats);
> - }
> - if (should_log_flow_message(error)) {
> - log_flow_message(dpif, error, "flow_del", key, key_len,
> - !error ? stats : NULL, NULL, 0);
> - }
> - return error;
> + del.key = key;
> + del.key_len = key_len;
> + del.stats = stats;
> + return dpif_flow_del__(dpif, &del);
> }
>
> +
> /* Initializes 'dump' to begin dumping the flows in a dpif.
> *
> * This function provides no status indication. An error status for the
> entire
> @@ -994,6 +1005,10 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
> size_t n_ops)
> log_flow_put_message(dpif, &op->u.flow_put, op->error);
> break;
>
> + case DPIF_OP_FLOW_DEL:
> + log_flow_del_message(dpif, &op->u.flow_del, op->error);
> + break;
> +
> case DPIF_OP_EXECUTE:
> log_execute_message(dpif, &op->u.execute, op->error);
> break;
> @@ -1010,6 +1025,10 @@ dpif_operate(struct dpif *dpif, struct dpif_op **ops,
> size_t n_ops)
> op->error = dpif_flow_put__(dpif, &op->u.flow_put);
> break;
>
> + case DPIF_OP_FLOW_DEL:
> + op->error = dpif_flow_del__(dpif, &op->u.flow_del);
> + break;
> +
> case DPIF_OP_EXECUTE:
> op->error = dpif_execute__(dpif, &op->u.execute);
> break;
> @@ -1246,6 +1265,16 @@ log_flow_put_message(struct dpif *dpif, const struct
> dpif_flow_put *put,
> }
>
> static void
> +log_flow_del_message(struct dpif *dpif, const struct dpif_flow_del *del,
> + int error)
> +{
> + if (should_log_flow_message(error)) {
> + log_flow_message(dpif, error, "flow_del", del->key, del->key_len,
> + !error ? del->stats : NULL, NULL, 0);
> + }
> +}
> +
> +static void
> log_execute_message(struct dpif *dpif, const struct dpif_execute *execute,
> int error)
> {
> diff --git a/lib/dpif.h b/lib/dpif.h
> index 1a6ca05..768934b 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -177,7 +177,8 @@ int dpif_execute(struct dpif *,
>
> enum dpif_op_type {
> DPIF_OP_FLOW_PUT = 1,
> - DPIF_OP_EXECUTE
> + DPIF_OP_FLOW_DEL,
> + DPIF_OP_EXECUTE,
> };
>
> struct dpif_flow_put {
> @@ -192,6 +193,15 @@ struct dpif_flow_put {
> struct dpif_flow_stats *stats; /* Optional flow statistics. */
> };
>
> +struct dpif_flow_del {
> + /* Input. */
> + const struct nlattr *key; /* Flow to delete. */
> + size_t key_len; /* Length of 'key' in bytes. */
> +
> + /* Output. */
> + struct dpif_flow_stats *stats; /* Optional flow statistics. */
> +};
> +
> struct dpif_execute {
> const struct nlattr *key; /* Partial flow key (only for metadata).
> */
> size_t key_len; /* Length of 'key' in bytes. */
> @@ -205,6 +215,7 @@ struct dpif_op {
> int error;
> union {
> struct dpif_flow_put flow_put;
> + struct dpif_flow_del flow_del;
> struct dpif_execute execute;
> } u;
> };
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 265d6e9..594a705 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -2815,6 +2815,9 @@ handle_miss_upcalls(struct ofproto_dpif *ofproto,
> struct dpif_upcall *upcalls,
> op->subfacet->installed = true;
> }
> break;
> +
> + case DPIF_OP_FLOW_DEL:
> + NOT_REACHED();
> }
> }
> HMAP_FOR_EACH_SAFE (miss, next_miss, hmap_node, &todo) {
> @@ -3104,18 +3107,63 @@ subfacet_max_idle(const struct ofproto_dpif *ofproto)
> return bucket * BUCKET_WIDTH;
> }
>
> +enum { EXPIRE_MAX_BATCH = 50 };
> +
> +static void
> +expire_batch(struct ofproto_dpif *ofproto, struct subfacet **subfacets, int
> n)
> +{
> + struct odputil_keybuf keybufs[EXPIRE_MAX_BATCH];
> + struct dpif_op ops[EXPIRE_MAX_BATCH];
> + struct dpif_op *opsp[EXPIRE_MAX_BATCH];
> + struct ofpbuf keys[EXPIRE_MAX_BATCH];
> + struct dpif_flow_stats stats[EXPIRE_MAX_BATCH];
> + int i;
> +
> + for (i = 0; i < n; i++) {
> + ops[i].type = DPIF_OP_FLOW_DEL;
> + subfacet_get_key(subfacets[i], &keybufs[i], &keys[i]);
> + ops[i].u.flow_del.key = keys[i].data;
> + ops[i].u.flow_del.key_len = keys[i].size;
> + ops[i].u.flow_del.stats = &stats[i];
> + opsp[i] = &ops[i];
> + }
> +
> + dpif_operate(ofproto->dpif, opsp, n);
> + for (i = 0; i < n; i++) {
> + subfacet_reset_dp_stats(subfacets[i], &stats[i]);
> + subfacets[i]->installed = false;
> + subfacet_destroy(subfacets[i]);
> + }
> +}
> +
> static void
> expire_subfacets(struct ofproto_dpif *ofproto, int dp_max_idle)
> {
> long long int cutoff = time_msec() - dp_max_idle;
> +
> struct subfacet *subfacet, *next_subfacet;
> + struct subfacet *batch[50];
> + int n_batch;
>
> + n_batch = 0;
> HMAP_FOR_EACH_SAFE (subfacet, next_subfacet, hmap_node,
> &ofproto->subfacets) {
> if (subfacet->used < cutoff) {
> - subfacet_destroy(subfacet);
> + if (subfacet->installed) {
> + batch[n_batch++] = subfacet;
> + if (n_batch >= EXPIRE_MAX_BATCH) {
> + expire_batch(ofproto, batch, n_batch);
> + n_batch = 0;
> + }
> + } else {
> + subfacet_destroy(subfacet);
> + }
> }
> }
> +
> + if (n_batch > 0) {
> + expire_batch(ofproto, batch, n_batch);
> + }
> }
>
> /* If 'rule' is an OpenFlow rule, that has expired according to OpenFlow
> rules,
> --
> 1.7.9
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
