On Thu, Mar 2, 2017 at 2:59 PM, Jarno Rajahalme <[email protected]> wrote: > With the notes below: > > Acked-by: Jarno Rajahalme <[email protected]>
Thanks for the review, I have pushed all patches in the series with most comments applied as suggested. > >> On Feb 16, 2017, at 5:11 PM, Andy Zhou <[email protected]> wrote: >> >> Allow execute_controller_action() to accept actions encoded with >> nested netlink attributes. >> >> execute_controller_action() can be called during 'xlate_actions'. It >> tries executes all actions translated so far to get the current packet >> that needs to be sent to the controller. This works fine until when >> the action is enclosed within a nested netlink message, and the >> action translation has not finished yet. >> >> For example; >> A, clone(B, controller, C) >> >> In this case, we can not execute 'clone' since its translation has not >> be finished (missing C), However, A still needs to be executed before >> the packet can be sent to the controller. >> >> This solution is to make a copy of the odp actions translated so far, >> and 'fix up' the copy so that it can be executed. The original odp >> actions are left intact so that xlate can continue. >> >> Signed-off-by: Andy Zhou <[email protected]> >> --- >> ofproto/ofproto-dpif-xlate.c | 149 >> +++++++++++++++++++++++++++++++++++++++++-- >> 1 file changed, 144 insertions(+), 5 deletions(-) >> >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 503a347..c4ca5d2 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -3805,13 +3805,150 @@ flood_packets(struct xlate_ctx *ctx, bool all) >> ctx->nf_output_iface = NF_OUT_FLOOD; >> } >> >> +/* Copy and reformat a partially xlated odp actions to a new >> + * odp actions list in 'b', so that the new actions list >> + * can be executed by odp_execute_actions. >> + * >> + * When xlate using nested odp actions, such as sample and clone, >> + * The nested action created by nl_msg_start_nested() may not > > “the” > >> + * have been properly closed yet, thus can not be executed >> + * directly. >> + * >> + * Since unclosed nested action has to be last action, it can be >> + * fixed by skip the outer header, and treat the actions within > > “skipping”, “treating" > >> + * as if they are outside the nested attribute. Since the effect > > “, since" > >> + * of executing them on packet is the same. >> + * >> + * As an optimization, a fully closed 'sample' or 'clone' action >> + * is skipped since their execution has no effect to the packet. >> + * > > In this case the actions are executed without a datapath helper, so none of > the datapath dependent actions (HASH, OUTPUT, USERSPACE, RECIRC, CT) are > actually executed, so maybe they could be skipped as well? Same for the > TRUNC, as it only has an effect on OUTPUT, which will not be executed. I did not do this because it seems to be a layer violation, The idea of which actions needs datapath help is currently encapsulated within the ODP layer. Since this is a slow path anyways, such optimization may not be critical. > >> + * Returns true if success. 'b' contains the new actions list. >> + * The caller is responsible for dispose 'b'. >> + * > > “disposing" > >> + * Returns false if error, 'b' has been freed already. */ >> +static bool >> +xlate_fixup_actions(struct ofpbuf *b, const struct nlattr *actions, >> + size_t actions_len) >> +{ >> + const struct nlattr *a; >> + unsigned int left; >> + >> + NL_ATTR_FOR_EACH_UNSAFE (a, left, actions, actions_len) { >> + int type = nl_attr_type(a); >> + >> + switch ((enum ovs_action_attr) type) { >> + case OVS_ACTION_ATTR_HASH: >> + case OVS_ACTION_ATTR_PUSH_VLAN: >> + case OVS_ACTION_ATTR_POP_VLAN: >> + case OVS_ACTION_ATTR_PUSH_MPLS: >> + case OVS_ACTION_ATTR_POP_MPLS: >> + case OVS_ACTION_ATTR_SET: >> + case OVS_ACTION_ATTR_SET_MASKED: >> + case OVS_ACTION_ATTR_TRUNC: >> + case OVS_ACTION_ATTR_OUTPUT: >> + case OVS_ACTION_ATTR_TUNNEL_PUSH: >> + case OVS_ACTION_ATTR_TUNNEL_POP: >> + case OVS_ACTION_ATTR_USERSPACE: >> + case OVS_ACTION_ATTR_RECIRC: >> + case OVS_ACTION_ATTR_CT: >> + ofpbuf_put(b, a, nl_attr_len_pad(a, left)); >> + break; >> + >> + case OVS_ACTION_ATTR_CLONE: >> + /* If the clone action has been fully xlated, it can >> + * be skipped, since any actions executed within clone >> + * do not affect the current packet. >> + * >> + * When xlating actions wihtin clone, the clone action, > > “within” > >> + * because it is an nested netlink attribute, do not have >> + * a vlaid 'nla_len'; it will be zero instead. Skip > > “valid” > >> + * the clone heaer to find the start of the actions > > “header” > >> + * enclosed. Treat those actions as if they are written >> + * outside of clone. */ >> + if (!a->nla_len) { >> + bool ok; >> + if (left < NLA_HDRLEN) { >> + goto error; >> + } >> + >> + ok = xlate_fixup_actions(b, nl_attr_get_unspec(a, 0), >> + left - NLA_HDRLEN); > > If we refactored nested netlink code so that the unclosed nla_len is > NLA_HDRLEN we could simply use “continue;” here (and below) instead of a > recursive call. This is an interesting idea. But it is beyond the scope of the patch. > >> + if (!ok) { >> + goto error; >> + } >> + } >> + break; >> + >> + case OVS_ACTION_ATTR_SAMPLE: >> + if (!a->nla_len) { >> + bool ok; >> + if (left < NLA_HDRLEN) { >> + goto error; >> + } >> + const struct nlattr *attr = nl_attr_get_unspec(a, 0); >> + left -= NLA_HDRLEN; >> + >> + while (left > 0 && >> + nl_attr_type(attr) != OVS_SAMPLE_ATTR_ACTIONS) { >> + /* Only OVS_SAMPLE_ATTR_ACTIONS can have unclosed >> + * nested netlink attribute. */ >> + if (!attr->nla_len) { >> + goto error; >> + } >> + >> + left -= NLA_ALIGN(attr->nla_len); >> + attr = nl_attr_next(attr); >> + } >> + > > So we are effectively ignoring the sample probability here? Yes, we are removing the sample envelope here. > >> + if (left < NLA_HDRLEN) { >> + goto error; >> + } >> + >> + ok = xlate_fixup_actions(b, nl_attr_get_unspec(attr, 0), >> + left - NLA_HDRLEN); >> + if (!ok) { >> + goto error; >> + } >> + } >> + break; >> + >> + case OVS_ACTION_ATTR_UNSPEC: >> + case __OVS_ACTION_ATTR_MAX: >> + OVS_NOT_REACHED(); >> + } >> + } >> + >> + return true; >> + >> +error: >> + ofpbuf_delete(b); >> + return false; >> +} >> + >> +static bool >> +xlate_execute_odp_actions(struct dp_packet *packet, >> + const struct nlattr *actions, int actions_len) >> +{ >> + struct dp_packet_batch batch; >> + struct ofpbuf *b = ofpbuf_new(actions_len); >> + >> + if (!xlate_fixup_actions(b, actions, actions_len)) { >> + return false; >> + } >> + >> + dp_packet_batch_init_packet(&batch, packet); >> + odp_execute_actions(NULL, &batch, false, b->data, b->size, NULL); >> + ofpbuf_delete(b); >> + >> + return true; >> +} >> + >> static void >> execute_controller_action(struct xlate_ctx *ctx, int len, >> enum ofp_packet_in_reason reason, >> uint16_t controller_id, >> const uint8_t *userdata, size_t userdata_len) >> { >> - struct dp_packet_batch batch; >> struct dp_packet *packet; >> >> ctx->xout->slow |= SLOW_CONTROLLER; >> @@ -3825,10 +3962,12 @@ execute_controller_action(struct xlate_ctx *ctx, int >> len, >> } >> >> packet = dp_packet_clone(ctx->xin->packet); >> - dp_packet_batch_init_packet(&batch, packet); >> - odp_execute_actions(NULL, &batch, false, >> - ctx->odp_actions->data, ctx->odp_actions->size, >> NULL); >> - >> + if (!xlate_execute_odp_actions(packet, ctx->odp_actions->data, >> + ctx->odp_actions->size)) { >> + xlate_report_error(ctx, "Failed to execute controller action"); >> + dp_packet_delete(packet); >> + return; >> + } >> /* 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 >> * it will get translated back to OFPR_ACTION for those versions. */ >> -- >> 1.8.3.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
