[ovs-dev] [PATCH v2 02/15] lib: Retire packet buffering feature.

Ben Pfaff blp at ovn.org
Tue Aug 30 00:03:01 UTC 2016


On Mon, Aug 29, 2016 at 02:07:40PM -0700, Ben Pfaff wrote:
> On Mon, Aug 22, 2016 at 04:31:28PM -0700, Jarno Rajahalme wrote:
> > OVS implementation of buffering packets that are sent to the
> > controller is not compliant with the OpenFlow specifications after
> > OpenFlow 1.0, which is possibly true since OpenFlow 1.0 is not really
> > specifying the packet buffering behavior.
> > 
> > OVS implementation executes the buffered packet against the actions of
> > the modified or added rule, whereas OpenFlow (since 1.1) specifies
> > that the packet should be matched against the flow table 0 and
> > processed accordingly.
> > 
> > Rather than fix this behavior, and potentially break OVS users, we
> > propose to remove the feature altogether. After all, such packet
> > buffering is an optional OpenFlow feature, and as such any possible
> > users should continue to work without this feature.
> > 
> > This patch also makes OVS check the received 'buffer_id' values more
> > rigorously, and fixes some internal users accordingly.
> > 
> > Signed-off-by: Jarno Rajahalme <jarno at ovn.org>
> 
> I believe that this came out of a bug report.  Will you credit the bug
> reporter?
> 
> I think that we will probably start getting a question about "what
> happened to the buffers?" so it might be worthwhile to add a question
> and answer to the FAQ about that.
> 
> The change to lib/learn.c seems unnecessary.
> 
> I'd be inclined to also remove the buffer_id parameters from the
> functions that ofputil_encode_packet_in_private() calls.  They're always
> going to be UINT32_MAX now.
> 
> In ofputil_encode_packet_in_private(), here:
>     if (check_buffer_id && fm->buffer_id != UINT32_MAX) {
>         error = OFPERR_OFPBRC_BUFFER_UNKNOWN;
>     }
> I would add "!error &&" to the condition, because I think that this
> error is less important than the others.
> 
> The changes to ofctrl.c should not be necessary, because the
> encode_flow_mod() function that all of the cases use already sets
> buffer_id to UINT32_MAX.

I think that we can also now eliminate pin.max_len from struct
ofproto_async_msg, because it no longer has any visible effect on
behavior.



More information about the dev mailing list