[ovs-dev] [RFC PATCH 4/4] Add packet recirculation

[Jesse Gross]

On Mon, Mar 18, 2013 at 6:34 PM, Simon Horman <horms at verge.net.au> wrote:
> On Mon, Mar 18, 2013 at 01:49:43PM -0700, Jesse Gross wrote:
>> However, do we actually want to tie this directly to recirculation as
>> opposed to as a separate action?  I see possible benefits of
>> separating it out:
>
> I'm not really sure that I understand. Could you explain
> how you see this working?

Just a set action plus recirculation with no argument.  Separating out
the issues of reusing skb mark, this could be done today with
something like:
pass 1: match MPLS -> pop_mpls, set(mark(X)), recirculate
pass 2: match mark X, match MPLS -> pop_mpls, recirculate
pass 3: match mark X, match IP -> output

>>  * It makes it more generic - we could potentially use skb_mark
>> directly or worst case introduce a new metadata field if we are
>> worried about conflicting uses (although we could always set the real
>> skb mark on the last pass).  Either way there is a better potential
>> for reuse.
>
> I'm a bit wary of using skb_mark, what if its desirable to
> use it for something else?

Strictly speaking, I don't think that it can conflict because
recirculation is completely internal to OVS where as all other uses of
mark are external.  We can always restore the mark to whatever we want
before outputting (even if outputs and recirculation are interleaved,
we should have worst case we can set the appropriate value right
before each action).

This might complicate a few cases but I think those are likely to be
very rare and it reduces our match size, which is the common pain
point.

>> > +               }
>> > +               OVS_CB(skb)->flow = NULL;
>>
>> I think the check for having OVS_CB(skb)->flow already set is dead
>> code at this point and we don't need to special case it.
>
> Ok, I'll prepare a preparatory patch to remove the dead code
> and build on top of that. It should simplify the code a bit :)

I actually fixed this after I noticed it here, so the dead code should
already be removed from master.

>> > @@ -905,10 +929,16 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>> >                 goto err_unlock;
>> >
>> >         local_bh_disable();
>> > -       err = ovs_execute_actions(dp, packet);
>> > +       err = ovs_execute_actions(dp, packet, NULL);
>> >         local_bh_enable();
>> >         rcu_read_unlock();
>> >
>> > +       if (err > 0) {
>> > +               /* Recirculation is invalid on packet execute */
>> > +               err = -EINVAL;
>> > +               goto err_flow_free;
>> > +       }
>>
>> Isn't this going to add a lot of complication to userspace?  It's
>> clearly possible for userspace to not use recirculation since it has
>> the full packet but it's basically going to require a separate
>> processing path just to handle this.
>
> I think that the path needs to be present in order to handle
> cases where facets aren't present. Such misses handled without facets
> and packet out handling.

I think it's possible to handle those cases in the kernel but I can
see how that would add complications of its own.  It's certainly easy
enough to disallow this for now and enable it in the future if
necessary.