> On Mar 3, 2015, at 3:04 PM, Ben Pfaff <b...@nicira.com> wrote:
>
> On Tue, Mar 03, 2015 at 11:22:43AM -0800, Ben Pfaff wrote:
>> I need to continue reading this patch starting from
>> ofproto-dpif-upcall.c.
>
> Continuing:
>
> ofproto-dpif-upcall.c
> ---------------------
>
> s/deleate/delete/:
> + * relies on OVS userspace internal state, we need to deleate all old
>
Done.
>
> ofproto-dpif-upcall.h
> ---------------------
>
> It might be nice to add a helper to report whether the recircs are
> inline, since all the existing helpers check that.
>
> Any particular reason the new recirc_id_node parameter to
> xlate_actions() isn't part of xlate_in?
>
I guess none, I’ll put it in there, keeps the function prototypes simpler,
thanks!
>
> ofproto-dpif-xlate.c
> --------------------
>
> This commit introduces much more extensive use of the 'exit' member of
> xlate_ctx. Until now, I think the usage was simple: if the 'exit'
> action was encountered, or some action had something like a "fatal
> error", we set 'exit', which stopped translating actions. Now there's
> something more subtle going on that I don't understand yet. Can you
> tell me a little more about it?
Right, if we are exiting due to recirculation, the new code stores the
remaining actions and adds unroll actions to restore the pipeline state when
the stored actions are executed later.
We signal the ‘exit due to recirculation’ via changing the
‘recirc_action_offset’ from the default -1 to a zero or a positive value.
>
> There's what looks like a really complicated dance going on with
> recirc_action_offset and adding actions to the action set. I'm having
> trouble figuring out what's going on. So far, it appears to me that
> it's basically unrolling the OpenFlow invocation call stack, but
> that's not really clear to me. Can you explain a little bit?
>
I decided to extend the use of the action_set ofpbuf instead of adding another
for recirculation alone. 'recirc_action_offset’ is an offset to the
‘action_set’, indicating where the action set ends and where the
post-recirculation actions start.
Note that the 'recirc_action_offset’ is NOT an offset of the datapath RECIRC
action in the xout->odp_actions.
> In struct xlate_ctx, the passive voice here forces the reader to guess
> what code must do the associating. I think it probably means that
> xlate_actions()'s caller must do it, but it would be nice to say that:
> + /* These are used for non-bond recirculation. The recirculation IDs are
> + * stored in xout and must be associated with a datapath flow (ukey),
> + * otherwise they will be freed when the xout is uninitialized. */
> + int recirc_action_offset; /* Offset in 'action_set' to actions to be
> + * executed after recirculation, or -1. */
> + int last_unroll_offset; /* Offset in 'action_set' to the latest
> unroll
> + * action, or -1. */
>
Action translation is done before the datapath flow is installed. It is the
responsibility of the caller to take the recirculation ID references from the
xout before xout is uninitialized if the recirculation contexts should be kept
around beyond the 250ms lingering period.
> This comment in compose_output_action__() jumped out at me. Why do
> you think that the action set should be executed before outputting to
> a patch port?
> + /* XXX: Action set should be executed (xlate_action_set()) before
> + * output to a patch port? */
I was confused about the action set semantics here. Indeed, if the output (to a
patch port) was int the action set, all the other actions in the set would
already have been executed. If the output action (to a patch port, or any other
port) is not in the action set, then that output action is executed at the
point is is present in the pipeline without any actions in the set being
executed. Maybe one part of the confusion was the language in the OF 1.4 spec
stating that if there is no output or group action in the action set the packet
is dropped. This language does not consider any output or group actions outside
of the action set.
>
> In xlate_actions(), the use of - here instead of != or > looks odd to me:
> + /* Restore recirculation actions. These include unroll actions to
> + * restore the pipeline metadata. */
> + if (recirc->ofpacts_len - recirc->action_set_len) {
>
The logic here is that the difference gives the length of the
post-recirculation actions in recirc->ofpacts, and is this is zero there are no
actions to be executed.
>
> I need to read more but I think I need some more explanations first.
Thanks Ben,
Jarno
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
