[ovs-dev] [PATCH V2 01/10] netdev: Extend rx_recv to pass multiple packets.

[Pravin Shelar]

On Fri, Mar 21, 2014 at 1:55 PM, Thomas Graf <tgraf at redhat.com> wrote:
> On 03/21/2014 07:02 PM, Pravin wrote:
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk
>>
>> @@ -1863,7 +1850,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct
>> ofpbuf *packet,
>>           dp_netdev_flow_used(netdev_flow, packet, &key);
>>
>>           actions = dp_netdev_flow_get_actions(netdev_flow);
>> -        dp_netdev_execute_actions(dp, &key, packet, md,
>> +        dp_netdev_execute_actions(dp, &key, packet, true, md,
>>                                     actions->actions, actions->size);
>
>
> I am not 100% sure I understand who is responsible for deleting the
> packet, e.g. if the actions are not terminated with an output or
> userspace. Or is that never possible?
>
I think it is not possible. Why vswitchd would generate such action if
the packet is not going somewhere?

>
>>           dp_netdev_count_packet(dp, DP_STAT_HIT);
>>       } else if (dp->handler_queues) {
>> @@ -1871,6 +1858,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct
>> ofpbuf *packet,
>>           dp_netdev_output_userspace(dp, packet,
>>                                      flow_hash_5tuple(&key, 0) %
>> dp->n_handlers,
>>                                      DPIF_UC_MISS, &key, NULL);
>> +        ofpbuf_delete(packet);
>
>
> Given we now have to free packets if they are not stolen, the
> if (packet->size < ETH_HEADER_LEN) check in dp_netdev_port_input()
> seems to leak now. How about dp_netdev_port_input() returns bool
> indicating whether packet was stolen or not so we can free it in a
> central place?
>

right, there is memory leak for packet size less than eth-header.
unfortunately dp_netdev_port_input() would not know if packet is
deleted, since it can send it to odp-execute.
At this point there is three places where we delete packet:
1. dp_netdev_port_input()
2. odp action (drop)
3. odp-actions callback.

I do not think if we can centralize it.
>
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index e9c8d8f..0d7b69d 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -551,22 +551,13 @@ netdev_rx_close(struct netdev_rx *rx)
>>       }
>>   }
>>
>> -/* Attempts to receive a packet from 'rx' into the tailroom of 'buffer',
>> which
>> - * must initially be empty.  If successful, returns 0 and increments
>> - * 'buffer->size' by the number of bytes in the received packet,
>> otherwise a
>> - * positive errno value.
>> +/* Attempts to receive batch of packets from 'rx'.
>>    *
>>    * Returns EAGAIN immediately if no packet is ready to be received.
>>    *
>>    * Returns EMSGSIZE, and discards the packet, if the received packet is
>> longer
>>    * than 'ofpbuf_tailroom(buffer)'.
>>    *
>> - * Implementations may make use of VLAN_HEADER_LEN bytes of tailroom to
>> - * add a VLAN header which is obtained out-of-band to the packet. If
>> - * this occurs then VLAN_HEADER_LEN bytes of tailroom will no longer be
>> - * available for the packet, otherwise it may be used for the packet
>> - * itself.
>
>
> Any reason for removing this comment other than that it seems oudated?
> The headroom containing space for an additional VLAN header should be
> documented somewhere.
>
Since it is describing packet allocated by dpif-netdev. Now it is
moved to netdev implementation. I will update comment.

>
>> @@ -575,23 +566,15 @@ netdev_rx_close(struct netdev_rx *rx)
>>    * This function may be set to null if it would always return EOPNOTSUPP
>>    * anyhow. */
>>   int
>> -netdev_rx_recv(struct netdev_rx *rx, struct ofpbuf *buffer)
>> +netdev_rx_recv(struct netdev_rx *rx, struct ofpbuf **buffers, int *cnt)
>>   {
>>       int retval;
>>
>> -    ovs_assert(buffer->size == 0);
>> -    ovs_assert(ofpbuf_tailroom(buffer) >= ETH_TOTAL_MIN);
>> -
>> -    retval = rx->netdev->netdev_class->rx_recv(rx, buffer);
>> +    retval = rx->netdev->netdev_class->rx_recv(rx, buffers, cnt);
>>       if (!retval) {
>>           COVERAGE_INC(netdev_received);
>
>
> What would be awesome here is a COVERAGE_ADD(netdev_batched, cnt).

I am not sure what do u mean, but we can get avg batch size by looking
at all packets recved and this count for given period of time.