| Message ID | 162134902591.762107.15543938565196336653.stgit@wsfd-netdev64.ntdv.lab.eng.bos.redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v4] datapath: Add a new action dec_ttl | expand |
On 5/18/21 4:44 PM, Eelco Chaudron wrote: > Add support for the dec_ttl action. Instead of programming the datapath with > a flow that matches the packet TTL and an IP set, use a single dec_ttl action. > > The old behavior is kept if the new action is not supported by the datapath. > > # ovs-ofctl dump-flows br0 > cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip actions=dec_ttl,NORMAL > cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, actions=NORMAL > > # ping -c1 -t 20 192.168.0.2 > PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. > IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), length 84) > 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64 > > Linux netlink datapath support depends on upstream Linux commit: > 744676e77720 ("openvswitch: add TTL decrement action") > > > Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been > defined, and to make sure the IDs are in sync, it had to be added to the > OVS source tree. This required some additional case statements, which > should be revisited once the OVS implementation is added. > > Co-authored-by: Bindiya Kurle <bindiyakurle@gmail.com> > Co-authored-by: Matteo Croce <mcroce@linux.microsoft.com> > Signed-off-by: Eelco Chaudron <echaudro@redhat.com> > Signed-off-by: Bindiya Kurle <bindiyakurle@gmail.com> > Signed-off-by: Matteo Croce <mcroce@linux.microsoft.com> > Acked-by: Bindiya Kurle <bindiyakurle@gmail.com> > > --- > v2: - Used definition instead of numeric value in format_dec_ttl_action() > - Changed format from "dec_ttl(ttl<=1(<actions>)) to > "dec_ttl(le_1(<actions>))" to be more in line with the check_pkt_len action. > - Cleaned up format_dec_ttl_action() > v3: > - Fixed parsing of "dec_ttl()" action for adding a dp flow. > - Changed implementation to use the fixed kernel mod implementation > https://marc.info/?l=linux-netdev&m=160577671609295&w=2 > - Removed introduced force_last flag from odp_execute_actions > - For now, do not use this new attribute if HW offload is supported, as > it's causing a performance regression due to HW offload not being > supported. I will fix this in a separate patch. > - Added datapath test case for dec_ttl action. > v4: > - Fixed some style issues reported by Ilya. > - Changed OVS_NOT_REACHED() to an ovs_assert(), so information gets logged > on error. > - Use batch helper functions rather than access batch->count directly. > - Fixed memory leak when dec_ttl was the last action in the chain. > - Fixed do_xlate_actions() return if for exception in backward > compatibility mode. > - Added actions parsing test to odp.at > > datapath/linux/compat/include/linux/openvswitch.h | 10 ++ > lib/dpif-netdev.c | 2 > lib/dpif.c | 2 > lib/odp-execute.c | 88 +++++++++++++++++++++ > lib/odp-util.c | 45 +++++++++++ > lib/packets.h | 13 +++ > ofproto/ofproto-dpif-ipfix.c | 2 > ofproto/ofproto-dpif-sflow.c | 2 > ofproto/ofproto-dpif-xlate.c | 60 +++++++++++--- > ofproto/ofproto-dpif.c | 40 ++++++++++ > ofproto/ofproto-dpif.h | 6 + > tests/odp.at | 1 > tests/system-traffic.at | 28 +++++++ > 13 files changed, 282 insertions(+), 17 deletions(-) > > diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h > index 875de2025..a5357024d 100644 > --- a/datapath/linux/compat/include/linux/openvswitch.h > +++ b/datapath/linux/compat/include/linux/openvswitch.h > @@ -1030,6 +1030,8 @@ enum ovs_action_attr { > OVS_ACTION_ATTR_METER, /* u32 meter number. */ > OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ > OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ > + OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ > + OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ > > #ifndef __KERNEL__ > OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ > @@ -1133,4 +1135,12 @@ struct ovs_zone_limit { > * keys. False otherwise. > */ > > +enum ovs_dec_ttl_attr { > + OVS_DEC_TTL_ATTR_UNSPEC, > + OVS_DEC_TTL_ATTR_ACTION, /* Nested struct nlattr */ > + __OVS_DEC_TTL_ATTR_MAX > +}; > + > +#define OVS_DEC_TTL_ATTR_MAX (__OVS_DEC_TTL_ATTR_MAX - 1) > + > #endif /* _LINUX_OPENVSWITCH_H */ > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 251788b04..c6032e013 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -8046,6 +8046,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, > case OVS_ACTION_ATTR_CT_CLEAR: > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_DEC_TTL: > + case OVS_ACTION_ATTR_ADD_MPLS: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/dpif.c b/lib/dpif.c > index 26e8bfb7d..a25d8e9ef 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1274,6 +1274,8 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, > case OVS_ACTION_ATTR_UNSPEC: > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/odp-execute.c b/lib/odp-execute.c > index 6eeda2a61..ff2921045 100644 > --- a/lib/odp-execute.c > +++ b/lib/odp-execute.c > @@ -719,6 +719,55 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, > nl_attr_get_size(subactions), dp_execute_action); > } > > +static bool > +execute_dec_ttl(struct dp_packet *packet) > +{ > + struct eth_header *eth = dp_packet_eth(packet); > + > + if (dl_type_is_ipv4(eth->eth_type)) { > + struct ip_header *nh = dp_packet_l3(packet); > + uint8_t old_ttl = nh->ip_ttl; > + > + if (old_ttl <= 1) { > + return true; > + } > + > + nh->ip_ttl--; > + nh->ip_csum = recalc_csum16(nh->ip_csum, htons(old_ttl << 8), > + htons(nh->ip_ttl << 8)); > + } else if (dl_type_is_ipv6(eth->eth_type)) { > + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet); > + > + if (nh->ip6_hlim <= 1) { > + return true; > + } > + > + nh->ip6_hlim--; > + } > + > + return false; > +} > + > +static void > +odp_dec_ttl_exception_handler(void *dp, struct dp_packet_batch *batch, > + const struct nlattr *action, > + odp_execute_cb dp_execute_action) > +{ > + static const struct nl_policy dec_ttl_policy[] = { > + [OVS_DEC_TTL_ATTR_ACTION] = { .type = NL_A_NESTED }, > + }; > + struct nlattr *attrs[ARRAY_SIZE(dec_ttl_policy)]; > + > + ovs_assert(nl_parse_nested(action, dec_ttl_policy, attrs, > + ARRAY_SIZE(attrs)) > + && attrs[OVS_DEC_TTL_ATTR_ACTION]); > + > + odp_execute_actions(dp, batch, true, > + nl_attr_get(attrs[OVS_DEC_TTL_ATTR_ACTION]), > + nl_attr_get_size(attrs[OVS_DEC_TTL_ATTR_ACTION]), > + dp_execute_action); > +} > + > static void > odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, > const struct nlattr *actions, > @@ -734,7 +783,7 @@ odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, > dp_packet_batch_clone(&clone_pkt_batch, batch); > dp_packet_batch_reset_cutlen(batch); > odp_execute_actions(dp, &clone_pkt_batch, true, nl_attr_get(actions), > - nl_attr_get_size(actions), dp_execute_action); > + nl_attr_get_size(actions), dp_execute_action); > } > else { > odp_execute_actions(dp, batch, true, nl_attr_get(actions), > @@ -820,6 +869,8 @@ requires_datapath_assistance(const struct nlattr *a) > case OVS_ACTION_ATTR_CT_CLEAR: > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > return false; > > case OVS_ACTION_ATTR_UNSPEC: > @@ -979,6 +1030,40 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > } > break; > > + case OVS_ACTION_ATTR_DEC_TTL: { > + const size_t cnt = dp_packet_batch_size(batch); > + struct dp_packet_batch invalid_ttl; > + size_t i; > + > + /* Make batch for invalid ttl packets. */ > + dp_packet_batch_init(&invalid_ttl); > + invalid_ttl.trunc = batch->trunc; > + invalid_ttl.do_not_steal = batch->do_not_steal; > + > + /* Add packets with ttl <=1 to the invalid_ttl batch > + * and remove it from the batch. */ > + DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, batch) { > + if (execute_dec_ttl(packet)) { > + dp_packet_batch_add(&invalid_ttl, packet); > + } else { > + dp_packet_batch_refill(batch, packet, i); > + } > + } > + > + /* Execute action on packets with ttl <= 1. */ > + if (!dp_packet_batch_is_empty(&invalid_ttl)) { > + odp_dec_ttl_exception_handler(dp, &invalid_ttl, a, > + dp_execute_action); > + } > + > + if (dp_packet_batch_is_empty(batch)) { > + /* The entire batch was handled as an exception, so we can > + * stop processing actions. */ > + return; > + } > + break; > + } > + > case OVS_ACTION_ATTR_TRUNC: { > const struct ovs_action_trunc *trunc = > nl_attr_get_unspec(a, sizeof *trunc); > @@ -1077,6 +1162,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, > case OVS_ACTION_ATTR_RECIRC: > case OVS_ACTION_ATTR_CT: > case OVS_ACTION_ATTR_UNSPEC: > + case OVS_ACTION_ATTR_ADD_MPLS: > case __OVS_ACTION_ATTR_MAX: > OVS_NOT_REACHED(); > } > diff --git a/lib/odp-util.c b/lib/odp-util.c > index e1199d1da..922104e72 100644 > --- a/lib/odp-util.c > +++ b/lib/odp-util.c > @@ -143,8 +143,10 @@ odp_action_len(uint16_t type) > case OVS_ACTION_ATTR_POP_NSH: return 0; > case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; > case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); > + case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; > > case OVS_ACTION_ATTR_UNSPEC: > + case OVS_ACTION_ATTR_ADD_MPLS: > case __OVS_ACTION_ATTR_MAX: > return ATTR_LEN_INVALID; > } > @@ -1110,6 +1112,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr, > ds_put_cstr(ds, "))"); > } > > +static void > +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr, > + const struct hmap *portno_names) > +{ > + const struct nlattr *a; > + unsigned int left; > + > + ds_put_cstr(ds,"dec_ttl(le_1("); > + NL_ATTR_FOR_EACH (a, left, > + nl_attr_get(attr), nl_attr_get_size(attr)) { > + if (nl_attr_type(a) == OVS_DEC_TTL_ATTR_ACTION) { > + format_odp_actions(ds, nl_attr_get(a), > + nl_attr_get_size(a), portno_names); > + break; > + } > + } > + ds_put_format(ds, "))"); > +} > + > static void > format_odp_action(struct ds *ds, const struct nlattr *a, > const struct hmap *portno_names) > @@ -1257,7 +1278,11 @@ format_odp_action(struct ds *ds, const struct nlattr *a, > case OVS_ACTION_ATTR_DROP: > ds_put_cstr(ds, "drop"); > break; > + case OVS_ACTION_ATTR_DEC_TTL: > + format_dec_ttl_action(ds, a, portno_names); > + break; > case OVS_ACTION_ATTR_UNSPEC: > + case OVS_ACTION_ATTR_ADD_MPLS: > case __OVS_ACTION_ATTR_MAX: > default: > format_generic_odp_action(ds, a); > @@ -2505,6 +2530,26 @@ parse_odp_action__(struct parse_odp_context *context, const char *s, > return n + 1; > } > } > + { > + if (!strncmp(s, "dec_ttl(le_1(", 13)) { > + size_t actions_ofs, ofs; > + int n = 13; > + > + ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_DEC_TTL); > + actions_ofs = nl_msg_start_nested(actions, > + OVS_DEC_TTL_ATTR_ACTION); > + > + int retval = parse_action_list(context, s + n, actions); > + if (retval < 0) { > + return retval; > + } > + > + n += retval; > + nl_msg_end_nested(actions, actions_ofs); > + nl_msg_end_nested(actions, ofs); > + return n + 2; > + } > + } > > { > if (!strncmp(s, "push_nsh(", 9)) { > diff --git a/lib/packets.h b/lib/packets.h > index 481bc22fa..b7fa7b121 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -1211,10 +1211,19 @@ bool in6_is_lla(struct in6_addr *addr); > void ipv6_multicast_to_ethernet(struct eth_addr *eth, > const struct in6_addr *ip6); > > +static inline bool dl_type_is_ipv4(ovs_be16 dl_type) > +{ > + return dl_type == htons(ETH_TYPE_IP); > +} > + > +static inline bool dl_type_is_ipv6(ovs_be16 dl_type) > +{ > + return dl_type == htons(ETH_TYPE_IPV6); > +} > + > static inline bool dl_type_is_ip_any(ovs_be16 dl_type) > { > - return dl_type == htons(ETH_TYPE_IP) > - || dl_type == htons(ETH_TYPE_IPV6); > + return dl_type_is_ipv4(dl_type) || dl_type_is_ipv6(dl_type); > } > > /* Tunnel header */ > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c > index 796eb6f88..1c8e7ae26 100644 > --- a/ofproto/ofproto-dpif-ipfix.c > +++ b/ofproto/ofproto-dpif-ipfix.c > @@ -3018,6 +3018,8 @@ dpif_ipfix_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_UNSPEC: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_ADD_MPLS: > + case OVS_ACTION_ATTR_DEC_TTL: > case __OVS_ACTION_ATTR_MAX: > default: > break; > diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c > index 864c136b5..83989fb90 100644 > --- a/ofproto/ofproto-dpif-sflow.c > +++ b/ofproto/ofproto-dpif-sflow.c > @@ -1178,6 +1178,7 @@ dpif_sflow_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_CT_CLEAR: > case OVS_ACTION_ATTR_METER: > case OVS_ACTION_ATTR_LB_OUTPUT: > + case OVS_ACTION_ATTR_DEC_TTL: > break; > > case OVS_ACTION_ATTR_SET_MASKED: > @@ -1226,6 +1227,7 @@ dpif_sflow_read_actions(const struct flow *flow, > case OVS_ACTION_ATTR_UNSPEC: > case OVS_ACTION_ATTR_CHECK_PKT_LEN: > case OVS_ACTION_ATTR_DROP: > + case OVS_ACTION_ATTR_ADD_MPLS: > case __OVS_ACTION_ATTR_MAX: > default: > break; > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c > index 7108c8a30..3cac92940 100644 > --- a/ofproto/ofproto-dpif-xlate.c > +++ b/ofproto/ofproto-dpif-xlate.c > @@ -40,6 +40,7 @@ > #include "mac-learning.h" > #include "mcast-snooping.h" > #include "multipath.h" > +#include "netdev-offload.h" > #include "netdev-vport.h" > #include "netlink.h" > #include "nx-match.h" > @@ -4840,9 +4841,12 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, > enum ofp_packet_in_reason reason, > uint16_t controller_id, > uint32_t provider_meter_id, > - const uint8_t *userdata, size_t userdata_len) > + const uint8_t *userdata, size_t userdata_len, > + bool commit) > { > - xlate_commit_actions(ctx); > + if (commit) { > + xlate_commit_actions(ctx); > + } > > /* A packet sent by an action in a table-miss rule is considered an > * explicit table miss. OpenFlow before 1.3 doesn't have that concept so > @@ -5075,10 +5079,6 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) > { > struct flow *flow = &ctx->xin->flow; > > - if (!is_ip_any(flow)) { > - return false; > - } > - > ctx->wc->masks.nw_ttl = 0xff; > if (flow->nw_ttl > 1) { > flow->nw_ttl--; > @@ -5088,7 +5088,8 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) > > for (i = 0; i < ids->n_controllers; i++) { > xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, > - ids->cnt_ids[i], UINT32_MAX, NULL, 0); > + ids->cnt_ids[i], UINT32_MAX, NULL, 0, > + true); > } > > /* Stop processing for current table. */ > @@ -5129,7 +5130,7 @@ compose_dec_nsh_ttl_action(struct xlate_ctx *ctx) > return false; > } else { > xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, > - 0, UINT32_MAX, NULL, 0); > + 0, UINT32_MAX, NULL, 0, true); > } > } > > @@ -5162,7 +5163,7 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx) > return false; > } else { > xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0, > - UINT32_MAX, NULL, 0); > + UINT32_MAX, NULL, 0, true); > } > } > > @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, > ds_destroy(&s); > } > > +/* New handling for dec_ttl action. */ 'New handling' makes sense in a patch, but doesn't while reading the code. > +static bool > +xlate_dec_ttl_action(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) > +{ > + struct flow *flow = &ctx->xin->flow; > + size_t offset, offset_actions; > + size_t i; > + > + if (!is_ip_any(flow)) { > + return false; > + } > + > + if (!ctx->xbridge->support.dec_ttl_action > + || netdev_is_flow_api_enabled()) { > + return compose_dec_ttl(ctx, ids); > + } > + > + xlate_commit_actions(ctx); > + offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_DEC_TTL); > + offset_actions = nl_msg_start_nested(ctx->odp_actions, > + OVS_DEC_TTL_ATTR_ACTION); > + > + for (i = 0; i < ids->n_controllers; i++) { > + xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, > + ids->cnt_ids[i], UINT32_MAX, NULL, 0, false); > + } > + > + nl_msg_end_nested(ctx->odp_actions, offset_actions); > + nl_msg_end_nested(ctx->odp_actions, offset); > + > + xlate_commit_actions(ctx); > + return false; > +} > + > /* Emits an action that outputs to 'port', within 'ctx'. > * > * 'controller_len' affects only packets sent to an OpenFlow controller. It > @@ -5236,7 +5271,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port, > : group_bucket_action ? OFPR_GROUP > : ctx->in_action_set ? OFPR_ACTION_SET > : OFPR_ACTION), > - 0, UINT32_MAX, NULL, 0); > + 0, UINT32_MAX, NULL, 0, true); > break; > case OFPP_NONE: > break; > @@ -6797,7 +6832,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, > controller->controller_id, > controller->provider_meter_id, > controller->userdata, > - controller->userdata_len); > + controller->userdata_len, true); > } > break; > > @@ -7005,8 +7040,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, > break; > > case OFPACT_DEC_TTL: > - wc->masks.nw_ttl = 0xff; > - if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) { > + if (xlate_dec_ttl_action(ctx, ofpact_get_DEC_TTL(a))) { > return; > } > break; > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index fd0b2fdea..49e026359 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -1186,6 +1186,45 @@ check_trunc_action(struct dpif_backer *backer) > return !error; > } > > +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via > + * OVS_ACTION_DEC_TTL. */ > +static bool > +check_dec_ttl_action(struct dpif *dpif) > +{ > + struct odputil_keybuf keybuf; > + struct flow flow = { 0 }; It's probbaly better to just memset it as in other similar functions to avoid compiler's complains. > + struct ofpbuf actions; > + uint32_t actbuf[4]; > + struct ofpbuf key; > + bool supported; > + size_t start; > + > + struct odp_flow_key_parms odp_parms = { > + .flow = &flow, > + .probe = true, > + }; > + > + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); > + odp_flow_key_from_flow(&odp_parms, &key); > + > + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); > + start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_DEC_TTL); > + nl_msg_put_flag(&actions, OVS_DEC_TTL_ATTR_ACTION); > + nl_msg_end_nested(&actions, start); > + > + supported = dpif_probe_feature(dpif, "dec_ttl", &key, &actions, NULL); > + > + if (supported) { > + VLOG_INFO("%s: Datapath supports dec_ttl action", > + dpif_name(dpif)); > + } else { > + VLOG_INFO("%s: Datapath does not support dec_ttl action", > + dpif_name(dpif)); > + } > + > + return supported; > +} > + > /* Tests whether 'backer''s datapath supports the clone action > * OVS_ACTION_ATTR_CLONE. */ > static bool > @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) > dpif_supports_explicit_drop_action(backer->dpif); > backer->rt_support.lb_output_action= > dpif_supports_lb_output_action(backer->dpif); > + backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif); During discussions about all-zero SNAT feature support I remembered that we have a 'capabilities' table that should reflect all the datapath supported fetures. So, this should be added there as well. And documented in vswitchd/vswitch.xml. > > /* Flow fields. */ > backer->rt_support.odp.ct_state = check_ct_state(backer); > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index b41c3d82a..f4d1152bc 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -204,7 +204,11 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, > DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ > \ > /* True if the datapath supports balance_tcp optimization */ \ > - DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode") > + DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\ > + \ > + /* True if the datapath supports dec_ttl action. */ \ > + DPIF_SUPPORT_FIELD(bool, dec_ttl_action, "Decrement TTL action") > + > > > /* Stores the various features which the corresponding backer supports. */ > diff --git a/tests/odp.at b/tests/odp.at > index b762ebb2b..24946bec4 100644 > --- a/tests/odp.at > +++ b/tests/odp.at > @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) > check_pkt_len(size=200,gt(ct(nat)),le(drop)) > check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) > lb_output(1) > +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) Maybe it will make sense to also add a very simple variant of this action, e.g. dec_ttl(le_1(drop)). > ]) > AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], > [`cat actions.txt` > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index fb5b9a36d..ef9b555b6 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -1400,6 +1400,34 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl > OVS_TRAFFIC_VSWITCHD_STOP > AT_CLEANUP > > +AT_SETUP([datapath - dec_ttl action]) > +OVS_TRAFFIC_VSWITCHD_START() > + > +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) > + > +ADD_NAMESPACES(at_ns0, at_ns1) > + > +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") > +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") > + > +AT_CHECK([ovs-vsctl -- set interface ovs-p0 ofport_request=1 \ > + -- set interface ovs-p1 ofport_request=2]) > + > +AT_CHECK([ovs-ofctl del-flows br0]) > +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=dec_ttl,2]) > +AT_CHECK([ovs-ofctl add-flow br0 in_port=2,action=dec_ttl,1]) > + > +NS_CHECK_EXEC([at_ns0], [ping -c 1 -w 2 10.1.1.2 | grep -o -E 'ttl=[[[:digit:]]]+'], [0], [dnl > +ttl=63 > +]) > + > +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -t 1 10.1.1.2 | FORMAT_PING], [0], [dnl > +1 packets transmitted, 0 received, 100% packet loss, time 0ms > +]) > + > +OVS_TRAFFIC_VSWITCHD_STOP > +AT_CLEANUP > + > AT_BANNER([conntrack]) > > AT_SETUP([conntrack - controller]) >
On 18 May 2021, at 20:15, Ilya Maximets wrote: > On 5/18/21 4:44 PM, Eelco Chaudron wrote: >> Add support for the dec_ttl action. Instead of programming the datapath with >> a flow that matches the packet TTL and an IP set, use a single dec_ttl action. >> >> The old behavior is kept if the new action is not supported by the datapath. >> >> # ovs-ofctl dump-flows br0 >> cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip actions=dec_ttl,NORMAL >> cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, actions=NORMAL >> >> # ping -c1 -t 20 192.168.0.2 >> PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. >> IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), length 84) >> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64 >> >> Linux netlink datapath support depends on upstream Linux commit: >> 744676e77720 ("openvswitch: add TTL decrement action") >> >> >> Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been >> defined, and to make sure the IDs are in sync, it had to be added to the >> OVS source tree. This required some additional case statements, which >> should be revisited once the OVS implementation is added. <SNIP> >> @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, >> ds_destroy(&s); >> } >> >> +/* New handling for dec_ttl action. */ > > 'New handling' makes sense in a patch, but doesn't while reading the code. Yes, I will remove this comment altogether as it makes no sense. <SNIP> >> >> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via >> + * OVS_ACTION_DEC_TTL. */ >> +static bool >> +check_dec_ttl_action(struct dpif *dpif) >> +{ >> + struct odputil_keybuf keybuf; >> + struct flow flow = { 0 }; > > It's probbaly better to just memset it as in other similar functions > to avoid compiler's complains. ACK, will use a memset here. <SNIP> >> /* Tests whether 'backer''s datapath supports the clone action >> * OVS_ACTION_ATTR_CLONE. */ >> static bool >> @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) >> dpif_supports_explicit_drop_action(backer->dpif); >> backer->rt_support.lb_output_action= >> dpif_supports_lb_output_action(backer->dpif); >> + backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif); > > During discussions about all-zero SNAT feature support I remembered that > we have a 'capabilities' table that should reflect all the datapath > supported fetures. So, this should be added there as well. And documented > in vswitchd/vswitch.xml. ACK, will add. <SNIP> >> >> /* Stores the various features which the corresponding backer supports. */ >> diff --git a/tests/odp.at b/tests/odp.at >> index b762ebb2b..24946bec4 100644 >> --- a/tests/odp.at >> +++ b/tests/odp.at >> @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) >> check_pkt_len(size=200,gt(ct(nat)),le(drop)) >> check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) >> lb_output(1) >> +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) > > Maybe it will make sense to also add a very simple variant of this action, > e.g. dec_ttl(le_1(drop)). Added, drop and it resulted in an issue (fixed). Doing some final tests and will sent out a V5 soon! //Eelco
On Wed, May 26, 2021 at 2:34 PM Eelco Chaudron <echaudro@redhat.com> wrote: > On 18 May 2021, at 20:15, Ilya Maximets wrote: > >> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via > >> + * OVS_ACTION_DEC_TTL. */ > >> +static bool > >> +check_dec_ttl_action(struct dpif *dpif) > >> +{ > >> + struct odputil_keybuf keybuf; > >> + struct flow flow = { 0 }; > > > > It's probbaly better to just memset it as in other similar functions > > to avoid compiler's complains. > > ACK, will use a memset here. > Which complaint? Memset is a bit slower because it clears also the struct padding.
On 5/26/21 2:46 PM, Matteo Croce wrote: > On Wed, May 26, 2021 at 2:34 PM Eelco Chaudron <echaudro@redhat.com> wrote: >> On 18 May 2021, at 20:15, Ilya Maximets wrote: >>>> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via >>>> + * OVS_ACTION_DEC_TTL. */ >>>> +static bool >>>> +check_dec_ttl_action(struct dpif *dpif) >>>> +{ >>>> + struct odputil_keybuf keybuf; >>>> + struct flow flow = { 0 }; >>> >>> It's probbaly better to just memset it as in other similar functions >>> to avoid compiler's complains. >> >> ACK, will use a memset here. >> > > Which complaint? Memset is a bit slower because it clears also the > struct padding. > At least, ovsrobot failed to build this patch: https://mail.openvswitch.org/pipermail/ovs-dev/2021-May/383122.html Best regards, Ilya Maximets.
On 26 May 2021, at 14:46, Matteo Croce wrote: > On Wed, May 26, 2021 at 2:34 PM Eelco Chaudron <echaudro@redhat.com> wrote: >> On 18 May 2021, at 20:15, Ilya Maximets wrote: >>>> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via >>>> + * OVS_ACTION_DEC_TTL. */ >>>> +static bool >>>> +check_dec_ttl_action(struct dpif *dpif) >>>> +{ >>>> + struct odputil_keybuf keybuf; >>>> + struct flow flow = { 0 }; >>> >>> It's probbaly better to just memset it as in other similar functions >>> to avoid compiler's complains. >> >> ACK, will use a memset here. >> > > Which complaint? Memset is a bit slower because it clears also the > struct padding. See the robot’s responses to this patchset: ofproto/ofproto-dpif.c:1195:12: error: missing braces around initializer [-Werror=missing-braces] struct flow flow = { 0 }; But I guess memset is ok, as other similar functions use it. > > -- > per aspera ad upstream
On Wed, May 26, 2021 at 2:50 PM Eelco Chaudron <echaudro@redhat.com> wrote: > > > > On 26 May 2021, at 14:46, Matteo Croce wrote: > > > On Wed, May 26, 2021 at 2:34 PM Eelco Chaudron <echaudro@redhat.com> wrote: > >> On 18 May 2021, at 20:15, Ilya Maximets wrote: > >>>> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via > >>>> + * OVS_ACTION_DEC_TTL. */ > >>>> +static bool > >>>> +check_dec_ttl_action(struct dpif *dpif) > >>>> +{ > >>>> + struct odputil_keybuf keybuf; > >>>> + struct flow flow = { 0 }; > >>> > >>> It's probbaly better to just memset it as in other similar functions > >>> to avoid compiler's complains. > >> > >> ACK, will use a memset here. > >> > > > > Which complaint? Memset is a bit slower because it clears also the > > struct padding. > > See the robot’s responses to this patchset: > > ofproto/ofproto-dpif.c:1195:12: error: missing braces around initializer [-Werror=missing-braces] > struct flow flow = { 0 }; > > But I guess memset is ok, as other similar functions use it. > > > > > -- > > per aspera ad upstream > With C99 you can just do: struct flow flow = { };
On 26 May 2021, at 14:34, Eelco Chaudron wrote: > On 18 May 2021, at 20:15, Ilya Maximets wrote: > >> On 5/18/21 4:44 PM, Eelco Chaudron wrote: >>> Add support for the dec_ttl action. Instead of programming the >>> datapath with >>> a flow that matches the packet TTL and an IP set, use a single >>> dec_ttl action. >>> >>> The old behavior is kept if the new action is not supported by the >>> datapath. >>> >>> # ovs-ofctl dump-flows br0 >>> cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, >>> ip actions=dec_ttl,NORMAL >>> cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, >>> actions=NORMAL >>> >>> # ping -c1 -t 20 192.168.0.2 >>> PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. >>> IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP >>> (1), length 84) >>> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, >>> length 64 >>> >>> Linux netlink datapath support depends on upstream Linux commit: >>> 744676e77720 ("openvswitch: add TTL decrement action") >>> >>> >>> Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has >>> been >>> defined, and to make sure the IDs are in sync, it had to be added to >>> the >>> OVS source tree. This required some additional case statements, >>> which >>> should be revisited once the OVS implementation is added. > > <SNIP> > >>> @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, >>> ds_destroy(&s); >>> } >>> >>> +/* New handling for dec_ttl action. */ >> >> 'New handling' makes sense in a patch, but doesn't while reading the >> code. > > Yes, I will remove this comment altogether as it makes no sense. > > <SNIP> > >>> >>> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL >>> via >>> + * OVS_ACTION_DEC_TTL. */ >>> +static bool >>> +check_dec_ttl_action(struct dpif *dpif) >>> +{ >>> + struct odputil_keybuf keybuf; >>> + struct flow flow = { 0 }; >> >> It's probbaly better to just memset it as in other similar functions >> to avoid compiler's complains. > > ACK, will use a memset here. > > <SNIP> > >>> /* Tests whether 'backer''s datapath supports the clone action >>> * OVS_ACTION_ATTR_CLONE. */ >>> static bool >>> @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) >>> dpif_supports_explicit_drop_action(backer->dpif); >>> backer->rt_support.lb_output_action= >>> dpif_supports_lb_output_action(backer->dpif); >>> + backer->rt_support.dec_ttl_action = >>> check_dec_ttl_action(backer->dpif); >> >> During discussions about all-zero SNAT feature support I remembered >> that >> we have a 'capabilities' table that should reflect all the datapath >> supported fetures. So, this should be added there as well. And >> documented >> in vswitchd/vswitch.xml. > > ACK, will add. > > <SNIP> >>> >>> /* Stores the various features which the corresponding backer >>> supports. */ >>> diff --git a/tests/odp.at b/tests/odp.at >>> index b762ebb2b..24946bec4 100644 >>> --- a/tests/odp.at >>> +++ b/tests/odp.at >>> @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) >>> check_pkt_len(size=200,gt(ct(nat)),le(drop)) >>> check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) >>> lb_output(1) >>> +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) >> >> Maybe it will make sense to also add a very simple variant of this >> action, >> e.g. dec_ttl(le_1(drop)). > > Added, drop and it resulted in an issue (fixed). > > > Doing some final tests and will sent out a V5 soon! > Hi Ilya/Bindiya, I was preparing my v5, and I noticed that a bunch of self-tests fail. I was wondering why I (and Matteo/Bindiya) never noticed. For me, it was because I was running make check on my dev system, which had an old kernel :( The datapath tests I was running on my DUT. I did solve most of the problems, but there are some corner cases that might be hard (and was wondering if you guys even thought about them): - As dec_ttl is none reversible there are some cases that it needs to clone the packet. Take the following example with a patch port (to solve this I sent another preceding patch, "ofproto-dpif: fix issue with non-reversible actions on a patch port"): Rule set: ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=2,1]) [br0 port 2 <===> port 1 br1] ovs-ofctl -O OpenFlow13 add-flow br1 in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,3]) Becomes: clone(dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=1,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3),1 Where it was: set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1' This clone action is NOT hardware offloadable, so wondering if this is needed in your use case? This is fixed in my v5. - tunnel encapsulation copies TTL value from the header however, because dec_ttl is a dynamic action, the value from the upcall packet is copied as the mpls_pop is static Not sure how to solve this as the TTL value is hardcoded by the datapath rule? Any ideas, other than changing flower to support dynamic TTL (with all the additional complex logic), or track the number of dec_ttl actions before encap? Note that doing encapsulation after dec_ttl might already cause dec_ttl dp action to lose its benefits as the nw_ttl match is added. - The documentation for the dec_ttl OpenFlow action states the following: "However, if the current set of actions was reached through resubmit, the remaining actions in outer levels resume processing." This is also something that is hard, as the dec_ttl dp action will stop processing after the exception. So take the following example: bridge("br0") ------------- 0. in_port=1, priority 32768 dec_ttl output:2 resubmit(1,1) 1. in_port=1, priority 32768 dec_ttl output:3 output:4 The old actions are resolved as follows: Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=2,nw_frag=no Datapath actions: set(ipv4(ttl=1)),2,userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)),4 The new action without a proper fix: Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no Datapath actions: dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),2,dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535)))),3,4" Without a proper fix, this will not send the packet to port 4. Also not sure how to solve this, guess we could do another clone here. But I have no idea if this path can be detected to avoid an unnecessary clone, need some research. With all the above, I'm wondering if dec_ttl will solve the actual problem of HW offloading? Bindiya do you have an example of flows to show the use case (both OpenFlow and the generated datapath flows)? Cheers, Eelco
Comments inline BK >> On Wed, Jun 23, 2021 at 3:50 PM Eelco Chaudron <echaudro@redhat.com> wrote: > On 26 May 2021, at 14:34, Eelco Chaudron wrote: > > On 18 May 2021, at 20:15, Ilya Maximets wrote: > > On 5/18/21 4:44 PM, Eelco Chaudron wrote: > > Add support for the dec_ttl action. Instead of programming the datapath > with > a flow that matches the packet TTL and an IP set, use a single dec_ttl > action. > > The old behavior is kept if the new action is not supported by the > datapath. > > # ovs-ofctl dump-flows br0 > cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip > actions=dec_ttl,NORMAL > cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, > actions=NORMAL > > # ping -c1 -t 20 192.168.0.2 > PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. > IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), > length 84) > 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64 > > Linux netlink datapath support depends on upstream Linux commit: > 744676e77720 ("openvswitch: add TTL decrement action") > > Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been > defined, and to make sure the IDs are in sync, it had to be added to the > OVS source tree. This required some additional case statements, which > should be revisited once the OVS implementation is added. > > <SNIP> > > @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, > ds_destroy(&s); > } > > +/* New handling for dec_ttl action. */ > > 'New handling' makes sense in a patch, but doesn't while reading the code. > > Yes, I will remove this comment altogether as it makes no sense. > > <SNIP> > > +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via > + * OVS_ACTION_DEC_TTL. */ > +static bool > +check_dec_ttl_action(struct dpif *dpif) > +{ > + struct odputil_keybuf keybuf; > + struct flow flow = { 0 }; > > It's probbaly better to just memset it as in other similar functions > to avoid compiler's complains. > > ACK, will use a memset here. > > <SNIP> > > /* Tests whether 'backer''s datapath supports the clone action > * OVS_ACTION_ATTR_CLONE. */ > static bool > @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) > dpif_supports_explicit_drop_action(backer->dpif); > backer->rt_support.lb_output_action= > dpif_supports_lb_output_action(backer->dpif); > + backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif); > > During discussions about all-zero SNAT feature support I remembered that > we have a 'capabilities' table that should reflect all the datapath > supported fetures. So, this should be added there as well. And documented > in vswitchd/vswitch.xml. > > ACK, will add. > > <SNIP> > > /* Stores the various features which the corresponding backer supports. */ > diff --git a/tests/odp.at b/tests/odp.at > index b762ebb2b..24946bec4 100644 > --- a/tests/odp.at > +++ b/tests/odp.at > @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) > check_pkt_len(size=200,gt(ct(nat)),le(drop)) > check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) > > lb_output(1) > > +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) > > Maybe it will make sense to also add a very simple variant of this action, > e.g. dec_ttl(le_1(drop)). > > Added, drop and it resulted in an issue (fixed). > > Doing some final tests and will sent out a V5 soon! > > Hi Ilya/Bindiya, > > I was preparing my v5, and I noticed that a bunch of self-tests fail. I > was wondering why I (and Matteo/Bindiya) never noticed. For me, it was > because I was running make check on my dev system, which had an old kernel > :( The datapath tests I was running on my DUT. > > I did solve most of the problems, but there are some corner cases that > might be hard (and was wondering if you guys even thought about them): > > - > > As dec_ttl is none reversible there are some cases that it needs to > clone the packet. Take the following example with a patch port (to solve > this I sent another preceding patch, "ofproto-dpif: fix issue with > non-reversible actions on a patch port"): > > Rule set: > ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=2,1]) > [br0 port 2 <===> port 1 br1] > ovs-ofctl -O OpenFlow13 add-flow br1 > in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,3]) > > Becomes: > > clone(dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=1,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3),1 > > Where it was: > > set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1' > > This clone action is NOT hardware offloadable, so wondering if this is > needed in your use case? This is fixed in my v5. > > BK >> We do not have use-case of offload flows with clone action to hardware > > - > - > > tunnel encapsulation copies TTL value from the header however, because > dec_ttl is a dynamic action, the value from the upcall packet is copied as > the mpls_pop is static > > Not sure how to solve this as the TTL value is hardcoded by the > datapath rule? Any ideas, other than changing flower to support dynamic TTL > (with all the additional complex logic), or track the number of dec_ttl > actions before encap? > Note that doing encapsulation after dec_ttl might already cause > dec_ttl dp action to lose its benefits as the nw_ttl match is added. > > BK >> Didn't get this question clearly can you please elaborate more on this " because dec_ttl is a dynamic action, the value from the upcall packet is copied as the mpls_pop is static" > > - > - > > The documentation for the dec_ttl OpenFlow action states the following: > > "However, if the current set of actions was reached through resubmit, > the remaining actions in outer levels resume processing." > > This is also something that is hard, as the dec_ttl dp action will > stop processing after the exception. So take the following example: > > bridge("br0") > ------------- > 0. in_port=1, priority 32768 > dec_ttl > output:2 > resubmit(1,1) > 1. in_port=1, priority 32768 > dec_ttl > output:3 > output:4 > > The old actions are resolved as follows: > > Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=2,nw_frag=no > Datapath actions: set(ipv4(ttl=1)),2,userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)),4 > > The new action without a proper fix: > > Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no > Datapath actions: dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),2,dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535)))),3,4" > > Without a proper fix, this will not send the packet to port 4. Also > not sure how to solve this, guess we could do another clone here. But I > have no idea if this path can be detected to avoid an unnecessary clone, > need some research. > > BK >> when dec_ttl is changed in datapath , assumption was once exception happens packets should be forwarded to controller.Controller will take right action on that packet and no further action will be taken by datapath. I understand it contradicts the documentation that you are referring to. But do you think this needs to be changed in documentation rather than in implementation.Since dec_ttl action is existing in user-space for a long time , I'm not sure about the reasoning behind the same. > > - > > With all the above, I'm wondering if dec_ttl will solve the actual problem > of HW offloading? > BK >> Do you mean by this, to support dec_ttl in some use-cases , clone action needs to be taken care of ,and since clone action is not off loadable, hw offloading problem is not solved ? > Bindiya do you have an example of flows to show the use case (both > OpenFlow and the generated datapath flows)? > BK > In our system we do not have the use-case that you mentioned above , so I do not have openflow action and corresponding datapath flows . > Cheers, > > Eelco >
On 20 Jul 2021, at 9:55, bindiya Kurle wrote: > Comments inline BK >> > > On Wed, Jun 23, 2021 at 3:50 PM Eelco Chaudron <echaudro@redhat.com> wrote: > >> On 26 May 2021, at 14:34, Eelco Chaudron wrote: >> >> On 18 May 2021, at 20:15, Ilya Maximets wrote: >> >> On 5/18/21 4:44 PM, Eelco Chaudron wrote: >> >> Add support for the dec_ttl action. Instead of programming the datapath >> with >> a flow that matches the packet TTL and an IP set, use a single dec_ttl >> action. >> >> The old behavior is kept if the new action is not supported by the >> datapath. >> >> # ovs-ofctl dump-flows br0 >> cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip >> actions=dec_ttl,NORMAL >> cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, >> actions=NORMAL >> >> # ping -c1 -t 20 192.168.0.2 >> PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. >> IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), >> length 84) >> 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64 >> >> Linux netlink datapath support depends on upstream Linux commit: >> 744676e77720 ("openvswitch: add TTL decrement action") >> >> Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been >> defined, and to make sure the IDs are in sync, it had to be added to the >> OVS source tree. This required some additional case statements, which >> should be revisited once the OVS implementation is added. >> >> <SNIP> >> >> @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, >> ds_destroy(&s); >> } >> >> +/* New handling for dec_ttl action. */ >> >> 'New handling' makes sense in a patch, but doesn't while reading the code. >> >> Yes, I will remove this comment altogether as it makes no sense. >> >> <SNIP> >> >> +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via >> + * OVS_ACTION_DEC_TTL. */ >> +static bool >> +check_dec_ttl_action(struct dpif *dpif) >> +{ >> + struct odputil_keybuf keybuf; >> + struct flow flow = { 0 }; >> >> It's probbaly better to just memset it as in other similar functions >> to avoid compiler's complains. >> >> ACK, will use a memset here. >> >> <SNIP> >> >> /* Tests whether 'backer''s datapath supports the clone action >> * OVS_ACTION_ATTR_CLONE. */ >> static bool >> @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) >> dpif_supports_explicit_drop_action(backer->dpif); >> backer->rt_support.lb_output_action= >> dpif_supports_lb_output_action(backer->dpif); >> + backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif); >> >> During discussions about all-zero SNAT feature support I remembered that >> we have a 'capabilities' table that should reflect all the datapath >> supported fetures. So, this should be added there as well. And documented >> in vswitchd/vswitch.xml. >> >> ACK, will add. >> >> <SNIP> >> >> /* Stores the various features which the corresponding backer supports. */ >> diff --git a/tests/odp.at b/tests/odp.at >> index b762ebb2b..24946bec4 100644 >> --- a/tests/odp.at >> +++ b/tests/odp.at >> @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) >> check_pkt_len(size=200,gt(ct(nat)),le(drop)) >> check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) >> >> lb_output(1) >> >> +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) >> >> Maybe it will make sense to also add a very simple variant of this action, >> e.g. dec_ttl(le_1(drop)). >> >> Added, drop and it resulted in an issue (fixed). >> >> Doing some final tests and will sent out a V5 soon! >> >> Hi Ilya/Bindiya, >> >> I was preparing my v5, and I noticed that a bunch of self-tests fail. I >> was wondering why I (and Matteo/Bindiya) never noticed. For me, it was >> because I was running make check on my dev system, which had an old kernel >> :( The datapath tests I was running on my DUT. >> >> I did solve most of the problems, but there are some corner cases that >> might be hard (and was wondering if you guys even thought about them): >> >> - >> >> As dec_ttl is none reversible there are some cases that it needs to >> clone the packet. Take the following example with a patch port (to solve >> this I sent another preceding patch, "ofproto-dpif: fix issue with >> non-reversible actions on a patch port"): >> >> Rule set: >> ovs-ofctl -O OpenFlow13 add-flow br0 in_port=local,ip,actions=2,1]) >> [br0 port 2 <===> port 1 br1] >> ovs-ofctl -O OpenFlow13 add-flow br1 >> in_port=1,ip,actions=dec_ttl,push_mpls:0x8847,3]) >> >> Becomes: >> >> clone(dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=1,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3),1 >> >> Where it was: >> >> set(ipv4(ttl=63)),push_mpls(label=0,tc=0,ttl=63,bos=1,eth_type=0x8847),3,pop_mpls(eth_type=0x800),set(ipv4(ttl=64)),1' >> >> This clone action is NOT hardware offloadable, so wondering if this is >> needed in your use case? This is fixed in my v5. >> > BK >> We do not have use-case of offload flows with clone action to hardware Ack so this is less of a problem for you. >> >> - >> - >> >> tunnel encapsulation copies TTL value from the header however, because >> dec_ttl is a dynamic action, the value from the upcall packet is copied as >> the mpls_pop is static >> >> Not sure how to solve this as the TTL value is hardcoded by the >> datapath rule? Any ideas, other than changing flower to support dynamic TTL >> (with all the additional complex logic), or track the number of dec_ttl >> actions before encap? >> Note that doing encapsulation after dec_ttl might already cause >> dec_ttl dp action to lose its benefits as the nw_ttl match is added. >> >> BK >> Didn't get this question clearly can you please > elaborate more on this " because dec_gtl is a dynamic action, the value > from the upcall packet is copied as the mpls_pop is static" When you encapsulate an IP packet in a tunnel the TTL value for the tunnel is copied from the original IP packet (the matching part). In the past, this was not a problem, as we would adjust the TTL during the lookup and it was part of the flow (i.e. we would have one flow entry for each TTL value) and copy it. Now we do not adjust/match the TTL as part of the flow, so when we do the copy the wrong TTL value is copied. >> >> - >> - >> >> The documentation for the dec_ttl OpenFlow action states the following: >> >> "However, if the current set of actions was reached through resubmit, >> the remaining actions in outer levels resume processing." >> >> This is also something that is hard, as the dec_ttl dp action will >> stop processing after the exception. So take the following example: >> >> bridge("br0") >> ------------- >> 0. in_port=1, priority 32768 >> dec_ttl >> output:2 >> resubmit(1,1) >> 1. in_port=1, priority 32768 >> dec_ttl >> output:3 >> output:4 >> >> The old actions are resolved as follows: >> >> Megaflow: recirc_id=0,eth,ip,in_port=1,nw_ttl=2,nw_frag=no >> Datapath actions: set(ipv4(ttl=1)),2,userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)),4 >> >> The new action without a proper fix: >> >> Megaflow: recirc_id=0,eth,ip,in_port=1,nw_frag=no >> Datapath actions: dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))),2,dec_ttl(le_1(userspace(pid=0,controller(reason=2,dont_send=0,continuation=0,recirc_id=2,rule_cookie=0,controller_id=0,max_len=65535)))),3,4" >> >> Without a proper fix, this will not send the packet to port 4. Also >> not sure how to solve this, guess we could do another clone here. But I >> have no idea if this path can be detected to avoid an unnecessary clone, >> need some research. >> >> BK >> when dec_ttl is changed in datapath , assumption was once exception > happens packets should be forwarded to controller.Controller will take > right action on that packet and no further action will be taken by > datapath. I understand it contradicts the documentation that you are > referring to. But do you think this needs to be changed in documentation > rather than in implementation.Since dec_ttl action is existing in > user-space for a long time , I'm not sure about the reasoning behind the > same. I think it’s too late to change an existing behavior by just changing the documentation :) I have not checked the OpenFlow specification, but it might be part of it. >> >> - >> >> With all the above, I'm wondering if dec_ttl will solve the actual problem >> of HW offloading? >> > BK >> Do you mean by this, to support dec_ttl in some use-cases , clone > action needs to be taken care of ,and since clone action is not off > loadable, hw offloading problem is not solved ? ACK >> Bindiya do you have an example of flows to show the use case (both >> OpenFlow and the generated datapath flows)? >> > BK > In our system we do not have the use-case that you mentioned above , > so I do not have openflow action and corresponding datapath flows . I’m referring to your use cases specifically. Can you share your OpenFlow rule set and some example traffic patterns? However in general to make dec_ttl work as is (even without the HW offload) we still need to solve both above problems.
diff --git a/datapath/linux/compat/include/linux/openvswitch.h b/datapath/linux/compat/include/linux/openvswitch.h index 875de2025..a5357024d 100644 --- a/datapath/linux/compat/include/linux/openvswitch.h +++ b/datapath/linux/compat/include/linux/openvswitch.h @@ -1030,6 +1030,8 @@ enum ovs_action_attr { OVS_ACTION_ATTR_METER, /* u32 meter number. */ OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */ OVS_ACTION_ATTR_CHECK_PKT_LEN, /* Nested OVS_CHECK_PKT_LEN_ATTR_*. */ + OVS_ACTION_ATTR_ADD_MPLS, /* struct ovs_action_add_mpls. */ + OVS_ACTION_ATTR_DEC_TTL, /* Nested OVS_DEC_TTL_ATTR_*. */ #ifndef __KERNEL__ OVS_ACTION_ATTR_TUNNEL_PUSH, /* struct ovs_action_push_tnl*/ @@ -1133,4 +1135,12 @@ struct ovs_zone_limit { * keys. False otherwise. */ +enum ovs_dec_ttl_attr { + OVS_DEC_TTL_ATTR_UNSPEC, + OVS_DEC_TTL_ATTR_ACTION, /* Nested struct nlattr */ + __OVS_DEC_TTL_ATTR_MAX +}; + +#define OVS_DEC_TTL_ATTR_MAX (__OVS_DEC_TTL_ATTR_MAX - 1) + #endif /* _LINUX_OPENVSWITCH_H */ diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 251788b04..c6032e013 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -8046,6 +8046,8 @@ dp_execute_cb(void *aux_, struct dp_packet_batch *packets_, case OVS_ACTION_ATTR_CT_CLEAR: case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_DEC_TTL: + case OVS_ACTION_ATTR_ADD_MPLS: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } diff --git a/lib/dpif.c b/lib/dpif.c index 26e8bfb7d..a25d8e9ef 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1274,6 +1274,8 @@ dpif_execute_helper_cb(void *aux_, struct dp_packet_batch *packets_, case OVS_ACTION_ATTR_UNSPEC: case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } diff --git a/lib/odp-execute.c b/lib/odp-execute.c index 6eeda2a61..ff2921045 100644 --- a/lib/odp-execute.c +++ b/lib/odp-execute.c @@ -719,6 +719,55 @@ odp_execute_sample(void *dp, struct dp_packet *packet, bool steal, nl_attr_get_size(subactions), dp_execute_action); } +static bool +execute_dec_ttl(struct dp_packet *packet) +{ + struct eth_header *eth = dp_packet_eth(packet); + + if (dl_type_is_ipv4(eth->eth_type)) { + struct ip_header *nh = dp_packet_l3(packet); + uint8_t old_ttl = nh->ip_ttl; + + if (old_ttl <= 1) { + return true; + } + + nh->ip_ttl--; + nh->ip_csum = recalc_csum16(nh->ip_csum, htons(old_ttl << 8), + htons(nh->ip_ttl << 8)); + } else if (dl_type_is_ipv6(eth->eth_type)) { + struct ovs_16aligned_ip6_hdr *nh = dp_packet_l3(packet); + + if (nh->ip6_hlim <= 1) { + return true; + } + + nh->ip6_hlim--; + } + + return false; +} + +static void +odp_dec_ttl_exception_handler(void *dp, struct dp_packet_batch *batch, + const struct nlattr *action, + odp_execute_cb dp_execute_action) +{ + static const struct nl_policy dec_ttl_policy[] = { + [OVS_DEC_TTL_ATTR_ACTION] = { .type = NL_A_NESTED }, + }; + struct nlattr *attrs[ARRAY_SIZE(dec_ttl_policy)]; + + ovs_assert(nl_parse_nested(action, dec_ttl_policy, attrs, + ARRAY_SIZE(attrs)) + && attrs[OVS_DEC_TTL_ATTR_ACTION]); + + odp_execute_actions(dp, batch, true, + nl_attr_get(attrs[OVS_DEC_TTL_ATTR_ACTION]), + nl_attr_get_size(attrs[OVS_DEC_TTL_ATTR_ACTION]), + dp_execute_action); +} + static void odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, const struct nlattr *actions, @@ -734,7 +783,7 @@ odp_execute_clone(void *dp, struct dp_packet_batch *batch, bool steal, dp_packet_batch_clone(&clone_pkt_batch, batch); dp_packet_batch_reset_cutlen(batch); odp_execute_actions(dp, &clone_pkt_batch, true, nl_attr_get(actions), - nl_attr_get_size(actions), dp_execute_action); + nl_attr_get_size(actions), dp_execute_action); } else { odp_execute_actions(dp, batch, true, nl_attr_get(actions), @@ -820,6 +869,8 @@ requires_datapath_assistance(const struct nlattr *a) case OVS_ACTION_ATTR_CT_CLEAR: case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: return false; case OVS_ACTION_ATTR_UNSPEC: @@ -979,6 +1030,40 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, } break; + case OVS_ACTION_ATTR_DEC_TTL: { + const size_t cnt = dp_packet_batch_size(batch); + struct dp_packet_batch invalid_ttl; + size_t i; + + /* Make batch for invalid ttl packets. */ + dp_packet_batch_init(&invalid_ttl); + invalid_ttl.trunc = batch->trunc; + invalid_ttl.do_not_steal = batch->do_not_steal; + + /* Add packets with ttl <=1 to the invalid_ttl batch + * and remove it from the batch. */ + DP_PACKET_BATCH_REFILL_FOR_EACH (i, cnt, packet, batch) { + if (execute_dec_ttl(packet)) { + dp_packet_batch_add(&invalid_ttl, packet); + } else { + dp_packet_batch_refill(batch, packet, i); + } + } + + /* Execute action on packets with ttl <= 1. */ + if (!dp_packet_batch_is_empty(&invalid_ttl)) { + odp_dec_ttl_exception_handler(dp, &invalid_ttl, a, + dp_execute_action); + } + + if (dp_packet_batch_is_empty(batch)) { + /* The entire batch was handled as an exception, so we can + * stop processing actions. */ + return; + } + break; + } + case OVS_ACTION_ATTR_TRUNC: { const struct ovs_action_trunc *trunc = nl_attr_get_unspec(a, sizeof *trunc); @@ -1077,6 +1162,7 @@ odp_execute_actions(void *dp, struct dp_packet_batch *batch, bool steal, case OVS_ACTION_ATTR_RECIRC: case OVS_ACTION_ATTR_CT: case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_ADD_MPLS: case __OVS_ACTION_ATTR_MAX: OVS_NOT_REACHED(); } diff --git a/lib/odp-util.c b/lib/odp-util.c index e1199d1da..922104e72 100644 --- a/lib/odp-util.c +++ b/lib/odp-util.c @@ -143,8 +143,10 @@ odp_action_len(uint16_t type) case OVS_ACTION_ATTR_POP_NSH: return 0; case OVS_ACTION_ATTR_CHECK_PKT_LEN: return ATTR_LEN_VARIABLE; case OVS_ACTION_ATTR_DROP: return sizeof(uint32_t); + case OVS_ACTION_ATTR_DEC_TTL: return ATTR_LEN_VARIABLE; case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_ADD_MPLS: case __OVS_ACTION_ATTR_MAX: return ATTR_LEN_INVALID; } @@ -1110,6 +1112,25 @@ format_odp_check_pkt_len_action(struct ds *ds, const struct nlattr *attr, ds_put_cstr(ds, "))"); } +static void +format_dec_ttl_action(struct ds *ds, const struct nlattr *attr, + const struct hmap *portno_names) +{ + const struct nlattr *a; + unsigned int left; + + ds_put_cstr(ds,"dec_ttl(le_1("); + NL_ATTR_FOR_EACH (a, left, + nl_attr_get(attr), nl_attr_get_size(attr)) { + if (nl_attr_type(a) == OVS_DEC_TTL_ATTR_ACTION) { + format_odp_actions(ds, nl_attr_get(a), + nl_attr_get_size(a), portno_names); + break; + } + } + ds_put_format(ds, "))"); +} + static void format_odp_action(struct ds *ds, const struct nlattr *a, const struct hmap *portno_names) @@ -1257,7 +1278,11 @@ format_odp_action(struct ds *ds, const struct nlattr *a, case OVS_ACTION_ATTR_DROP: ds_put_cstr(ds, "drop"); break; + case OVS_ACTION_ATTR_DEC_TTL: + format_dec_ttl_action(ds, a, portno_names); + break; case OVS_ACTION_ATTR_UNSPEC: + case OVS_ACTION_ATTR_ADD_MPLS: case __OVS_ACTION_ATTR_MAX: default: format_generic_odp_action(ds, a); @@ -2505,6 +2530,26 @@ parse_odp_action__(struct parse_odp_context *context, const char *s, return n + 1; } } + { + if (!strncmp(s, "dec_ttl(le_1(", 13)) { + size_t actions_ofs, ofs; + int n = 13; + + ofs = nl_msg_start_nested(actions, OVS_ACTION_ATTR_DEC_TTL); + actions_ofs = nl_msg_start_nested(actions, + OVS_DEC_TTL_ATTR_ACTION); + + int retval = parse_action_list(context, s + n, actions); + if (retval < 0) { + return retval; + } + + n += retval; + nl_msg_end_nested(actions, actions_ofs); + nl_msg_end_nested(actions, ofs); + return n + 2; + } + } { if (!strncmp(s, "push_nsh(", 9)) { diff --git a/lib/packets.h b/lib/packets.h index 481bc22fa..b7fa7b121 100644 --- a/lib/packets.h +++ b/lib/packets.h @@ -1211,10 +1211,19 @@ bool in6_is_lla(struct in6_addr *addr); void ipv6_multicast_to_ethernet(struct eth_addr *eth, const struct in6_addr *ip6); +static inline bool dl_type_is_ipv4(ovs_be16 dl_type) +{ + return dl_type == htons(ETH_TYPE_IP); +} + +static inline bool dl_type_is_ipv6(ovs_be16 dl_type) +{ + return dl_type == htons(ETH_TYPE_IPV6); +} + static inline bool dl_type_is_ip_any(ovs_be16 dl_type) { - return dl_type == htons(ETH_TYPE_IP) - || dl_type == htons(ETH_TYPE_IPV6); + return dl_type_is_ipv4(dl_type) || dl_type_is_ipv6(dl_type); } /* Tunnel header */ diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c index 796eb6f88..1c8e7ae26 100644 --- a/ofproto/ofproto-dpif-ipfix.c +++ b/ofproto/ofproto-dpif-ipfix.c @@ -3018,6 +3018,8 @@ dpif_ipfix_read_actions(const struct flow *flow, case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_UNSPEC: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_ADD_MPLS: + case OVS_ACTION_ATTR_DEC_TTL: case __OVS_ACTION_ATTR_MAX: default: break; diff --git a/ofproto/ofproto-dpif-sflow.c b/ofproto/ofproto-dpif-sflow.c index 864c136b5..83989fb90 100644 --- a/ofproto/ofproto-dpif-sflow.c +++ b/ofproto/ofproto-dpif-sflow.c @@ -1178,6 +1178,7 @@ dpif_sflow_read_actions(const struct flow *flow, case OVS_ACTION_ATTR_CT_CLEAR: case OVS_ACTION_ATTR_METER: case OVS_ACTION_ATTR_LB_OUTPUT: + case OVS_ACTION_ATTR_DEC_TTL: break; case OVS_ACTION_ATTR_SET_MASKED: @@ -1226,6 +1227,7 @@ dpif_sflow_read_actions(const struct flow *flow, case OVS_ACTION_ATTR_UNSPEC: case OVS_ACTION_ATTR_CHECK_PKT_LEN: case OVS_ACTION_ATTR_DROP: + case OVS_ACTION_ATTR_ADD_MPLS: case __OVS_ACTION_ATTR_MAX: default: break; diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c index 7108c8a30..3cac92940 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -40,6 +40,7 @@ #include "mac-learning.h" #include "mcast-snooping.h" #include "multipath.h" +#include "netdev-offload.h" #include "netdev-vport.h" #include "netlink.h" #include "nx-match.h" @@ -4840,9 +4841,12 @@ xlate_controller_action(struct xlate_ctx *ctx, int len, enum ofp_packet_in_reason reason, uint16_t controller_id, uint32_t provider_meter_id, - const uint8_t *userdata, size_t userdata_len) + const uint8_t *userdata, size_t userdata_len, + bool commit) { - xlate_commit_actions(ctx); + if (commit) { + xlate_commit_actions(ctx); + } /* A packet sent by an action in a table-miss rule is considered an * explicit table miss. OpenFlow before 1.3 doesn't have that concept so @@ -5075,10 +5079,6 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) { struct flow *flow = &ctx->xin->flow; - if (!is_ip_any(flow)) { - return false; - } - ctx->wc->masks.nw_ttl = 0xff; if (flow->nw_ttl > 1) { flow->nw_ttl--; @@ -5088,7 +5088,8 @@ compose_dec_ttl(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) for (i = 0; i < ids->n_controllers; i++) { xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, - ids->cnt_ids[i], UINT32_MAX, NULL, 0); + ids->cnt_ids[i], UINT32_MAX, NULL, 0, + true); } /* Stop processing for current table. */ @@ -5129,7 +5130,7 @@ compose_dec_nsh_ttl_action(struct xlate_ctx *ctx) return false; } else { xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, - 0, UINT32_MAX, NULL, 0); + 0, UINT32_MAX, NULL, 0, true); } } @@ -5162,7 +5163,7 @@ compose_dec_mpls_ttl_action(struct xlate_ctx *ctx) return false; } else { xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, 0, - UINT32_MAX, NULL, 0); + UINT32_MAX, NULL, 0, true); } } @@ -5186,6 +5187,40 @@ xlate_delete_field(struct xlate_ctx *ctx, ds_destroy(&s); } +/* New handling for dec_ttl action. */ +static bool +xlate_dec_ttl_action(struct xlate_ctx *ctx, struct ofpact_cnt_ids *ids) +{ + struct flow *flow = &ctx->xin->flow; + size_t offset, offset_actions; + size_t i; + + if (!is_ip_any(flow)) { + return false; + } + + if (!ctx->xbridge->support.dec_ttl_action + || netdev_is_flow_api_enabled()) { + return compose_dec_ttl(ctx, ids); + } + + xlate_commit_actions(ctx); + offset = nl_msg_start_nested(ctx->odp_actions, OVS_ACTION_ATTR_DEC_TTL); + offset_actions = nl_msg_start_nested(ctx->odp_actions, + OVS_DEC_TTL_ATTR_ACTION); + + for (i = 0; i < ids->n_controllers; i++) { + xlate_controller_action(ctx, UINT16_MAX, OFPR_INVALID_TTL, + ids->cnt_ids[i], UINT32_MAX, NULL, 0, false); + } + + nl_msg_end_nested(ctx->odp_actions, offset_actions); + nl_msg_end_nested(ctx->odp_actions, offset); + + xlate_commit_actions(ctx); + return false; +} + /* Emits an action that outputs to 'port', within 'ctx'. * * 'controller_len' affects only packets sent to an OpenFlow controller. It @@ -5236,7 +5271,7 @@ xlate_output_action(struct xlate_ctx *ctx, ofp_port_t port, : group_bucket_action ? OFPR_GROUP : ctx->in_action_set ? OFPR_ACTION_SET : OFPR_ACTION), - 0, UINT32_MAX, NULL, 0); + 0, UINT32_MAX, NULL, 0, true); break; case OFPP_NONE: break; @@ -6797,7 +6832,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, controller->controller_id, controller->provider_meter_id, controller->userdata, - controller->userdata_len); + controller->userdata_len, true); } break; @@ -7005,8 +7040,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len, break; case OFPACT_DEC_TTL: - wc->masks.nw_ttl = 0xff; - if (compose_dec_ttl(ctx, ofpact_get_DEC_TTL(a))) { + if (xlate_dec_ttl_action(ctx, ofpact_get_DEC_TTL(a))) { return; } break; diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index fd0b2fdea..49e026359 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -1186,6 +1186,45 @@ check_trunc_action(struct dpif_backer *backer) return !error; } +/* Tests whether 'dpif' datapath supports decrement of the IP TTL via + * OVS_ACTION_DEC_TTL. */ +static bool +check_dec_ttl_action(struct dpif *dpif) +{ + struct odputil_keybuf keybuf; + struct flow flow = { 0 }; + struct ofpbuf actions; + uint32_t actbuf[4]; + struct ofpbuf key; + bool supported; + size_t start; + + struct odp_flow_key_parms odp_parms = { + .flow = &flow, + .probe = true, + }; + + ofpbuf_use_stack(&key, &keybuf, sizeof keybuf); + odp_flow_key_from_flow(&odp_parms, &key); + + ofpbuf_use_stack(&actions, &actbuf, sizeof actbuf); + start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_DEC_TTL); + nl_msg_put_flag(&actions, OVS_DEC_TTL_ATTR_ACTION); + nl_msg_end_nested(&actions, start); + + supported = dpif_probe_feature(dpif, "dec_ttl", &key, &actions, NULL); + + if (supported) { + VLOG_INFO("%s: Datapath supports dec_ttl action", + dpif_name(dpif)); + } else { + VLOG_INFO("%s: Datapath does not support dec_ttl action", + dpif_name(dpif)); + } + + return supported; +} + /* Tests whether 'backer''s datapath supports the clone action * OVS_ACTION_ATTR_CLONE. */ static bool @@ -1590,6 +1629,7 @@ check_support(struct dpif_backer *backer) dpif_supports_explicit_drop_action(backer->dpif); backer->rt_support.lb_output_action= dpif_supports_lb_output_action(backer->dpif); + backer->rt_support.dec_ttl_action = check_dec_ttl_action(backer->dpif); /* Flow fields. */ backer->rt_support.odp.ct_state = check_ct_state(backer); diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index b41c3d82a..f4d1152bc 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -204,7 +204,11 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *, DPIF_SUPPORT_FIELD(bool, explicit_drop_action, "Explicit Drop action") \ \ /* True if the datapath supports balance_tcp optimization */ \ - DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode") + DPIF_SUPPORT_FIELD(bool, lb_output_action, "Optimized Balance TCP mode")\ + \ + /* True if the datapath supports dec_ttl action. */ \ + DPIF_SUPPORT_FIELD(bool, dec_ttl_action, "Decrement TTL action") + /* Stores the various features which the corresponding backer supports. */ diff --git a/tests/odp.at b/tests/odp.at index b762ebb2b..24946bec4 100644 --- a/tests/odp.at +++ b/tests/odp.at @@ -384,6 +384,7 @@ check_pkt_len(size=200,gt(drop),le(5)) check_pkt_len(size=200,gt(ct(nat)),le(drop)) check_pkt_len(size=200,gt(set(eth(src=00:01:02:03:04:05,dst=10:11:12:13:14:15))),le(set(eth(src=00:01:02:03:04:06,dst=10:11:12:13:14:16)))) lb_output(1) +dec_ttl(le_1(userspace(pid=3614533484,controller(reason=2,dont_send=0,continuation=0,recirc_id=1,rule_cookie=0,controller_id=0,max_len=65535)))) ]) AT_CHECK_UNQUOTED([ovstest test-odp parse-actions < actions.txt], [0], [`cat actions.txt` diff --git a/tests/system-traffic.at b/tests/system-traffic.at index fb5b9a36d..ef9b555b6 100644 --- a/tests/system-traffic.at +++ b/tests/system-traffic.at @@ -1400,6 +1400,34 @@ AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], [dnl OVS_TRAFFIC_VSWITCHD_STOP AT_CLEANUP +AT_SETUP([datapath - dec_ttl action]) +OVS_TRAFFIC_VSWITCHD_START() + +AT_CHECK([ovs-ofctl add-flow br0 "actions=normal"]) + +ADD_NAMESPACES(at_ns0, at_ns1) + +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24") +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24") + +AT_CHECK([ovs-vsctl -- set interface ovs-p0 ofport_request=1 \ + -- set interface ovs-p1 ofport_request=2]) + +AT_CHECK([ovs-ofctl del-flows br0]) +AT_CHECK([ovs-ofctl add-flow br0 in_port=1,action=dec_ttl,2]) +AT_CHECK([ovs-ofctl add-flow br0 in_port=2,action=dec_ttl,1]) + +NS_CHECK_EXEC([at_ns0], [ping -c 1 -w 2 10.1.1.2 | grep -o -E 'ttl=[[[:digit:]]]+'], [0], [dnl +ttl=63 +]) + +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 -t 1 10.1.1.2 | FORMAT_PING], [0], [dnl +1 packets transmitted, 0 received, 100% packet loss, time 0ms +]) + +OVS_TRAFFIC_VSWITCHD_STOP +AT_CLEANUP + AT_BANNER([conntrack]) AT_SETUP([conntrack - controller])
Add support for the dec_ttl action. Instead of programming the datapath with a flow that matches the packet TTL and an IP set, use a single dec_ttl action. The old behavior is kept if the new action is not supported by the datapath. # ovs-ofctl dump-flows br0 cookie=0x0, duration=12.538s, table=0, n_packets=4, n_bytes=392, ip actions=dec_ttl,NORMAL cookie=0x0, duration=12.536s, table=0, n_packets=4, n_bytes=168, actions=NORMAL # ping -c1 -t 20 192.168.0.2 PING 192.168.0.2 (192.168.0.2) 56(84) bytes of data. IP (tos 0x0, ttl 19, id 45336, offset 0, flags [DF], proto ICMP (1), length 84) 192.168.0.1 > 192.168.0.2: ICMP echo request, id 8865, seq 1, length 64 Linux netlink datapath support depends on upstream Linux commit: 744676e77720 ("openvswitch: add TTL decrement action") Note that in the Linux kernel tree the OVS_ACTION_ATTR_ADD_MPLS has been defined, and to make sure the IDs are in sync, it had to be added to the OVS source tree. This required some additional case statements, which should be revisited once the OVS implementation is added. Co-authored-by: Bindiya Kurle <bindiyakurle@gmail.com> Co-authored-by: Matteo Croce <mcroce@linux.microsoft.com> Signed-off-by: Eelco Chaudron <echaudro@redhat.com> Signed-off-by: Bindiya Kurle <bindiyakurle@gmail.com> Signed-off-by: Matteo Croce <mcroce@linux.microsoft.com> Acked-by: Bindiya Kurle <bindiyakurle@gmail.com> --- v2: - Used definition instead of numeric value in format_dec_ttl_action() - Changed format from "dec_ttl(ttl<=1(<actions>)) to "dec_ttl(le_1(<actions>))" to be more in line with the check_pkt_len action. - Cleaned up format_dec_ttl_action() v3: - Fixed parsing of "dec_ttl()" action for adding a dp flow. - Changed implementation to use the fixed kernel mod implementation https://marc.info/?l=linux-netdev&m=160577671609295&w=2 - Removed introduced force_last flag from odp_execute_actions - For now, do not use this new attribute if HW offload is supported, as it's causing a performance regression due to HW offload not being supported. I will fix this in a separate patch. - Added datapath test case for dec_ttl action. v4: - Fixed some style issues reported by Ilya. - Changed OVS_NOT_REACHED() to an ovs_assert(), so information gets logged on error. - Use batch helper functions rather than access batch->count directly. - Fixed memory leak when dec_ttl was the last action in the chain. - Fixed do_xlate_actions() return if for exception in backward compatibility mode. - Added actions parsing test to odp.at datapath/linux/compat/include/linux/openvswitch.h | 10 ++ lib/dpif-netdev.c | 2 lib/dpif.c | 2 lib/odp-execute.c | 88 +++++++++++++++++++++ lib/odp-util.c | 45 +++++++++++ lib/packets.h | 13 +++ ofproto/ofproto-dpif-ipfix.c | 2 ofproto/ofproto-dpif-sflow.c | 2 ofproto/ofproto-dpif-xlate.c | 60 +++++++++++--- ofproto/ofproto-dpif.c | 40 ++++++++++ ofproto/ofproto-dpif.h | 6 + tests/odp.at | 1 tests/system-traffic.at | 28 +++++++ 13 files changed, 282 insertions(+), 17 deletions(-)