[ovs-dev] [PATCH 3/4] dp-packet: Remove ofpbuf dependency.
Kavanagh, Mark B
mark.b.kavanagh at intel.com
Tue Mar 3 17:01:04 UTC 2015
> Currently dp-packet make use of ofpbuf for managing packet
> buffers. That complicates ofpbuf, by making dp-packet
> independent of ofpbuf both libraries can be optimized for
> their own use case.
> This avoids mapping operation between ofpbuf and dp_packet
> in datapath upcalls.
>
> Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
> ---
> lib/bfd.c | 23 +-
> lib/bfd.h | 6 +-
> lib/cfm.c | 12 +-
> lib/cfm.h | 6 +-
> lib/dp-packet.c | 556 +++++++++++++++++++++++++++++++++++++--
> lib/dp-packet.h | 442 ++++++++++++++++++++++++++++++--
> lib/dpif-netdev.c | 44 ++--
> lib/dpif-netdev.h | 8 +-
> lib/dpif-netlink.c | 20 +-
> lib/dpif.c | 31 +--
> lib/dpif.h | 11 +-
> lib/flow.c | 79 +++---
> lib/flow.h | 32 +--
> lib/lacp.c | 8 +-
> lib/lacp.h | 2 +-
> lib/learning-switch.c | 11 +-
> lib/netdev-bsd.c | 31 +--
> lib/netdev-dpdk.c | 11 +-
> lib/netdev-dummy.c | 94 ++++----
> lib/netdev-linux.c | 35 ++--
> lib/netdev-vport.c | 47 ++--
> lib/netdev.c | 4 +-
> lib/odp-execute.c | 105 ++++----
> lib/ofp-print.c | 22 +-
> lib/packets.c | 158 ++++++------
> lib/packets.h | 34 ++--
> lib/pcap-file.c | 48 ++--
> lib/pcap-file.h | 10 +-
> lib/rstp-common.h | 2 +-
> lib/rstp-state-machines.c | 15 +-
> lib/rstp.c | 3 +-
> lib/rstp.h | 4 +-
> lib/stp.c | 19 +-
> lib/stp.h | 4 +-
> ofproto/bond.c | 7 +-
> ofproto/bond.h | 2 +-
> ofproto/connmgr.c | 2 +-
> ofproto/connmgr.h | 2 +-
> ofproto/fail-open.c | 15 +-
> ofproto/ofproto-dpif-ipfix.c | 101 ++++----
> ofproto/ofproto-dpif-ipfix.h | 6 +-
> ofproto/ofproto-dpif-monitor.c | 19 +-
> ofproto/ofproto-dpif-sflow.c | 8 +-
> ofproto/ofproto-dpif-sflow.h | 2 +-
> ofproto/ofproto-dpif-upcall.c | 35 ++--
> ofproto/ofproto-dpif-xlate.c | 54 ++--
> ofproto/ofproto-dpif-xlate.h | 7 +-
> ofproto/ofproto-dpif.c | 91 ++++----
> ofproto/ofproto-dpif.h | 6 +-
> ofproto/ofproto-provider.h | 4 +-
> ofproto/ofproto.c | 19 +-
> ofproto/pktbuf.c | 16 +-
> ofproto/pktbuf.h | 4 +-
> tests/test-flows.c | 11 +-
> tests/test-rstp.c | 9 +-
> tests/test-stp.c | 9 +-
> utilities/ovs-ofctl.c | 42 ++--
> 57 files changed, 1639 insertions(+), 769 deletions(-)
General: what impacts (if any) to performance do these changes have? Are any figures available (e.g. for Phy-Phy use case)?
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 3db1d57..62ace8f 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -35,6 +35,7 @@
(snip)
> @@ -741,7 +742,7 @@ bfd_process_packet(struct bfd *bfd, const struct flow *flow,
> * If the Length field is greater than the payload of the encapsulating
> * protocol, the packet MUST be discarded.
> *
> - * Note that we make this check implicity. Above we use ofpbuf_at() to
> + * Note that we make this check implicity. Above we use dp_packet_at() to
Typo in comment: 'implicity' (sic)
(snip)
> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
> index d77f8e4..b90f678 100644
> --- a/lib/dp-packet.c
> +++ b/lib/dp-packet.c
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -15,57 +15,555 @@
> */
>
> #include <config.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include "dynamic-string.h"
> +#include "netdev-dpdk.h"
> #include "dp-packet.h"
> +#include "util.h"
>
(snip)
> +/* Frees memory that 'b' points to. */
> +void
> +dp_packet_uninit(struct dp_packet *b)
> +{
> + if (b) {
> + if (b->source == DPBUF_MALLOC) {
> + free(dp_packet_base(b));
> + } else if (b->source == DPBUF_DPDK) {
> +#ifdef DPDK_NETDEV
> + /* If this dp_packet was allocated by DPDK it must have been
> + * created as a dp_packet */
> + free_dpdk_buf((struct dp_packet*) b);
> +#else
> + ovs_assert(b->source != DPBUF_DPDK);
The else condition ensures that b->source is DPBUF_DPDK - how can this assert ever be true?
> +#endif
> + }
> + }
> +}
> +
>
> +/* Reallocates 'b' so that it has exactly 'new_headroom' and 'new_tailroom'
> + * bytes of headroom and tailroom, respectively. */
> +static void
> +dp_packet_resize__(struct dp_packet *b, size_t new_headroom, size_t new_tailroom)
> +{
> + void *new_base, *new_data;
> + size_t new_allocated;
> +
> + new_allocated = new_headroom + dp_packet_size(b) + new_tailroom;
> +
> + switch (b->source) {
> + case DPBUF_DPDK:
> + OVS_NOT_REACHED();
> +
> + case DPBUF_MALLOC:
> + if (new_headroom == dp_packet_headroom(b)) {
> + new_base = xrealloc(dp_packet_base(b), new_allocated);
> + } else {
> + new_base = xmalloc(new_allocated);
> + dp_packet_copy__(b, new_base, new_headroom, new_tailroom);
> + free(dp_packet_base(b));
> + }
> + break;
>
> - p->ofpbuf.frame = (char *) b->frame + data_delta;
> + case DPBUF_STACK:
> + OVS_NOT_REACHED();
> +
> + case DPBUF_STUB:
> + b->source = DPBUF_MALLOC;
> + new_base = xmalloc(new_allocated);
> + dp_packet_copy__(b, new_base, new_headroom, new_tailroom);
> + break;
> +
> + default:
> + OVS_NOT_REACHED();
> }
> - p->ofpbuf.l2_5_ofs = b->l2_5_ofs;
> - p->ofpbuf.l3_ofs = b->l3_ofs;
> - p->ofpbuf.l4_ofs = b->l4_ofs;
>
> + b->allocated = new_allocated;
> + dp_packet_set_base(b, new_base);
> +
> + new_data = (char *) new_base + new_headroom;
> + if (dp_packet_data(b) != new_data) {
> + if (b->frame) {
> + uintptr_t data_delta = (char *) new_data - (char *) dp_packet_data(b);
> +
General: You already know this, but just to state it explicitly: these changes won't work with DPDK versions > 1.7.1
> + b->frame = (char *) b->frame + data_delta;
> + }
> + dp_packet_set_data(b, new_data);
(snip)
> +/* Shifts all of the data within the allocated space in 'b' by 'delta' bytes.
> + * For example, a 'delta' of 1 would cause each byte of data to move one byte
> + * forward (from address 'p' to 'p+1'), and a 'delta' of -1 would cause each
> + * byte to move one byte backward (from 'p' to 'p-1'). */
> +void
> +dp_packet_shift(struct dp_packet *b, int delta)
> +{
> + ovs_assert(delta > 0 ? delta <= dp_packet_tailroom(b)
> + : delta < 0 ? -delta <= dp_packet_headroom(b)
> + : true);
> +
> + if (delta != 0) {
> + char *dst = (char *) dp_packet_data(b) + delta;
> + memmove(dst, dp_packet_data(b), dp_packet_size(b));
Presumably, it is the caller's responsibility to ensure that the memory at 'dst' is not in use?
> + dp_packet_set_data(b, dst);
> + }
> +}
> +
(snip)
> +
> +static inline void
> +dp_packet_adjust_layer_offset(uint16_t *offset, int increment)
> {
> - struct dp_packet *newp;
> + if (*offset != UINT16_MAX) {
> + *offset += increment;
Is a warning needed if offset wraps around?
(snip)
> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
> index 74abdec..e23660d 100644
> --- a/lib/dp-packet.h
> +++ b/lib/dp-packet.h
> @@ -1,5 +1,5 @@
> /*
> - * Copyright (c) 2014 Nicira, Inc.
> + * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -14,43 +14,451 @@
> * limitations under the License.
> */
>
> -#ifndef PACKET_DPIF_H
> -#define PACKET_DPIF_H 1
> +#ifndef DPBUF_H
> +#define DPBUF_H 1
>
> -#include "ofpbuf.h"
> +#include <stddef.h>
> +#include <stdint.h>
> +#include "list.h"
> +#include "packets.h"
> +#include "util.h"
> +#include "netdev-dpdk.h"
General: Rearrange includes alphabetically, if possible
>
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> -/* A packet received from a netdev and passed to a dpif. */
> +enum OVS_PACKED_ENUM dp_packet_source {
> + DPBUF_MALLOC, /* Obtained via malloc(). */
> + DPBUF_STACK, /* Un-movable stack space or static buffer. */
> + DPBUF_STUB, /* Starts on stack, may expand into heap. */
> + DPBUF_DPDK, /* buffer data is from DPDK allocated memory.
> + ref to build_dp_packet() in netdev-dpdk. */
Alignment is off on second line of comment above
More information about the dev
mailing list