[ovs-dev] [PATCH 2/2] ofproto: Make OFPFC_MODIFY and OFPFC_MODIFY_STRICT add a flow if no match.
Justin Pettit
jpettit at nicira.com
Fri Apr 2 22:55:09 UTC 2010
I'm going to assume that your revised version is perfect. :)
--Justin
On Apr 2, 2010, at 3:15 PM, Ben Pfaff wrote:
> Updated patch at very end of this email. Before that, replies to comments.
>
> On Wed, Mar 24, 2010 at 03:57:03PM -0700, Justin Pettit wrote:
>> On Mar 19, 2010, at 12:49 PM, Ben Pfaff wrote:
>>> +/* Implements OFPFC_MODIFY. Returns 0 on success or an OpenFlow error code as
>>> + * encoded by ofp_mkerr() on failure. */
>>> +static int
>>> +modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
>>> + size_t n_actions)
>>> +{
>>> + struct modify_flows_cbdata cbdata;
>>> + struct cls_rule target;
>>> +
>>> + cbdata.ofproto = p;
>>> + cbdata.ofm = ofm;
>>> + cbdata.n_actions = n_actions;
>>> + cbdata.matched = false;
>>> +
>>> + cls_rule_from_match(&target, &ofm->match, 0);
>>> +
>>> + classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
>>> + modify_flows_cb, &cbdata);
>>> + if (!cbdata.matched) {
>>> + return add_flow(p, NULL, ofm, n_actions);
>>
>> I think this assumes that Modify commands never have a buffer ID, but
>> I don't think that's true.
>
> I was trying to ignore that possibility. I guess you're right, though.
> OK, I updated the patch.
>
>>> +/* Implements OFPFC_MODIFY_STRICT. Returns 0 on success or an OpenFlow error
>>> + * code as encoded by ofp_mkerr() on failure. */
>>> +static int
>>> +modify_flow_strict(struct ofproto *p,
>>> + struct ofp_flow_mod *ofm, size_t n_actions)
>>> +{
>>> + struct rule *rule = find_flow_strict(p, ofm);
>>> + return (rule && !rule_is_hidden(rule)
>>> + ? modify_flow(p, ofm, n_actions, rule)
>>> + : add_flow(p, NULL, ofm, n_actions));
>>
>> Same here, I think Modify Strict commands can have a valid buffer id.
>
> Argh.
>
>>> +static void delete_flows_cb(struct cls_rule *, void *cbdata_);
>>> +static void delete_flow(struct ofproto *, struct rule *, uint16_t out_port);
>>> +
>>> +/* Implements OFPFC_DELETE. Returns 0 on success or an OpenFlow error code as
>>> + * encoded by ofp_mkerr() on failure. */
>>> static int
>>> -modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
>>> - size_t n_actions, uint16_t command)
>>> +delete_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm)
>>> {
>>> ...
>>> +
>>> + return 0;
>>> +}
>>
>> I don't see how this will return an OpenFlow error, which the
>> description implies it might.
>
> OK, I changed it to void.
>
>>> +/* Implements OFPFC_DELETE_STRICT. Returns 0 on success or an OpenFlow error
>>> + * code as encoded by ofp_mkerr() on failure. */
>>> +static int
>>> +delete_flow_strict(struct ofproto *p, struct ofp_flow_mod *ofm)
>>> +{
>>> + struct rule *rule = find_flow_strict(p, ofm);
>>> + delete_flow(p, rule, ofm->out_port);
>>
>> I think "rule" can be NULL if it's not found, which could cause
>> problems for delete_flow().
>
> I guess you missed the section of the OpenFlow spec that says:
>
> For OFPFC_DELETE_STRICT, if the specified flow does not exist,
> then the switch must segfault and take down your network.
>
> But I've never agreed with that part of the spec, so I went ahead and
> fixed this anyhow.
>
>>> return 0;
>>> }
>>
>> Once again, I don't see how this will return an OpenFlow error, which
>> the description implies it might.
>
> OK, changed to void.
>
>>> +/* Implements core of OFPFC_DELETE and OFPFC_DELETE_STRICT where 'rule' has
>>> + * been identified as a flow to delete from 'p''s flow table, by deleting the
>>> + * flow and sending out a OFPT_FLOW_REMOVED message to any interested
>>> + * controller.
>>> + *
>>> + * Will not delete 'rule' if it is hidden or, if 'out_port' is not
>>> + * htons(OFPP_NONE), if it does not output to port 'out_port'. */
>>
>> This description was kind of confusing to me. How about something along the lines of:
>>
>> Will not delete 'rule' if it is hidden or if outputting to 'out_port' is not
>> among the actions when 'out_port' is not htons(OFPP_NONE).
>
> I changed it to this:
>
> * Will not delete 'rule' if it is hidden. Will delete 'rule' only if
> * 'out_port' is htons(OFPP_NONE) or if 'rule' actually outputs to the
> * specified 'out_port'. */
>
> Seems clearer to me, too, now.
>
> From ee54c60cc6d8392e4c34d007444b9d0398bec9fe Mon Sep 17 00:00:00 2001
> From: Ben Pfaff <blp at nicira.com>
> Date: Fri, 2 Apr 2010 15:12:42 -0700
> Subject: [PATCH] ofproto: Make OFPFC_MODIFY and OFPFC_MODIFY_STRICT add a flow if no match.
>
> OpenFlow 1.0 says that OFPFC_MODIFY and OFPFC_MODIFY_STRICT are supposed
> to add the specified flow to the flow table if it does not already contain
> one that matches.
>
> Reported-by: Natasha Gude <natasha at nicira.com>
>
> Bug #2506.
> ---
> ofproto/ofproto.c | 270 +++++++++++++++++++++++++++++++++++++++--------------
> 1 files changed, 198 insertions(+), 72 deletions(-)
>
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 9efc96e..fd1256f 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -109,6 +109,9 @@ struct rule {
>
> /* OpenFlow actions.
> *
> + * 'n_actions' is the number of elements in the 'actions' array. A single
> + * action may take up more more than one element's worth of space.
> + *
> * A subrule has no actions (it uses the super-rule's actions). */
> int n_actions;
> union ofp_action *actions;
> @@ -2874,9 +2877,18 @@ update_stats(struct ofproto *ofproto, struct rule *rule,
> }
> }
>
> +/* Implements OFPFC_ADD and the cases for OFPFC_MODIFY and OFPFC_MODIFY_STRICT
> + * in which no matching flow already exists in the flow table.
> + *
> + * Adds the flow specified by 'ofm', which is followed by 'n_actions'
> + * ofp_actions, to 'p''s flow table. Returns 0 on success or an OpenFlow error
> + * code as encoded by ofp_mkerr() on failure.
> + *
> + * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id,
> + * if any. */
> static int
> add_flow(struct ofproto *p, struct ofconn *ofconn,
> - struct ofp_flow_mod *ofm, size_t n_actions)
> + const struct ofp_flow_mod *ofm, size_t n_actions)
> {
> struct ofpbuf *packet;
> struct rule *rule;
> @@ -2914,112 +2926,224 @@ add_flow(struct ofproto *p, struct ofconn *ofconn,
> return error;
> }
>
> -static int
> -modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
> - size_t n_actions, uint16_t command, struct rule *rule)
> -{
> - if (rule_is_hidden(rule)) {
> - return 0;
> - }
> -
> - if (command == OFPFC_DELETE) {
> - long long int now = time_msec();
> - send_flow_removed(p, rule, now, OFPRR_DELETE);
> - rule_remove(p, rule);
> - } else {
> - size_t actions_len = n_actions * sizeof *rule->actions;
> -
> - rule->flow_cookie = ofm->cookie;
> - if (n_actions == rule->n_actions
> - && !memcmp(ofm->actions, rule->actions, actions_len))
> - {
> - return 0;
> - }
> -
> - free(rule->actions);
> - rule->actions = xmemdup(ofm->actions, actions_len);
> - rule->n_actions = n_actions;
> -
> - if (rule->cr.wc.wildcards) {
> - COVERAGE_INC(ofproto_mod_wc_flow);
> - p->need_revalidate = true;
> - } else {
> - rule_update_actions(p, rule);
> - }
> - }
> -
> - return 0;
> -}
> -
> -static int
> -modify_flows_strict(struct ofproto *p, const struct ofp_flow_mod *ofm,
> - size_t n_actions, uint16_t command)
> +static struct rule *
> +find_flow_strict(struct ofproto *p, const struct ofp_flow_mod *ofm)
> {
> - struct rule *rule;
> uint32_t wildcards;
> flow_t flow;
>
> flow_from_match(&flow, &wildcards, &ofm->match);
> - rule = rule_from_cls_rule(classifier_find_rule_exactly(
> + return rule_from_cls_rule(classifier_find_rule_exactly(
> &p->cls, &flow, wildcards,
> ntohs(ofm->priority)));
> +}
>
> - if (rule) {
> - if (command == OFPFC_DELETE
> - && ofm->out_port != htons(OFPP_NONE)
> - && !rule_has_out_port(rule, ofm->out_port)) {
> - return 0;
> - }
> +static int
> +send_buffered_packet(struct ofproto *ofproto, struct ofconn *ofconn,
> + struct rule *rule, const struct ofp_flow_mod *ofm)
> +{
> + struct ofpbuf *packet;
> + uint16_t in_port;
> + flow_t flow;
> + int error;
>
> - modify_flow(p, ofm, n_actions, command, rule);
> + if (ofm->buffer_id == htonl(UINT32_MAX)) {
> + return 0;
> }
> +
> + error = pktbuf_retrieve(ofconn->pktbuf, ntohl(ofm->buffer_id),
> + &packet, &in_port);
> + if (error) {
> + return error;
> + }
> +
> + flow_extract(packet, in_port, &flow);
> + rule_execute(ofproto, rule, packet, &flow);
> + ofpbuf_delete(packet);
> +
> return 0;
> }
> +
> +/* OFPFC_MODIFY and OFPFC_MODIFY_STRICT. */
>
> struct modify_flows_cbdata {
> struct ofproto *ofproto;
> const struct ofp_flow_mod *ofm;
> - uint16_t out_port;
> size_t n_actions;
> - uint16_t command;
> + struct rule *match;
> };
>
> +static int modify_flow(struct ofproto *, const struct ofp_flow_mod *,
> + size_t n_actions, struct rule *);
> +static void modify_flows_cb(struct cls_rule *, void *cbdata_);
> +
> +/* Implements OFPFC_MODIFY. Returns 0 on success or an OpenFlow error code as
> + * encoded by ofp_mkerr() on failure.
> + *
> + * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id,
> + * if any. */
> +static int
> +modify_flows_loose(struct ofproto *p, struct ofconn *ofconn,
> + const struct ofp_flow_mod *ofm, size_t n_actions)
> +{
> + struct modify_flows_cbdata cbdata;
> + struct cls_rule target;
> +
> + cbdata.ofproto = p;
> + cbdata.ofm = ofm;
> + cbdata.n_actions = n_actions;
> + cbdata.match = NULL;
> +
> + cls_rule_from_match(&target, &ofm->match, 0);
> +
> + classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
> + modify_flows_cb, &cbdata);
> + if (cbdata.match) {
> + /* This credits the packet to whichever flow happened to happened to
> + * match last. That's weird. Maybe we should do a lookup for the
> + * flow that actually matches the packet? Who knows. */
> + send_buffered_packet(p, ofconn, cbdata.match, ofm);
> + return 0;
> + } else {
> + return add_flow(p, ofconn, ofm, n_actions);
> + }
> +}
> +
> +/* Implements OFPFC_MODIFY_STRICT. Returns 0 on success or an OpenFlow error
> + * code as encoded by ofp_mkerr() on failure.
> + *
> + * 'ofconn' is used to retrieve the packet buffer specified in ofm->buffer_id,
> + * if any. */
> +static int
> +modify_flow_strict(struct ofproto *p, struct ofconn *ofconn,
> + struct ofp_flow_mod *ofm, size_t n_actions)
> +{
> + struct rule *rule = find_flow_strict(p, ofm);
> + if (rule && !rule_is_hidden(rule)) {
> + modify_flow(p, ofm, n_actions, rule);
> + return send_buffered_packet(p, ofconn, rule, ofm);
> + } else {
> + return add_flow(p, ofconn, ofm, n_actions);
> + }
> +}
> +
> +/* Callback for modify_flows_loose(). */
> static void
> modify_flows_cb(struct cls_rule *rule_, void *cbdata_)
> {
> struct rule *rule = rule_from_cls_rule(rule_);
> struct modify_flows_cbdata *cbdata = cbdata_;
>
> - if (cbdata->out_port != htons(OFPP_NONE)
> - && !rule_has_out_port(rule, cbdata->out_port)) {
> - return;
> + if (!rule_is_hidden(rule)) {
> + cbdata->match = rule;
> + modify_flow(cbdata->ofproto, cbdata->ofm, cbdata->n_actions, rule);
> }
> -
> - modify_flow(cbdata->ofproto, cbdata->ofm, cbdata->n_actions,
> - cbdata->command, rule);
> }
>
> +/* Implements core of OFPFC_MODIFY and OFPFC_MODIFY_STRICT where 'rule' has
> + * been identified as a flow in 'p''s flow table to be modified, by changing
> + * the rule's actions to match those in 'ofm' (which is followed by 'n_actions'
> + * ofp_action[] structures). */
> static int
> -modify_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm,
> - size_t n_actions, uint16_t command)
> +modify_flow(struct ofproto *p, const struct ofp_flow_mod *ofm,
> + size_t n_actions, struct rule *rule)
> {
> - struct modify_flows_cbdata cbdata;
> + size_t actions_len = n_actions * sizeof *rule->actions;
> +
> + rule->flow_cookie = ofm->cookie;
> +
> + /* If the actions are the same, do nothing. */
> + if (n_actions == rule->n_actions
> + && !memcmp(ofm->actions, rule->actions, actions_len))
> + {
> + return 0;
> + }
> +
> + /* Replace actions. */
> + free(rule->actions);
> + rule->actions = xmemdup(ofm->actions, actions_len);
> + rule->n_actions = n_actions;
> +
> + /* Make sure that the datapath gets updated properly. */
> + if (rule->cr.wc.wildcards) {
> + COVERAGE_INC(ofproto_mod_wc_flow);
> + p->need_revalidate = true;
> + } else {
> + rule_update_actions(p, rule);
> + }
> +
> + return 0;
> +}
> +
> +/* OFPFC_DELETE implementation. */
> +
> +struct delete_flows_cbdata {
> + struct ofproto *ofproto;
> + uint16_t out_port;
> +};
> +
> +static void delete_flows_cb(struct cls_rule *, void *cbdata_);
> +static void delete_flow(struct ofproto *, struct rule *, uint16_t out_port);
> +
> +/* Implements OFPFC_DELETE. */
> +static void
> +delete_flows_loose(struct ofproto *p, const struct ofp_flow_mod *ofm)
> +{
> + struct delete_flows_cbdata cbdata;
> struct cls_rule target;
>
> cbdata.ofproto = p;
> - cbdata.ofm = ofm;
> - cbdata.out_port = (command == OFPFC_DELETE ? ofm->out_port
> - : htons(OFPP_NONE));
> - cbdata.n_actions = n_actions;
> - cbdata.command = command;
> + cbdata.out_port = ofm->out_port;
>
> cls_rule_from_match(&target, &ofm->match, 0);
>
> classifier_for_each_match(&p->cls, &target, CLS_INC_ALL,
> - modify_flows_cb, &cbdata);
> - return 0;
> + delete_flows_cb, &cbdata);
> +}
> +
> +/* Implements OFPFC_DELETE_STRICT. */
> +static void
> +delete_flow_strict(struct ofproto *p, struct ofp_flow_mod *ofm)
> +{
> + struct rule *rule = find_flow_strict(p, ofm);
> + if (rule) {
> + delete_flow(p, rule, ofm->out_port);
> + }
> +}
> +
> +/* Callback for delete_flows_loose(). */
> +static void
> +delete_flows_cb(struct cls_rule *rule_, void *cbdata_)
> +{
> + struct rule *rule = rule_from_cls_rule(rule_);
> + struct delete_flows_cbdata *cbdata = cbdata_;
> +
> + delete_flow(cbdata->ofproto, rule, cbdata->out_port);
> }
>
> +/* Implements core of OFPFC_DELETE and OFPFC_DELETE_STRICT where 'rule' has
> + * been identified as a flow to delete from 'p''s flow table, by deleting the
> + * flow and sending out a OFPT_FLOW_REMOVED message to any interested
> + * controller.
> + *
> + * Will not delete 'rule' if it is hidden. Will delete 'rule' only if
> + * 'out_port' is htons(OFPP_NONE) or if 'rule' actually outputs to the
> + * specified 'out_port'. */
> +static void
> +delete_flow(struct ofproto *p, struct rule *rule, uint16_t out_port)
> +{
> + if (rule_is_hidden(rule)) {
> + return;
> + }
> +
> + if (out_port != htons(OFPP_NONE) && !rule_has_out_port(rule, out_port)) {
> + return;
> + }
> +
> + send_flow_removed(p, rule, time_msec(), OFPRR_DELETE);
> + rule_remove(p, rule);
> +}
> +
> static int
> handle_flow_mod(struct ofproto *p, struct ofconn *ofconn,
> struct ofp_flow_mod *ofm)
> @@ -3057,16 +3181,18 @@ handle_flow_mod(struct ofproto *p, struct ofconn *ofconn,
> return add_flow(p, ofconn, ofm, n_actions);
>
> case OFPFC_MODIFY:
> - return modify_flows_loose(p, ofm, n_actions, OFPFC_MODIFY);
> + return modify_flows_loose(p, ofconn, ofm, n_actions);
>
> case OFPFC_MODIFY_STRICT:
> - return modify_flows_strict(p, ofm, n_actions, OFPFC_MODIFY);
> + return modify_flow_strict(p, ofconn, ofm, n_actions);
>
> case OFPFC_DELETE:
> - return modify_flows_loose(p, ofm, n_actions, OFPFC_DELETE);
> + delete_flows_loose(p, ofm);
> + return 0;
>
> case OFPFC_DELETE_STRICT:
> - return modify_flows_strict(p, ofm, n_actions, OFPFC_DELETE);
> + delete_flow_strict(p, ofm);
> + return 0;
>
> default:
> return ofp_mkerr(OFPET_FLOW_MOD_FAILED, OFPFMFC_BAD_COMMAND);
> --
> 1.6.6.1
>
More information about the dev
mailing list