I removed the comments from the .h file before pushing.
Jarno
On Apr 23, 2014, at 4:53 PM, Ethan Jackson <et...@nicira.com> wrote:
> 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
