On May 29, 2012, at 10:34 AM, Ben Pfaff wrote:
> On Fri, May 25, 2012 at 06:48:40PM -0700, Justin Pettit wrote:
>> Commit e72e793 (Add ability to restrict flow mods and flow stats
>> requests to cookies.) modified cookie handling. Some of its behavior
>> was unintuitive and there was at least one bug (described below).
>> Commit f66b87d (DESIGN: Document uses for flow cookies.) attempted to
>> document a clean design for cookie handling. This commit updates the
>> DESIGN document and brings the implementation in line with it.
>>
>> In commit e72e793, the code that handled processing OpenFlow flow
>> modification requests set the cookie mask to exact-match. This seems
>> reasonable for adding flows, but is not correct for matching, since
>> OpenFlow 1.0 doesn't support matching based on the cookie. This commit
>> changes to cookie mask to fully wildcarded, which is the correct
>> behavior for modifications and deletions. It doesn't cause any problems
>> for flow additions, since the mask is ignored for that operation.
>>
>> Bug #9742
>>
>> Reported-by: Luca Giraudo <lgira...@nicira.com>
>> Reported-by: Paul Ingram <p...@nicira.com>
>> Signed-off-by: Justin Pettit <jpet...@nicira.com>
>
> Thank you. I only have small comments.
Oh, thank Christ. :-)
> In DESIGN below, I think that the new described behavior is actually
> the same as OpenFlow 1.1, just described in different words:
>> + - But, unlike OpenFlow 1.1, OFPC_MODIFY and OFPFC_MODIFY_STRICT
>> + add a new flow if there is no match and the mask is zero (or
>> + not given). This provides OpenFlow 1.0-like behavior when a
>> + mask is not specified.
You're right. I still want to highlight that you don't need to specify a mask,
so I've reworded it to the following:
- Like OpenFlow 1.1, OFPC_MODIFY and OFPFC_MODIFY_STRICT add a
new flow if there is no match and the mask is zero (or not
given).
> In ofp_print_flow_mod(), the old version printed fm.cookie, the new
> version prints fm.new_cookie. Shouldn't we print both (potentially)?
How about the following right after we print fm.new_cookie?
if (fm.cookie_mask != htonll(0)) {
ds_put_format(s, "cookie:0x%"PRIx64"/0x%"PRIx64" ",
ntohll(fm.cookie), ntohll(fm.cookie_mask));
}
> I think that the precedence of == is higher than &, so that we need
> parentheses around "command & 0xff" below:
>> + if (command & 0xff == OFPFC_ADD && fm->cookie_mask) {
Whoops. Thank you.
> Should add "\" before each "-" below (not a new problem):
>> +supplied for the \fBdel-flows\fR, \fBmod-flows\fR, \fBdump-flows\fR, and
>> +\fBdump-aggregate\fR commands to limit matching cookies. A 1-bit in
>> +\fImask\fR indicates that the corresponding bit in \fIcookie\fR must
Done.
> Also in "-1" here (though not in "0-bit"):
>> +match exactly, and a 0-bit wildcards that bit. A mask of -1 may be used
>> +to exactly match a cookie.
Done.
> Also here:
>> +The \fBmod-flows\fR command can update the cookies of flows that
>> +match a cookie by specifying the \fIcookie\fR field twice (once with a
>> +mask for matching and once without to indicate the new value):
>> +.RS
Done.
> Also here:
>> +.IP \fBcookie=1/-1,cookie=2\fR
>> +Update cookies with a value of 1 to 2.
Done.
> The following example is rather confusing. "cookie=0/0" is a no-op
> AFAIK, is there any reason to ever write that explicitly? Also the
> wording is ambiguous. I'd say "Change all flows' cookies to 1." or
> something else that is equally clear:
>> +.IP \fBcookie=0/0,cookie=1\fR
>> +Update all flows with a cookie of 1.
>> +.RE
I did it to provide more examples, but it probably was just confusing. I
removed "cookie=0/0" and reordered the examples, which I think makes things
clearer. I also removed the ambiguity.
> It might be wise to give complete ovs-ofctl commands above, instead of
> just excerpts, and to describe the full effect of the command instead
> of just the effect on cookies (e.g. a complete "mod-flows" would also
> change the flows' actions). Otherwise readers might still end up
> misunderstanding.
Okay, combined with the previous comment, it now looks like this:
.IP "\fBovs\-ofctl mod\-flows br0 cookie=1,action=normal\fR"
Change all flows' cookies to 1 and change their actions to \fBnormal\fR.
.IP "\fBovs\-ofctl mod\-flows br0 cookie=1/\-1,cookie=2,action=normal\fR"
Update cookies with a value of 1 to 2 and change their actions to
\fBnormal\fR.
If all those changes look reasonable, I'll push it.
--Justin
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
