On Tue, Mar 3, 2015 at 9:01 AM, Kavanagh, Mark B
<mark.b.kavan...@intel.com> wrote:
>> 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 <pshe...@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)?
>>
There should not be any performance impact due to this patch.
>> 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?
>
right. I will remove it.
>> +#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
>
There is assert for DPDK case.
>> + 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?
>
yes.
>> + 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?
>
Caller should make sure it does not 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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
