On Fri, Mar 21, 2014 at 1:55 PM, Thomas Graf <tg...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
