Skip to content

Commit

Permalink
lib: Retire packet buffering feature.
Browse files Browse the repository at this point in the history 
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, the
packet buffering feature is removed 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.

Found by inspection.

Signed-off-by: Jarno Rajahalme <[email protected]>
Acked-by: Ben Pfaff <[email protected]>
  • Loading branch information
Jarno Rajahalme committed Aug 30, 2016
1 parent 88e6299 commit c184807
Show file tree
Hide file tree
Showing 16 changed files with 66 additions and 580 deletions.
64 changes: 15 additions & 49 deletions FAQ.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1970,55 +1970,21 @@ A: Reconfiguring your bridge can change your bridge's datapath-id because

ovs-vsctl set bridge br0 other-config:datapath-id=0123456789abcdef

### Q: My controller is getting errors about "buffers". What's going on?

A: When a switch sends a packet to an OpenFlow controller using a
"packet-in" message, it can also keep a copy of that packet in a
"buffer", identified by a 32-bit integer "buffer_id". There are
two advantages to buffering. First, when the controller wants to
tell the switch to do something with the buffered packet (with a
"packet-out" OpenFlow request), it does not need to send another
copy of the packet back across the OpenFlow connection, which
reduces the bandwidth cost of the connection and improves latency.
This enables the second advantage: the switch can optionally send
only the first part of the packet to the controller (assuming that
the switch only needs to look at the first few bytes of the
packet), further reducing bandwidth and improving latency.

However, buffering introduces some issues of its own. First, any
switch has limited resources, so if the controller does not use a
buffered packet, the switch has to decide how long to keep it
buffered. When many packets are sent to a controller and buffered,
Open vSwitch can discard buffered packets that the controller has
not used after as little as 5 seconds. This means that
controllers, if they make use of packet buffering, should use the
buffered packets promptly. (This includes sending a "packet-out"
with no actions if the controller does not want to do anything with
a buffered packet, to clear the packet buffer and effectively
"drop" its packet.)

Second, packet buffers are one-time-use, meaning that a controller
cannot use a single packet buffer in two or more "packet-out"
commands. Open vSwitch will respond with an error to the second
and subsequent "packet-out"s in such a case.

Finally, a common error early in controller development is to try
to use buffer_id 0 in a "packet-out" message as if 0 represented
"no buffered packet". This is incorrect usage: the buffer_id with
this meaning is actually 0xffffffff.

ovs-vswitchd(8) describes some details of Open vSwitch packet
buffering that the OpenFlow specification requires implementations
to document.

Note that the packet buffering support is deprecated in OVS 2.6
release, and will be removed in OVS 2.7. After the change OVS
always sends the 'buffer_id' as 0xffffffff in "packet-in" messages
and will send an error response if any other value of this field is
included in "packet-out" and "flow mod" sent by a controller.
Controllers are already expected to work properly in cases where
the switch can not buffer packets, so this change should not affect
existing users.
### Q: My controller complains that OVS is not buffering packets.
What's going on?

A: "Packet buffering" is an optional OpenFlow feature, and controllers
should detect how many "buffers" an OpenFlow switch implements. It
was recently noticed that OVS implementation of the buffering
feature was not compliant to OpenFlow specifications. Rather than
fix it and risk controller incompatibility, the buffering feature
is removed as of OVS 2.7. Controllers are already expected to work
properly in cases where the switch can not buffer packets, but
sends full packets in "packet-in" messages instead, so this change
should not affect existing users. After the change OVS always
sends the 'buffer_id' as 0xffffffff in "packet-in" messages and
will send an error response if any other value of this field is
included in a "packet-out" or a "flow mod" sent by a controller.

### Q: How does OVS divide flows among buckets in an OpenFlow "select" group?

Expand Down
4 changes: 1 addition & 3 deletions include/openvswitch/ofp-util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@
struct ofpbuf;
union ofp_action;
struct ofpact_set_field;
struct pktbuf;

/* Port numbers. */
enum ofperr ofputil_port_from_ofp11(ovs_be32 ofp11_port,
Expand Down Expand Up @@ -498,8 +497,7 @@ struct ofputil_packet_in_private {
struct ofpbuf *ofputil_encode_packet_in_private(
const struct ofputil_packet_in_private *,
enum ofputil_protocol protocol,
enum nx_packet_in_format,
uint16_t max_len, struct pktbuf *);
enum nx_packet_in_format);

enum ofperr ofputil_decode_packet_in_private(
const struct ofp_header *, bool loose,
Expand Down
2 changes: 0 additions & 2 deletions lib/automake.mk
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,6 @@ lib_libopenvswitch_la_SOURCES = \
lib/pcap-file.h \
lib/perf-counter.h \
lib/perf-counter.c \
lib/pktbuf.c \
lib/pktbuf.h \
lib/poll-loop.c \
lib/poll-loop.h \
lib/process.c \
Expand Down
80 changes: 24 additions & 56 deletions lib/ofp-util.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
#include "openvswitch/vlog.h"
#include "openflow/intel-ext.h"
#include "packets.h"
#include "pktbuf.h"
#include "random.h"
#include "tun-metadata.h"
#include "unaligned.h"
Expand Down Expand Up @@ -3588,17 +3587,14 @@ encode_packet_in_reason(enum ofp_packet_in_reason reason,
* function omits it. The caller can add it itself if desired. */
static void
ofputil_put_packet_in(const struct ofputil_packet_in *pin,
enum ofp_version version, uint32_t buffer_id,
size_t include_bytes, struct ofpbuf *msg)
enum ofp_version version, size_t include_bytes,
struct ofpbuf *msg)
{
/* Add packet properties. */
ofpprop_put(msg, NXPINT_PACKET, pin->packet, include_bytes);
if (include_bytes != pin->packet_len) {
ofpprop_put_u32(msg, NXPINT_FULL_LEN, pin->packet_len);
}
if (buffer_id != UINT32_MAX) {
ofpprop_put_u32(msg, NXPINT_BUFFER_ID, buffer_id);
}

/* Add flow properties. */
ofpprop_put_u8(msg, NXPINT_TABLE_ID, pin->table_id);
Expand Down Expand Up @@ -3642,11 +3638,10 @@ enum nx_continuation_prop_type {
* function omits it. The caller can add it itself if desired. */
static void
ofputil_put_packet_in_private(const struct ofputil_packet_in_private *pin,
enum ofp_version version, uint32_t buffer_id,
size_t include_bytes, struct ofpbuf *msg)
enum ofp_version version, size_t include_bytes,
struct ofpbuf *msg)
{
ofputil_put_packet_in(&pin->public, version, buffer_id,
include_bytes, msg);
ofputil_put_packet_in(&pin->public, version, include_bytes, msg);

size_t continuation_ofs = ofpprop_start_nested(msg, NXPINT_CONTINUATION);
size_t inner_ofs = msg->size;
Expand Down Expand Up @@ -3734,8 +3729,7 @@ ofputil_put_packet_in_private(const struct ofputil_packet_in_private *pin,
}

static struct ofpbuf *
ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin,
uint32_t buffer_id)
ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin)
{
struct ofp10_packet_in *opi;
struct ofpbuf *msg;
Expand All @@ -3746,14 +3740,14 @@ ofputil_encode_ofp10_packet_in(const struct ofputil_packet_in *pin,
opi->total_len = htons(pin->packet_len);
opi->in_port = htons(ofp_to_u16(pin->flow_metadata.flow.in_port.ofp_port));
opi->reason = encode_packet_in_reason(pin->reason, OFP10_VERSION);
opi->buffer_id = htonl(buffer_id);
opi->buffer_id = htonl(UINT32_MAX);

return msg;
}

static struct ofpbuf *
ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
enum ofp_version version, uint32_t buffer_id)
enum ofp_version version)
{
struct nx_packet_in *npi;
struct ofpbuf *msg;
Expand All @@ -3767,7 +3761,7 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,
ofpbuf_put_zeros(msg, 2);

npi = msg->msg;
npi->buffer_id = htonl(buffer_id);
npi->buffer_id = htonl(UINT32_MAX);
npi->total_len = htons(pin->packet_len);
npi->reason = encode_packet_in_reason(pin->reason, version);
npi->table_id = pin->table_id;
Expand All @@ -3779,8 +3773,7 @@ ofputil_encode_nx_packet_in(const struct ofputil_packet_in *pin,

static struct ofpbuf *
ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
enum ofp_version version, uint32_t buffer_id,
size_t include_bytes)
enum ofp_version version, size_t include_bytes)
{
/* 'extra' is just an estimate of the space required. */
size_t extra = (pin->public.packet_len
Expand All @@ -3792,7 +3785,7 @@ ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
struct ofpbuf *msg = ofpraw_alloc_xid(OFPRAW_NXT_PACKET_IN2, version,
htonl(0), extra);

ofputil_put_packet_in_private(pin, version, buffer_id, include_bytes, msg);
ofputil_put_packet_in_private(pin, version, include_bytes, msg);
if (pin->public.userdata_len) {
ofpprop_put(msg, NXPINT_USERDATA, pin->public.userdata,
pin->public.userdata_len);
Expand All @@ -3803,16 +3796,15 @@ ofputil_encode_nx_packet_in2(const struct ofputil_packet_in_private *pin,
}

static struct ofpbuf *
ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin,
uint32_t buffer_id)
ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin)
{
struct ofp11_packet_in *opi;
struct ofpbuf *msg;

msg = ofpraw_alloc_xid(OFPRAW_OFPT11_PACKET_IN, OFP11_VERSION,
htonl(0), pin->packet_len);
opi = ofpbuf_put_zeros(msg, sizeof *opi);
opi->buffer_id = htonl(buffer_id);
opi->buffer_id = htonl(UINT32_MAX);
opi->in_port = ofputil_port_to_ofp11(
pin->flow_metadata.flow.in_port.ofp_port);
opi->in_phy_port = opi->in_port;
Expand All @@ -3825,8 +3817,7 @@ ofputil_encode_ofp11_packet_in(const struct ofputil_packet_in *pin,

static struct ofpbuf *
ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
enum ofp_version version,
uint32_t buffer_id)
enum ofp_version version)
{
enum ofpraw raw = (version >= OFP13_VERSION
? OFPRAW_OFPT13_PACKET_IN
Expand All @@ -3838,7 +3829,7 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
htonl(0), NXM_TYPICAL_LEN + 2 + pin->packet_len);

struct ofp12_packet_in *opi = ofpbuf_put_zeros(msg, sizeof *opi);
opi->buffer_id = htonl(buffer_id);
opi->buffer_id = htonl(UINT32_MAX);
opi->total_len = htons(pin->packet_len);
opi->reason = encode_packet_in_reason(pin->reason, version);
opi->table_id = pin->table_id;
Expand All @@ -3857,11 +3848,6 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
/* Converts abstract ofputil_packet_in_private 'pin' into a PACKET_IN message
* for 'protocol', using the packet-in format specified by 'packet_in_format'.
*
* If 'pkt_buf' is nonnull and 'max_len' allows the packet to be buffered, this
* function will attempt to obtain a buffer ID from 'pktbuf' and truncate the
* packet to 'max_len' bytes. Otherwise, or if 'pktbuf' doesn't have a free
* buffer, it will send the whole packet without buffering.
*
* This function is really meant only for use by ovs-vswitchd. To any other
* code, the "continuation" data, i.e. the data that is in struct
* ofputil_packet_in_private but not in struct ofputil_packet_in, is supposed
Expand All @@ -3873,28 +3859,10 @@ ofputil_encode_ofp12_packet_in(const struct ofputil_packet_in *pin,
struct ofpbuf *
ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
enum ofputil_protocol protocol,
enum nx_packet_in_format packet_in_format,
uint16_t max_len, struct pktbuf *pktbuf)
enum nx_packet_in_format packet_in_format)
{
enum ofp_version version = ofputil_protocol_to_ofp_version(protocol);

/* Get buffer ID. */
ofp_port_t in_port = pin->public.flow_metadata.flow.in_port.ofp_port;
uint32_t buffer_id = (max_len != OFPCML12_NO_BUFFER && pktbuf
? pktbuf_save(pktbuf, pin->public.packet,
pin->public.packet_len, in_port)
: UINT32_MAX);

/* Calculate the number of bytes of the packet to include in the
* packet-in:
*
* - If not buffered, the whole thing.
*
* - Otherwise, no more than 'max_len' bytes. */
size_t include_bytes = (buffer_id == UINT32_MAX
? pin->public.packet_len
: MIN(max_len, pin->public.packet_len));

struct ofpbuf *msg;
switch (packet_in_format) {
case NXPIF_STANDARD:
Expand All @@ -3903,19 +3871,19 @@ ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
case OFPUTIL_P_OF10_STD_TID:
case OFPUTIL_P_OF10_NXM:
case OFPUTIL_P_OF10_NXM_TID:
msg = ofputil_encode_ofp10_packet_in(&pin->public, buffer_id);
msg = ofputil_encode_ofp10_packet_in(&pin->public);
break;

case OFPUTIL_P_OF11_STD:
msg = ofputil_encode_ofp11_packet_in(&pin->public, buffer_id);
msg = ofputil_encode_ofp11_packet_in(&pin->public);
break;

case OFPUTIL_P_OF12_OXM:
case OFPUTIL_P_OF13_OXM:
case OFPUTIL_P_OF14_OXM:
case OFPUTIL_P_OF15_OXM:
case OFPUTIL_P_OF16_OXM:
msg = ofputil_encode_ofp12_packet_in(&pin->public, version, buffer_id);
msg = ofputil_encode_ofp12_packet_in(&pin->public, version);
break;

default:
Expand All @@ -3924,18 +3892,18 @@ ofputil_encode_packet_in_private(const struct ofputil_packet_in_private *pin,
break;

case NXPIF_NXT_PACKET_IN:
msg = ofputil_encode_nx_packet_in(&pin->public, version, buffer_id);
msg = ofputil_encode_nx_packet_in(&pin->public, version);
break;

case NXPIF_NXT_PACKET_IN2:
return ofputil_encode_nx_packet_in2(pin, version, buffer_id,
include_bytes);
return ofputil_encode_nx_packet_in2(pin, version,
pin->public.packet_len);

default:
OVS_NOT_REACHED();
}

ofpbuf_put(msg, pin->public.packet, include_bytes);
ofpbuf_put(msg, pin->public.packet, pin->public.packet_len);
ofpmsg_update_length(msg);
return msg;
}
Expand Down Expand Up @@ -4004,7 +3972,7 @@ ofputil_encode_resume(const struct ofputil_packet_in *pin,
size_t extra = pin->packet_len + NXM_TYPICAL_LEN + continuation->size;
struct ofpbuf *msg = ofpraw_alloc_xid(OFPRAW_NXT_RESUME, version,
0, extra);
ofputil_put_packet_in(pin, version, UINT32_MAX, pin->packet_len, msg);
ofputil_put_packet_in(pin, version, pin->packet_len, msg);
ofpprop_put_nested(msg, NXPINT_CONTINUATION, continuation);
ofpmsg_update_length(msg);
return msg;
Expand Down
Loading

0 comments on commit c184807

Please sign in to comment.