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 <[email protected]> wrote: > On Tue, Jun 14, 2016 at 4:42 PM, William Tu <[email protected]> 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 <[email protected]> >> Cc: Pravin Shelar <[email protected]> >> --- >> 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; >> } >> _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
