[ovs-dev] [PATCH 5/5] dpif-netdev: batch packet sending

[Daniele Di Proietto]

On May 30, 2014, at 4:55 PM, Pravin Shelar <pshelar at nicira.com> wrote:

>> 
>>     int i;
>> 
>> +    memset(&batch, 0, sizeof batch);
>> +
> Is there need to reset batch? can batch_init() take care of it?

You’re right. It should be enough to set batch.flow to NULL.

> 
>> +    for (i = 0; i < c; i++) {
>> +        struct ofpbuf * packet = packets[i];
> extra space before packet.
> need blank line after declaration.

Fixed, sorry.

>> +        if (q->head - q->tail < MAX_QUEUE_LEN) {
>> +            struct dp_netdev_upcall *u = &q->upcalls[q->head++ & QUEUE_MASK];
>> +            struct dpif_upcall *upcall = &u->upcall;
>> +            struct ofpbuf *buf = &u->buf;
>> +            size_t buf_size;
>> +            struct flow flow;
>> +
>> +            upcall->type = type;
>> +
>> +            /* Allocate buffer big enough for everything. */
>> +            buf_size = ODPUTIL_FLOW_KEY_BYTES;
>> +            if (userdata) {
>> +                buf_size += NLA_ALIGN(userdata->nla_len);
>> +            }
>> +            buf_size += ofpbuf_size(packet);
>> +            ofpbuf_init(buf, buf_size);
>> +
>> +            /* Put ODP flow. */
>> +            miniflow_expand(key, &flow);
>> +            odp_flow_key_from_flow(buf, &flow, NULL, flow.in_port.odp_port, true);
>> +            upcall->key = ofpbuf_data(buf);
>> +            upcall->key_len = ofpbuf_size(buf);
>> +
>> +            /* Put userdata. */
>> +            if (userdata) {
>> +                upcall->userdata = ofpbuf_put(buf, userdata,
>> +                                              NLA_ALIGN(userdata->nla_len));
>> +            }
>> 
>> -        ofpbuf_set_data(&upcall->packet,
>> -                        ofpbuf_put(buf, ofpbuf_data(packet), ofpbuf_size(packet)));
>> -        ofpbuf_set_size(&upcall->packet, ofpbuf_size(packet));
>> +            ofpbuf_set_data(&upcall->packet,
>> +                            ofpbuf_put(buf, ofpbuf_data(packet),
>> +                            ofpbuf_size(packet)));
>> +            ofpbuf_set_size(&upcall->packet, ofpbuf_size(packet));
>> 
>> -        seq_change(q->seq);
>> +            seq_change(q->seq);
>> 
>> -        error = 0;
>> -    } else {
>> -        dp_netdev_count_packet(dp, DP_STAT_LOST, 1);
>> -        error = ENOBUFS;
>> +            error = 0;
>> +        } else {
>> +            dp_netdev_count_packet(dp, DP_STAT_LOST, 1);
>> +            error = ENOBUFS;
>> +        }
> Can you make separate function to send a packet for upcall processing?
> it is easier to read that way.

Sure, I did it. At some point, though, we might want to batch that too.

Thanks for the suggestions, I’m about to send an updated patch set.

Daniele

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140603/df944238/attachment-0005.html>