[ovs-dev] [PATCH 2/2] ofproto: Avoid buffer copy in OFPT_PACKET_IN path.
Justin Pettit
jpettit at nicira.com
Sat Apr 24 08:29:41 UTC 2010
On Apr 9, 2010, at 12:38 PM, Ben Pfaff wrote:
> + /* Repurpose packet buffer, only override header */
> + ofpbuf_pull(packet, sizeof(struct odp_msg));
> + opi = ofpbuf_push_zeros(packet, offsetof(struct ofp_packet_in, data));
> + opi->header.version = OFP_VERSION;
> + opi->header.type = OFPT_PACKET_IN;
> + opi->total_len = htons(total_len);
> + opi->in_port = htons(in_port);
> + opi->reason = reason;
> +
> + /* We are not done, we need to fix up the buffer length, set the
> + * length in the OpenFlow header and set buffer_id.
> + * That depends on the controller settings. */
> +}
>
I don't see where "xid" is set in the OpenFlow header anywhere.
> static void
> @@ -3664,15 +3702,30 @@ send_packet_in_action(struct ofpbuf *packet, void *p_)
> {
> struct ofproto *p = p_;
> struct ofconn *ofconn;
> + struct ofconn *prev_ofconn = NULL;
> struct odp_msg *msg;
> + uint32_t arg = 0;
Do you need to initialize the value, since it gets set right away? Also, I think it would be clearer if the variable were named for its actual purpose in this call, such as "max_len".
> msg = packet->data;
> + arg = msg->arg;
> +
> + do_convert_to_packet_in(packet);
> +
> LIST_FOR_EACH (ofconn, struct ofconn, node, &p->all_conns) {
> if (ofconn == p->controller || ofconn->miss_send_len) {
> - do_send_packet_in(ofconn, UINT32_MAX, packet, msg->arg);
> + if (prev_ofconn) {
> + do_send_converted_packet_in(prev_ofconn, INT32_MAX,
Shouldn't that be UINT32_MAX?
> + packet, true, arg);
> + }
> + prev_ofconn = ofconn;
> }
> }
> - ofpbuf_delete(packet);
> + if (prev_ofconn) {
> + do_send_converted_packet_in(prev_ofconn, INT32_MAX, packet, false,
Similarly, I would think this should be UINT32_MAX, too.
> + arg);
> + } else {
> + ofpbuf_delete(packet);
> + }
> }
>
> static void
> @@ -3681,24 +3734,40 @@ send_packet_in_miss(struct ofpbuf *packet, void *p_)
> struct ofproto *p = p_;
> bool in_fail_open = p->fail_open && fail_open_is_active(p->fail_open);
> struct ofconn *ofconn;
> + struct ofconn *prev_ofconn = NULL;
> struct ofpbuf payload;
> struct odp_msg *msg;
> + uint16_t port;
> + uint32_t buffer_id = 0;
> + int send_len = 0;
I don't think it's necessary to initialize either of these variables, since they don't seem to be referenced before being set.
> msg = packet->data;
> payload.data = msg + 1;
> payload.size = msg->length - sizeof *msg;
> + port = msg->port;
Do you need to do a odp_port_to_ofp_port() on msg->port?
Otherwise, it looks good. It's always nice reduce the amount of copying going on!
--Justin
More information about the dev
mailing list