[ovs-dev] [PATCH 1/3] ofproto-dpif: Enhance execute_controller_action().

Andy Zhou azhou at ovn.org
Mon Mar 6 22:55:15 UTC 2017


On Thu, Mar 2, 2017 at 2:59 PM, Jarno Rajahalme <jarno at ovn.org> wrote:
> With the notes below:
>
> Acked-by: Jarno Rajahalme <jarno at ovn.org>

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 <azhou at ovn.org> 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 <azhou at ovn.org>
>> ---
>> 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
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list