[ovs-dev] [PATCHv7 2/2] ofp-actions: Add truncate action.

William Tu u9012063 at gmail.com
Sat Jun 18 16:34:01 UTC 2016


Hi Pravin,

Thanks for the feedback, I've submitted another version to address these.

Regards,
William

On Fri, Jun 17, 2016 at 10:56 AM, pravin shelar <pshelar at ovn.org> wrote:
> On Tue, Jun 14, 2016 at 4:42 PM, William Tu <u9012063 at gmail.com> wrote:
>> The patch adds a new action to support packet truncation.  The new action
>> is formatted as 'output(port=n,max_len=m)', as output to port n, with
>> packet size being MIN(original_size, m).
>>
>> One use case is to enable port mirroring to send smaller packets to the
>> destination port so that only useful packet information is mirrored/copied,
>> saving some performance overhead of copying entire packet payload.  Example
>> use case is below as well as shown in the testcases:
>>
>>     - Output to port 1 with max_len 100 bytes.
>>     - The output packet size on port 1 will be MIN(original_packet_size, 100).
>>     # ovs-ofctl add-flow br0 'actions=output(port=1,max_len=100)'
>>
>>     - The scope of max_len is limited to output action itself.  The following
>>       packet size of output:1 and output:2 will be intact.
>>     # ovs-ofctl add-flow br0 \
>>             'actions=output(port=1,max_len=100),output:1,output:2'
>>     - The Datapath actions shows:
>>     # Datapath actions: trunc(100),1,1,2
>>
>> Tested-at: https://travis-ci.org/williamtu/ovs-travis/builds/137660394
>> Signed-off-by: William Tu <u9012063 at gmail.com>
>> Cc: Pravin Shelar <pshelar at nicira.com>
>> ---
>>  include/openvswitch/ofp-actions.h |  10 +++
>>  lib/dp-packet.c                   |   1 +
>>  lib/dp-packet.h                   |  27 ++++++
>>  lib/dpif-netdev.c                 |  33 +++++++-
>>  lib/dpif.c                        |  23 ++++++
>>  lib/dpif.h                        |   1 +
>>  lib/netdev-bsd.c                  |   5 ++
>>  lib/netdev-dpdk.c                 |   6 ++
>>  lib/netdev-dummy.c                |   5 ++
>>  lib/netdev-linux.c                |   5 ++
>>  lib/netdev.c                      |   8 +-
>>  lib/odp-execute.c                 |  11 +++
>>  lib/odp-util.c                    |  23 ++++++
>>  lib/ofp-actions.c                 | 107 ++++++++++++++++++++++++
>>  ofproto/ofproto-dpif-sflow.c      |   1 +
>>  ofproto/ofproto-dpif-xlate.c      |  57 +++++++++++++
>>  ofproto/ofproto-dpif.c            |  56 +++++++++++++
>>  ofproto/ofproto-dpif.h            |   3 +
>>  tests/odp.at                      |   1 +
>>  tests/ofp-actions.at              |   3 +
>>  tests/ofproto-dpif.at             | 124 ++++++++++++++++++++++++++++
>>  tests/ovs-ofctl.at                |   8 ++
>>  tests/system-traffic.at           | 168 ++++++++++++++++++++++++++++++++++++++
>>  23 files changed, 683 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/openvswitch/ofp-actions.h b/include/openvswitch/ofp-actions.h
>> index 038ef87..51ffa71 100644
>> --- a/include/openvswitch/ofp-actions.h
>> +++ b/include/openvswitch/ofp-actions.h
>> @@ -108,6 +108,7 @@
>>      OFPACT(UNROLL_XLATE,    ofpact_unroll_xlate, ofpact, "unroll_xlate") \
>>      OFPACT(CT,              ofpact_conntrack,   ofpact, "ct")           \
>>      OFPACT(NAT,             ofpact_nat,         ofpact, "nat")          \
>> +    OFPACT(OUTPUT_TRUNC,    ofpact_output_trunc,ofpact, "output_trunc") \
>>                                                                          \
>>      /* Debugging actions.                                               \
>>       *                                                                  \
>> @@ -290,6 +291,15 @@ struct ofpact_output_reg {
>>      struct mf_subfield src;
>>  };
>>
>> +/* OFPACT_OUTPUT_TRUNC.
>> + *
>> + * Used for NXAST_OUTPUT_TRUNC. */
>> +struct ofpact_output_trunc {
>> +    struct ofpact ofpact;
>> +    ofp_port_t port;            /* Output port. */
>> +    uint32_t max_len;           /* Max send len. */
>> +};
>> +
>>  /* Bundle slave choice algorithm to apply.
>>   *
>>   * In the descriptions below, 'slaves' is the list of possible slaves in the
>> diff --git a/lib/dp-packet.c b/lib/dp-packet.c
>> index 0c85d50..4ee2f56 100644
>> --- a/lib/dp-packet.c
>> +++ b/lib/dp-packet.c
>> @@ -30,6 +30,7 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum dp_packet_source so
>>      dp_packet_reset_offsets(b);
>>      pkt_metadata_init(&b->md, 0);
>>      dp_packet_rss_invalidate(b);
>> +    dp_packet_reset_cutlen(b);
>>  }
>>
>>  static void
>> diff --git a/lib/dp-packet.h b/lib/dp-packet.h
>> index 118c84d..a4b153b 100644
>> --- a/lib/dp-packet.h
>> +++ b/lib/dp-packet.h
>> @@ -19,6 +19,7 @@
>>
>>  #include <stddef.h>
>>  #include <stdint.h>
>> +#include <linux/if_ether.h>
>>  #include "openvswitch/list.h"
>>  #include "packets.h"
>>  #include "util.h"
>> @@ -60,6 +61,7 @@ struct dp_packet {
>>                                      * or UINT16_MAX. */
>>      uint16_t l4_ofs;               /* Transport-level header offset,
>>                                        or UINT16_MAX. */
>> +    uint32_t cutlen;               /* length in bytes to cut from the end. */
>>      union {
>>          struct pkt_metadata md;
>>          uint64_t data[DP_PACKET_CONTEXT_SIZE / 8];
>> @@ -494,6 +496,31 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
>>  }
>>  #endif
>>
>> +static inline void
>> +dp_packet_reset_cutlen(struct dp_packet *b)
>> +{
>> +    b->cutlen = 0;
>> +}
>> +
>> +static inline uint32_t
>> +dp_packet_set_cutlen(struct dp_packet *b, uint32_t max_len)
>> +{
>> +    if (max_len < ETH_HLEN ||
>> +        max_len >= dp_packet_size(b)) {
>> +        b->cutlen = 0;
>> +    }
>> +    else {
>> +        b->cutlen = dp_packet_size(b) - max_len;
>> +    }
>> +    return b->cutlen;
>> +}
>> +
>> +static inline uint32_t
>> +dp_packet_get_cutlen(struct dp_packet *b)
>> +{
>> +    return b->cutlen;
>> +}
>> +
>>  static inline void *
>>  dp_packet_data(const struct dp_packet *b)
>>  {
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 8d39d9e..70d4cc2 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -4057,6 +4057,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>      case OVS_ACTION_ATTR_TUNNEL_PUSH:
>>          if (*depth < MAX_RECIRC_DEPTH) {
>>              struct dp_packet_batch tnl_pkt;
>> +            struct dp_packet **orig_packets = packets_->packets;
>>              int err;
>>
>>              if (!may_steal) {
>> @@ -4064,6 +4065,19 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>                  packets_ = &tnl_pkt;
>>              }
>>
>> +            for (int i = 0; i < packets_->count; i++) {
>> +                /* if may_steal, then opacket == packet. */
>> +                struct dp_packet *orig_packet = orig_packets[i];
>> +                struct dp_packet *packet = packets_->packets[i];
>> +                uint32_t cutlen = dp_packet_get_cutlen(orig_packet);
>> +
>> +                if (cutlen > 0) {
>> +                    dp_packet_set_size(packet,
>> +                            dp_packet_size(packet) - cutlen);
>> +                    dp_packet_reset_cutlen(orig_packet);
>> +                }
>> +            }
>> +
> Can you write function to trim batch of packets so that same function
> can be used in tunnel  push and pop action.
>>              err = push_tnl_action(pmd, a, packets_);
>>              if (!err) {
>>                  (*depth)++;
>> @@ -4076,6 +4090,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>
>>      case OVS_ACTION_ATTR_TUNNEL_POP:
>>          if (*depth < MAX_RECIRC_DEPTH) {
>> +            struct dp_packet **orig_packets = packets_->packets;
>>              odp_port_t portno = u32_to_odp(nl_attr_get_u32(a));
>>
>>              p = pmd_tx_port_cache_lookup(pmd, portno);
>> @@ -4084,8 +4099,21 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>                  int i;
>>
>>                  if (!may_steal) {
>> -                   dp_packet_batch_clone(&tnl_pkt, packets_);
>> -                   packets_ = &tnl_pkt;
>> +                    dp_packet_batch_clone(&tnl_pkt, packets_);
>> +                    packets_ = &tnl_pkt;
>> +                }
>> +
>> +                for (int i = 0; i < packets_->count; i++) {
>> +                    /* if may_steal, then opacket == packet. */
>> +                    struct dp_packet *orig_packet = orig_packets[i];
>> +                    struct dp_packet *packet = packets_->packets[i];
>> +                    uint32_t cutlen = dp_packet_get_cutlen(orig_packet);
>> +
>> +                    if (cutlen > 0) {
>> +                        dp_packet_set_size(packet,
>> +                                dp_packet_size(packet) - cutlen);
>> +                        dp_packet_reset_cutlen(orig_packet);
>> +                    }
>>                  }
>>
>>                  netdev_pop_header(p->netdev, packets_);
>> @@ -4170,6 +4198,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_,
>>      case OVS_ACTION_ATTR_SAMPLE:
>>      case OVS_ACTION_ATTR_HASH:
>>      case OVS_ACTION_ATTR_UNSPEC:
>> +    case OVS_ACTION_ATTR_TRUNC:
>>      case __OVS_ACTION_ATTR_MAX:
>>          OVS_NOT_REACHED();
>>      }
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index c4f24c7..92f37f8 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -1092,6 +1092,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>>      struct dpif_execute_helper_aux *aux = aux_;
>>      int type = nl_attr_type(action);
>>      struct dp_packet *packet = packets_->packets[0];
>> +    struct dp_packet *trunc_packet = NULL, *orig_packet;
>>
>>      ovs_assert(packets_->count == 1);
>>
>> @@ -1107,6 +1108,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>>          uint64_t stub[256 / 8];
>>          struct pkt_metadata *md = &packet->md;
>>          bool dst_set;
>> +        uint32_t cutlen = dp_packet_get_cutlen(packet);
>>
>>          dst_set = flow_tnl_dst_is_set(&md->tunnel);
>>          if (dst_set) {
>> @@ -1124,6 +1126,18 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>>              execute.actions_len = NLA_ALIGN(action->nla_len);
>>          }
>>
>> +        orig_packet = packet;
>> +
>> +        if (cutlen > 0) {
>> +            if (!may_steal) {
>> +               trunc_packet = dp_packet_clone(packet);
>> +               packet = trunc_packet;
>> +            }
>> +            /* Truncation applies to the clone packet or the original
>> +             * packet with may_steal == true. */
>> +            dp_packet_set_size(packet, dp_packet_size(orig_packet) - cutlen);
>> +        }
>> +
>>          execute.packet = packet;
>>          execute.flow = aux->flow;
>>          execute.needs_help = false;
>> @@ -1135,6 +1149,14 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>>          if (dst_set) {
>>              ofpbuf_uninit(&execute_actions);
>>          }
>> +
>> +        /* Reset the truncation state so next output action is intact. */
>> +        if (cutlen > 0) {
>> +            dp_packet_reset_cutlen(orig_packet);
>> +            if (!may_steal) {
>> +                dp_packet_delete(trunc_packet);
>> +            }
>> +        }
>>          break;
>>      }
> The packet trim operation is applied to CT and RECIRC action, kernel
> datapath does not do that. we should not have any inconstancy in
> truncate action over different datapath.
>
>
>>
>> @@ -1146,6 +1168,7 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_,
>>      case OVS_ACTION_ATTR_SET:
>>      case OVS_ACTION_ATTR_SET_MASKED:
>>      case OVS_ACTION_ATTR_SAMPLE:
>> +    case OVS_ACTION_ATTR_TRUNC:
>>      case OVS_ACTION_ATTR_UNSPEC:
>>      case __OVS_ACTION_ATTR_MAX:
>>          OVS_NOT_REACHED();
>> diff --git a/lib/dpif.h b/lib/dpif.h
>> index 6788301..981868c 100644
>> --- a/lib/dpif.h
>> +++ b/lib/dpif.h
>> @@ -784,6 +784,7 @@ struct dpif_upcall {
>>      size_t key_len;             /* Length of 'key' in bytes. */
>>      ovs_u128 ufid;              /* Unique flow identifier for 'key'. */
>>      struct nlattr *mru;         /* Maximum receive unit. */
>> +    struct nlattr *cutlen;      /* Number of bytes shrink from the end. */
>>
>>      /* DPIF_UC_ACTION only. */
>>      struct nlattr *userdata;    /* Argument to OVS_ACTION_ATTR_USERSPACE. */
>> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
>> index 43fa982..7fc0888 100644
>> --- a/lib/netdev-bsd.c
>> +++ b/lib/netdev-bsd.c
>> @@ -698,6 +698,11 @@ netdev_bsd_send(struct netdev *netdev_, int qid OVS_UNUSED,
>>      for (i = 0; i < cnt; i++) {
>>          const void *data = dp_packet_data(pkts[i]);
>>          size_t size = dp_packet_size(pkts[i]);
>> +        uint32_t cutlen = dp_packet_get_cutlen(pkts[i]);
>> +
>> +        if (cutlen > 0) {
>> +            size -= cutlen;
>> +        }
>>
> We need to check for minimum packet length as we have done in kernel
> datapath. Can you add API to read truncated packet length to avoid
> duplicate code?
>
>>          while (!error) {
>>              ssize_t retval;
>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>> index 19d355f..a66d4ff 100644
>> --- a/lib/netdev-dpdk.c
>> +++ b/lib/netdev-dpdk.c
>> @@ -1602,6 +1602,12 @@ netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
>>
>>          for (i = 0; i < cnt; i++) {
>>              int size = dp_packet_size(pkts[i]);
>> +            uint32_t cutlen = dp_packet_get_cutlen(pkts[i]);
>> +
>> +            if (cutlen > 0) {
>> +                size -= cutlen;
>> +                dp_packet_set_size(pkts[i], size);
>> +            }
>>
>>              if (OVS_UNLIKELY(size > dev->max_packet_len)) {
>>                  if (next_tx_idx != i) {
>> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
>> index aa244b6..cd90c33 100644
>> --- a/lib/netdev-dummy.c
>> +++ b/lib/netdev-dummy.c
>> @@ -1043,6 +1043,11 @@ netdev_dummy_send(struct netdev *netdev, int qid OVS_UNUSED,
>>      for (i = 0; i < cnt; i++) {
>>          const void *buffer = dp_packet_data(pkts[i]);
>>          size_t size = dp_packet_size(pkts[i]);
>> +        uint32_t cutlen = dp_packet_get_cutlen(pkts[i]);
>> +
>> +        if (cutlen > 0) {
>> +            size -= cutlen;
>> +        }
>>
>>          if (size < ETH_HEADER_LEN) {
>>              error = EMSGSIZE;
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index 82813ba..082ca06 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -1169,6 +1169,11 @@ netdev_linux_send(struct netdev *netdev_, int qid OVS_UNUSED,
>>          const void *data = dp_packet_data(pkts[i]);
>>          size_t size = dp_packet_size(pkts[i]);
>>          ssize_t retval;
>> +        uint32_t cutlen = dp_packet_get_cutlen(pkts[i]);
>> +
>> +        if (cutlen > 0) {
>> +            size -= cutlen;
>> +        }
>>
>>          if (!is_tap_netdev(netdev_)) {
>>              /* Use our AF_PACKET socket to send to this device. */
>> diff --git a/lib/netdev.c b/lib/netdev.c
>> index 4be806d..f285ddc 100644
>> --- a/lib/netdev.c
>> +++ b/lib/netdev.c
>> @@ -681,7 +681,7 @@ netdev_set_tx_multiq(struct netdev *netdev, unsigned int n_txq)
>>      return error;
>>  }
>>
>> -/* Sends 'buffers' on 'netdev'.  Returns 0 if successful (for every packet),
>> +/* Sends 'batch' on 'netdev'.  Returns 0 if successful (for every packet),
>>   * otherwise a positive errno value.  Returns EAGAIN without blocking if
>>   * at least one the packets cannot be queued immediately.  Returns EMSGSIZE
>>   * if a partial packet was transmitted or if a packet is too big or too small
>> @@ -717,6 +717,12 @@ netdev_send(struct netdev *netdev, int qid, struct dp_packet_batch *batch,
>>      if (!error) {
>>          COVERAGE_INC(netdev_sent);
>>      }
>> +
>> +    if (!may_steal) {
>> +        for (int i = 0; i < batch->count; i++) {
>> +            dp_packet_reset_cutlen(batch->packets[i]);
>> +        }
>> +    }
>>      return error;
>>  }
>>



More information about the dev mailing list