> On Nov 24, 2015, at 10:27 AM, Ben Pfaff <[email protected]> wrote:
>
> On Fri, Nov 06, 2015 at 04:10:49PM -0800, Jarno Rajahalme wrote:
>> When modifying an existing datapath flow with recirculation actions,
>> the references to old (if any) recirculation actions need to be freed,
>> and references to new recirculation actions need to be stored.
>>
>> Signed-off-by: Jarno Rajahalme <[email protected]>
>> Acked-by: Joe Stringer <[email protected]>
>
> Our coding style calls for a new-line after "void":
>> +static void reval_op_init(struct ukey_op *op, enum reval_result result,
>> + struct udpif *udpif, struct udpif_key *ukey,
>> + struct recirc_refs *recircs,
>> + struct ofpbuf *odp_actions)
>
Fixed.
> Here, it wasn't obvious to me why the logic changed from only allocating
> a recirc_id if we have a packet, to always allocating one (don't we
> still need to reuse the recirc id from a previous translation?):
>
The separation of the packet (upcall) and no packet (revalidation) was suitable
before we added the support for modifying datapath flows in-place, when only
actions change. Before, when doing revalidation the produced actions were only
used for comparison, but now they can also be used as a replacement for the old
datapath actions. This is why we now need to allocate and hold a reference to a
recirculation context also when revalidating. The reference will be freed if
the actions are freed without installing them to an existing flow. Also, the
recirc_alloc_id_ctx() will reuse existing recirculation contexts (and adding a
reference) if possible. I’ll update the comment to mention this.
Jarno
>> @@ -3588,30 +3588,16 @@ compose_recirculate_action__(struct xlate_ctx *ctx,
>> uint8_t table)
>> .ofpacts = ctx->action_set.data,
>> };
>>
>> - /* Only allocate recirculation ID if we have a packet. */
>> - if (ctx->xin->packet) {
>> - /* Allocate a unique recirc id for the given metadata state in the
>> - * flow. The life-cycle of this recirc id is managed by
>> associating it
>> - * with the udpif key ('ukey') created for each new datapath flow.
>> */
>> - id = recirc_alloc_id_ctx(&state);
>> - if (!id) {
>> - XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id");
>> - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
>> - return;
>> - }
>> - xlate_out_add_recirc(ctx->xout, id);
>> - } else {
>> - /* Look up an existing recirc id for the given metadata state in the
>> - * flow. No new reference is taken, as the ID is RCU protected and
>> is
>> - * only required temporarily for verification.
>> - * If flow tables have changed sufficiently this can fail and we
>> will
>> - * delete the old datapath flow. */
>> - id = recirc_find_id(&state);
>> - if (!id) {
>> - ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
>> - return;
>> - }
>> + /* Allocate a unique recirc id for the given metadata state in the
>> + * flow. The life-cycle of this recirc id is managed by associating it
>> + * with the udpif key ('ukey') created for each new datapath flow. */
>> + id = recirc_alloc_id_ctx(&state);
>> + if (!id) {
>> + XLATE_REPORT_ERROR(ctx, "Failed to allocate recirculation id");
>> + ctx->error = XLATE_NO_RECIRCULATION_CONTEXT;
>> + return;
>> }
>> + recirc_refs_add(&ctx->xout->recircs, id);
>
> Thanks,
>
> Ben.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
