[ovs-dev] [PATCH 2/9] netdev: Send ofpuf directly to netdev.

Jarno Rajahalme jrajahalme at nicira.com
Tue Mar 18 23:04:19 UTC 2014


On Mar 18, 2014, at 1:53 PM, Pravin <pshelar at nicira.com> wrote:

> DPDK netdev need to access ofpbuf while sending buffer. Following
> patch changes netdev_send accordingly.

Would it still be possible to not transfer the responsibility of freeing the buffer? This relates to the comments I sent for patch 1.

> 
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> ---
> lib/dpif-netdev.c     |    9 +++++----
> lib/netdev-dummy.c    |    7 ++++++-
> lib/netdev-linux.c    |    9 ++++++++-
> lib/netdev-provider.h |   14 +++++++-------
> lib/netdev.c          |    4 ++--
> lib/netdev.h          |    2 +-
> 6 files changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d78fb25..440561e 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1806,7 +1806,7 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
>     case OVS_ACTION_ATTR_OUTPUT:
>         p = dp_netdev_lookup_port(aux->dp, u32_to_odp(nl_attr_get_u32(a)));
>         if (p) {
> -            netdev_send(p->netdev, packet);
> +            netdev_send(p->netdev, packet, may_steal);
>         }
>         break;
> 
> @@ -1817,6 +1817,10 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
> 
>         dp_netdev_output_userspace(aux->dp, packet, DPIF_UC_ACTION, aux->key,
>                                    userdata);
> +
> +        if (may_steal) {
> +            ofpbuf_delete(packet);
> +        }
>         break;
>     }
>     case OVS_ACTION_ATTR_PUSH_VLAN:
> @@ -1830,9 +1834,6 @@ dp_execute_cb(void *aux_, struct ofpbuf *packet,
>         OVS_NOT_REACHED();
>     }
> 
> -    if (may_steal) {
> -        ofpbuf_delete(packet);
> -    }
> }
> 
> static void
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 27487f9..7c73ca6 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -817,9 +817,11 @@ netdev_dummy_rx_drain(struct netdev_rx *rx_)
> }
> 
> static int
> -netdev_dummy_send(struct netdev *netdev, const void *buffer, size_t size)
> +netdev_dummy_send(struct netdev *netdev, struct ofpbuf *pkt, bool may_steal)
> {
>     struct netdev_dummy *dev = netdev_dummy_cast(netdev);
> +    const void *buffer = pkt->data;
> +    size_t size = pkt->size;
> 
>     if (size < ETH_HEADER_LEN) {
>         return EMSGSIZE;
> @@ -854,6 +856,9 @@ netdev_dummy_send(struct netdev *netdev, const void *buffer, size_t size)
>     }
> 
>     ovs_mutex_unlock(&dev->mutex);
> +    if (may_steal) {
> +        ofpbuf_delete(pkt);
> +    }
> 
>     return 0;
> }
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 574a572..a6e2217 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -1049,8 +1049,11 @@ netdev_linux_rx_drain(struct netdev_rx *rx_)
>  * The kernel maintains a packet transmission queue, so the caller is not
>  * expected to do additional queuing of packets. */
> static int
> -netdev_linux_send(struct netdev *netdev_, const void *data, size_t size)
> +netdev_linux_send(struct netdev *netdev_, struct ofpbuf *pkt, bool may_steal)
> {
> +    const void *data = pkt->data;
> +    size_t size = pkt->size;
> +
>     for (;;) {
>         ssize_t retval;
> 
> @@ -1122,6 +1125,10 @@ netdev_linux_send(struct netdev *netdev_, const void *data, size_t size)
>             return 0;
>         }
>     }
> +
> +    if (may_steal) {
> +        ofpbuf_delete(pkt);
> +    }
> }
> 
> /* Registers with the poll loop to wake up from the next call to poll_block()
> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h
> index 9bacaa0..90700db 100644
> --- a/lib/netdev-provider.h
> +++ b/lib/netdev-provider.h
> @@ -223,13 +223,13 @@ struct netdev_class {
>     const struct netdev_tunnel_config *
>         (*get_tunnel_config)(const struct netdev *netdev);
> 
> -    /* Sends the 'size'-byte packet in 'buffer' on 'netdev'.  Returns 0 if
> -     * successful, otherwise a positive errno value.  Returns EAGAIN without
> -     * blocking if the packet cannot be queued immediately.  Returns EMSGSIZE
> -     * if a partial packet was transmitted or if the packet is too big or too
> -     * small to transmit on the device.
> +    /* Sends the buffer->data of size buffer->sizefrom 'buffer' on 'netdev’.

Missing space.

> +     * Returns 0 if successful, otherwise a positive errno value.  Returns
> +     * EAGAIN without blocking if the packet cannot be queued immediately.
> +     * Returns EMSGSIZE if a partial packet was transmitted or if the packet
> +     * is too big or too small to transmit on the device.
>      *
> -     * The caller retains ownership of 'buffer' in all cases.
> +     * The caller retains ownership of 'buffer' if may_steal is true.

This looks like a reverse of your usage.

>      *
>      * The network device is expected to maintain a packet transmission queue,
>      * so that the caller does not ordinarily have to do additional queuing of
> @@ -241,7 +241,7 @@ struct netdev_class {
>      * network device from being usefully used by the netdev-based "userspace
>      * datapath".  It will also prevent the OVS implementation of bonding from
>      * working properly over 'netdev'.) */
> -    int (*send)(struct netdev *netdev, const void *buffer, size_t size);
> +    int (*send)(struct netdev *netdev, struct ofpbuf *buffer, bool may_steal);
> 
>     /* Registers with the poll loop to wake up from the next call to
>      * poll_block() when the packet transmission queue for 'netdev' has
> diff --git a/lib/netdev.c b/lib/netdev.c
> index a058742..295e121 100644
> --- a/lib/netdev.c
> +++ b/lib/netdev.c
> @@ -610,12 +610,12 @@ netdev_rx_drain(struct netdev_rx *rx)
>  * Some network devices may not implement support for this function.  In such
>  * cases this function will always return EOPNOTSUPP. */
> int
> -netdev_send(struct netdev *netdev, const struct ofpbuf *buffer)
> +netdev_send(struct netdev *netdev, struct ofpbuf *buffer, bool may_steal)
> {
>     int error;
> 
>     error = (netdev->netdev_class->send
> -             ? netdev->netdev_class->send(netdev, buffer->data, buffer->size)
> +             ? netdev->netdev_class->send(netdev, buffer, may_steal)
>              : EOPNOTSUPP);
>     if (!error) {
>         COVERAGE_INC(netdev_sent);
> diff --git a/lib/netdev.h b/lib/netdev.h
> index bfd8f91..2b4e2a9 100644
> --- a/lib/netdev.h
> +++ b/lib/netdev.h
> @@ -166,7 +166,7 @@ void netdev_rx_wait(struct netdev_rx *);
> int netdev_rx_drain(struct netdev_rx *);
> 
> /* Packet transmission. */
> -int netdev_send(struct netdev *, const struct ofpbuf *);
> +int netdev_send(struct netdev *, struct ofpbuf *, bool may_steal);
> void netdev_send_wait(struct netdev *);
> 
> /* Hardware address. */
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev




More information about the dev mailing list