Traditionally we've put function comments just in the .c file, not in
the .c and .h file as you've done for rule_dpif_lookup(). That said,
I don't know where that convention came from and don't care that much
. . .
Acked-by: Ethan Jackson <et...@nicira.com>
On Wed, Apr 23, 2014 at 4:20 PM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Prior to this paths the rule lookup functions have always taken a
> reference on the found rule before returning. Make this conditional,
> so that unnecessary refs/unrefs can be avoided in a later patch.
>
> Signed-off-by: Jarno Rajahalme <jrajaha...@nicira.com>
> ---
> ofproto/ofproto-dpif-xlate.c | 8 +++---
> ofproto/ofproto-dpif.c | 58
> +++++++++++++++++++++++++++++-------------
> ofproto/ofproto-dpif.h | 20 ++++++++++++---
> 3 files changed, 62 insertions(+), 24 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 248382f..c62424a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2057,7 +2057,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t
> in_port, uint8_t table_id,
> !skip_wildcards
> ? &ctx->xout->wc : NULL,
> honor_table_miss,
> - &ctx->table_id, &rule);
> + &ctx->table_id, &rule, true);
> ctx->xin->flow.in_port.ofp_port = old_in_port;
>
> if (ctx->xin->resubmit_hook) {
> @@ -2090,7 +2090,7 @@ xlate_table_action(struct xlate_ctx *ctx, ofp_port_t
> in_port, uint8_t table_id,
> }
>
> choose_miss_rule(config, ctx->xbridge->miss_rule,
> - ctx->xbridge->no_packet_in_rule, &rule);
> + ctx->xbridge->no_packet_in_rule, &rule, true);
>
> match:
> if (rule) {
> @@ -2654,7 +2654,7 @@ xlate_learn_action(struct xlate_ctx *ctx,
> entry = xlate_cache_add_entry(ctx->xin->xcache, XC_LEARN);
> entry->u.learn.ofproto = ctx->xin->ofproto;
> rule_dpif_lookup(ctx->xbridge->ofproto, &ctx->xin->flow, NULL,
> - &entry->u.learn.rule);
> + &entry->u.learn.rule, true);
> }
> }
>
> @@ -3263,7 +3263,7 @@ xlate_actions__(struct xlate_in *xin, struct xlate_out
> *xout)
> if (!xin->ofpacts && !ctx.rule) {
> ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
> !xin->skip_wildcards ? wc : NULL,
> - &rule);
> + &rule, true);
> if (ctx.xin->resubmit_stats) {
> rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
> }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 983cad5..5669cd1 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3189,10 +3189,16 @@ rule_dpif_get_actions(const struct rule_dpif *rule)
> *
> * The return value will be zero unless there was a miss and
> * OFPTC11_TABLE_MISS_CONTINUE is in effect for the sequence of tables
> - * where misses occur. */
> + * where misses occur.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
> uint8_t
> rule_dpif_lookup(struct ofproto_dpif *ofproto, struct flow *flow,
> - struct flow_wildcards *wc, struct rule_dpif **rule)
> + struct flow_wildcards *wc, struct rule_dpif **rule,
> + bool take_ref)
> {
> enum rule_dpif_lookup_verdict verdict;
> enum ofputil_port_config config = 0;
> @@ -3219,7 +3225,7 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
> flow *flow,
> }
>
> verdict = rule_dpif_lookup_from_table(ofproto, flow, wc, true,
> - &table_id, rule);
> + &table_id, rule, take_ref);
>
> switch (verdict) {
> case RULE_DPIF_LOOKUP_VERDICT_MATCH:
> @@ -3248,13 +3254,17 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, struct
> flow *flow,
> }
>
> choose_miss_rule(config, ofproto->miss_rule,
> - ofproto->no_packet_in_rule, rule);
> + ofproto->no_packet_in_rule, rule, take_ref);
> return table_id;
> }
>
> +/* The returned rule is valid at least until the next RCU quiescent period.
> + * If the '*rule' needs to stay around longer, a non-zero 'take_ref' must be
> + * passed in to cause a reference to be taken on it before this returns. */
> static struct rule_dpif *
> rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto, uint8_t table_id,
> - const struct flow *flow, struct flow_wildcards *wc)
> + const struct flow *flow, struct flow_wildcards *wc,
> + bool take_ref)
> {
> struct classifier *cls = &ofproto->up.tables[table_id].cls;
> const struct cls_rule *cls_rule;
> @@ -3288,7 +3298,9 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
> uint8_t table_id,
> }
>
> rule = rule_dpif_cast(rule_from_cls_rule(cls_rule));
> - rule_dpif_ref(rule);
> + if (take_ref) {
> + rule_dpif_ref(rule);
> + }
> fat_rwlock_unlock(&cls->rwlock);
>
> return rule;
> @@ -3323,13 +3335,19 @@ rule_dpif_lookup_in_table(struct ofproto_dpif
> *ofproto, uint8_t table_id,
> *
> * - RULE_DPIF_LOOKUP_VERDICT_DEFAULT if no rule was found,
> * 'honor_table_miss' is true and a table miss configuration has
> - * not been specified in this case. */
> + * not been specified in this case.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
> enum rule_dpif_lookup_verdict
> rule_dpif_lookup_from_table(struct ofproto_dpif *ofproto,
> const struct flow *flow,
> struct flow_wildcards *wc,
> bool honor_table_miss,
> - uint8_t *table_id, struct rule_dpif **rule)
> + uint8_t *table_id, struct rule_dpif **rule,
> + bool take_ref)
> {
> uint8_t next_id;
>
> @@ -3338,7 +3356,8 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
> next_id++, next_id += (next_id == TBL_INTERNAL))
> {
> *table_id = next_id;
> - *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc);
> + *rule = rule_dpif_lookup_in_table(ofproto, *table_id, flow, wc,
> + take_ref);
> if (*rule) {
> return RULE_DPIF_LOOKUP_VERDICT_MATCH;
> } else if (!honor_table_miss) {
> @@ -3365,13 +3384,21 @@ rule_dpif_lookup_from_table(struct ofproto_dpif
> *ofproto,
>
> /* Given a port configuration (specified as zero if there's no port), chooses
> * which of 'miss_rule' and 'no_packet_in_rule' should be used in case of a
> - * flow table miss. */
> + * flow table miss.
> + *
> + * The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a reference must be taken on it (rule_dpif_ref()).
> + */
> void
> choose_miss_rule(enum ofputil_port_config config, struct rule_dpif
> *miss_rule,
> - struct rule_dpif *no_packet_in_rule, struct rule_dpif
> **rule)
> + struct rule_dpif *no_packet_in_rule, struct rule_dpif
> **rule,
> + bool take_ref)
> {
> *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule;
> - rule_dpif_ref(*rule);
> + if (take_ref) {
> + rule_dpif_ref(*rule);
> + }
> }
>
> void
> @@ -4197,7 +4224,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow
> *flow,
> if (ofpacts) {
> rule = NULL;
> } else {
> - rule_dpif_lookup(ofproto, flow, &trace.wc, &rule);
> + rule_dpif_lookup(ofproto, flow, &trace.wc, &rule, false);
>
> trace_format_rule(ds, 0, rule);
> if (rule == ofproto->miss_rule) {
> @@ -4253,8 +4280,6 @@ ofproto_trace(struct ofproto_dpif *ofproto, struct flow
> *flow,
>
> xlate_out_uninit(&trace.xout);
> }
> -
> - rule_dpif_unref(rule);
> }
>
> /* Store the current ofprotos in 'ofproto_shash'. Returns a sorted list
> @@ -4819,9 +4844,8 @@ ofproto_dpif_add_internal_flow(struct ofproto_dpif
> *ofproto,
> }
>
> rule = rule_dpif_lookup_in_table(ofproto, TBL_INTERNAL, &match->flow,
> - &match->wc);
> + &match->wc, false);
> if (rule) {
> - rule_dpif_unref(rule);
> *rulep = &rule->up;
> } else {
> OVS_NOT_REACHED();
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index eb4787c..88b6684 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -86,15 +86,25 @@ extern struct ovs_rwlock xlate_rwlock;
> size_t ofproto_dpif_get_max_mpls_depth(const struct ofproto_dpif *);
> bool ofproto_dpif_get_enable_recirc(const struct ofproto_dpif *);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
> uint8_t rule_dpif_lookup(struct ofproto_dpif *, struct flow *,
> - struct flow_wildcards *, struct rule_dpif **rule);
> + struct flow_wildcards *, struct rule_dpif **rule,
> + bool take_ref);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
> enum rule_dpif_lookup_verdict rule_dpif_lookup_from_table(struct
> ofproto_dpif *,
> const struct flow
> *,
> struct
> flow_wildcards *,
> bool
> force_controller_on_miss,
> uint8_t *table_id,
> - struct rule_dpif
> **rule);
> + struct rule_dpif
> **rule,
> + bool take_ref);
>
> void rule_dpif_ref(struct rule_dpif *);
> void rule_dpif_unref(struct rule_dpif *);
> @@ -114,10 +124,14 @@ ovs_be64 rule_dpif_get_flow_cookie(const struct
> rule_dpif *rule);
> void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
> uint16_t hard_timeout);
>
> +/* The rule is returned in '*rule', which is valid at least until the next
> + * RCU quiescent period. If the '*rule' needs to stay around longer,
> + * a non-zero 'take_ref' must be passed in to cause a reference to be taken
> + * on it before this returns. */
> void choose_miss_rule(enum ofputil_port_config,
> struct rule_dpif *miss_rule,
> struct rule_dpif *no_packet_in_rule,
> - struct rule_dpif **rule);
> + struct rule_dpif **rule, bool take_ref);
>
> bool group_dpif_lookup(struct ofproto_dpif *ofproto, uint32_t group_id,
> struct group_dpif **group);
> --
> 1.7.10.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
