[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